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
Preserve pagination after actions which trigger a grid update #6546
base: master
Are you sure you want to change the base?
Conversation
5cb17af
to
12589c5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome! I took a pass through the code and didn't see any significant issues.
I need to think more about the implications of starting row vs page # from a UX point of view. Have you asked testers for feedback on the change?
Unfortunately the first thing that I tried, clear recon choice, reset the grid back to the beginning. I'm not sure if I was just unlucky and chose an overlooked operation or there's something the matter with my build. I was clearing the reconciliation choice in preparation for what I really wanted to test which was reconcile a single item manually.
} | ||
total++; | ||
totalRows++; | ||
|
||
return false; | ||
} | ||
|
||
@Override | ||
public boolean visit(Project project, Record record) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to deprecate the old signature or are both useful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's deprecated in the interface already, so I don't think it's necessary to add the @Deprecated
annotation on the implementation again, is it?
p.s. I had rebased your branch onto |
…OpenRefine#5411) * Adapt the GridState interface to separate sorting and pagination. This adds `getRowsBefore` methods, counterpart to the existing pagination methods, so that we can do efficient and correct pagination, solving #33 and OpenRefine#570 * Revert "(I OpenRefine#2638) Feature to Goto a page directly (OpenRefine#2639)" Go back to simply displaying a range of row numbers, because that will enable a solution for #33, OpenRefine#570 and OpenRefine#572. This reverts commit d7aaac2. * Do not move back to the first page when applying an action which refreshes the grid. Instead, the current page is refreshed. This closes #33, closes OpenRefine#570, closes OpenRefine#572. * Specify which actions preserve row and record ids This lets us preserve pagination only when it is guaranteed to show the same part of the data after the operation. * Make paging user-editable like before * Adapt cypress tests
12589c5
to
4dc666e
Compare
Co-authored-by: Tom Morris <[email protected]>
Thanks for the review! Good catch, I simply hadn't enabled the preservation of pagination for the 'Choose new match' button. It should work now. |
This is a backport of #5411 to the master branch. Closes #33, closes #570, closes #572 (already marked as such by #5411).
For this one, although the change was sort of forced on me by the new architecture, it can be backported with reasonable effort to the master branch.
Summary: this changes the grid pagination so that users to not input a page number, but the index of the first row in the view. Why do this?
It's a change that has implications for the UI and the web API (which is not meant to be stable).
It relies on manually annotating all operations with row/record preservation. In this PR it is done in the frontend, but further backend changes will move this information to the backend (making it possible to detect the row/record preservation of sequences of operations run via the
apply-operations
command, for instance).One oddity of this change is that in records mode, the pagination is still done based on row ids and not on record ids. This is intentional, because: