Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remember sort order and pagination of the process and search result list #5358

Merged
merged 12 commits into from
Oct 13, 2022

Conversation

thomaslow
Copy link
Collaborator

Related Issues:

This push request enables the option "multiViewState" for the process list and search result list. It preserves the state of the pagination and column sorting in the current session (while visiting other pages).

Process List Demo:

simplescreenrecorder-2022-09-21_12.21.45.mp4

The same is enabled for the search result list. However, the state is reset (page=1, ordering=default) for each new search request (in order to show the results in their default ordering by relevance).

Search Result Demo:

simplescreenrecorder-2022-09-21_12.36.34.mp4

Copy link
Member

@solth solth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thomaslow Thanks for the pull request. Great that there was a dedicated attribute for PrimeFaces datatables to facilitate this functionality.

Just two little remarks/questions:

  1. with the current implementation, the view state of the process list is kept, no matter from where the user comes. I think it would be preferable to only retain the sorting and pagination of the list when the user returns to it from some page which is located "deeper" (e.g. corresponding to the breadcrumb depth) than the list, e.g. the "Process edit" and "Metadata editor" pages. In all other cases - when the user navigates to the process list from 1) main menu "Processes" option, 2) desktop "All processes" button and 3) the breadcrumb "Processes" link - the view state should be reset to a default sorting and pagination. Perhaps this could be achieved by saving and updating the multiViewState value to a bean property instead of setting it to a static true?
  2. Kitodo already contains a makeshift way to at least store the pagination of a list when returning to if from another page. This utilizes a custom URL parameter called keepPagination (see BaseForm line 71 and - for example - the command link to open the metadata editor in ProcessList line 163). To avoid potential confusion and functional conflicts, I would suggest to remove at least those usages of the keepPagination parameter that correspond to the search result or process lists in this pull request.

@thomaslow
Copy link
Collaborator Author

@solth I understand what you are saying. Personally, I'm also not a fan of keeping the state of the process list alive over the whole session. I certainly can add code that resets the multiViewState when visiting certain pages or clicking certain buttons (e.g. the main menu or main logo). However, I can't see a way to do this without confusing at least some users that the state is sometimes reset, and sometimes not.

For example, suppose there is a user that:

  1. visits the process list and navigates to page 5
  2. clicks on some other page (e.g. the system page), which resets the process list state
  3. uses the browser "back" button and expects to come back to page 5 of the process list

In the long run, in my opinion, the more user-friendly way would be to implement a classic URL routing mechanism that includes state information in the URL (e.g. processes.xhtml?page=5&order=title_asc&filter=something) such that the browser history can be used correctly, bookmarks work properly, users can share URLs by mail, etc. However, this is way out of the scope of this pull request and related issues.

@solth
Copy link
Member

solth commented Oct 7, 2022

Yes, it could become quite confusing if the view state of the datatable component was reset everytime the user moved away from the process list to some different page - and it would nullify or at least diminish the advantages of this pull request, I think. But that is not what I was suggesting. Instead, my idea was to only reset the view state when the user explicitly uses one of the three main entry points listed above - main menu, desktop link, breadcrumb - to navigate towards the process list. For example, this could be done using a corresponding "reset" viewAction at the beginning of the processList template. Then the view state would remain stable in all other cases and the user could use the browser navigation the way you described.

I agree that we should eventually have parameters to control sorting, pagination and filtering (keepPagination was a first shot at this), but as you say, that is of course out of scope of this pull request.

@thomaslow
Copy link
Collaborator Author

@solth When clicking on the dashboard All processes link and the main menu Processes button, the table state is now reset (page=1, default sorting). For the breadcrumb, I felt it is more appropriate to keep the table state alive when you are at a lower level. For example, with a breadcrumb like this

desktop > processes > edit process

clicking on processes will not reset the table state (which in my opinion was the purpose of the issue #5002 in the first place). However, when you are at the processes list, meaning the breadcrumb

desktop > processes

Clicking the processes link will reload the page and reset the table state. What do you think? Of course, this is debatable (and could be easily changed via a single line edit).

I also removed the keepPagination url parameter for the processes page. I noticed that this parameter is also used for the user, project and task list pages. So, maybe we should use the same behavior via multiViewState also for the user, project and task list tables for a more consistent user experience?

@solth
Copy link
Member

solth commented Oct 13, 2022

I understand your reasoning but I think the processes breadcrumb should not behave differently in different situations. Instead, it should either always or never reset the process list (otherwise, this might just appear as unpredictable or unreliable functionality to the users). I can live with the former (always reset), because this is indeed - like you said - a slightly different situation than the other two links (desktop widget and main menu).
So I propose to unify the behavior of the "processes" breadcrumb link, so that it never resets the view state of the process list.

@thomaslow
Copy link
Collaborator Author

Removed the state-resetting from the breadcrumb desktop > processes and added new issue for remembering sort order in user, task and project list, see #5391.

Copy link
Member

@solth solth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me and works as advertised! Found just one tiny part which might be improved (see comment in code with change suggestion). Otherwise, I have no further change requests for this PR!

@solth solth merged commit 07f3cff into kitodo:master Oct 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Process list should remember its sorting Remember sort order of the process list
2 participants