-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Add pagination for search based fetchers #13490
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
Conversation
jabgui/src/main/java/org/jabref/gui/importer/ImportEntriesViewModel.java
Outdated
Show resolved
Hide resolved
|
Your code currently does not meet JabRef's code guidelines. We use Checkstyle to identify issues. You can see which checks are failing by locating the box "Some checks were not successful" on the pull request page. To see the test output, locate "Tests / Checkstyle (pull_request)" and click on it. In case of issues with the import order, double check that you activated Auto Import. You can trigger fixing imports by pressing Ctrl+Alt+O to trigger Optimize Imports. Please carefully follow the setup guide for the codestyle. Afterwards, please run checkstyle locally and fix the issues, commit, and push. |
- Clone and create web* (WebImportEntriesDialog and WebImportEntriesDialogViewModel for web implementation) - Cache prev pages in case of page based fetcher
- Enhance the background fetching mechanism for page-based search entries - Fix 'Import Entries' button functionality - Fix display of total and selected items counters - Fix selection buttons behavior (unselectAll, selectAllNewEntries, selectAllEntries)
- generalized fetching of entries for all page-search-based fetcher - improved pageNumberLabel UI - update JabRef_en.properties
koppor
left a comment
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.
Much code is copied - should not happen. Can you unify? Maybe show and hide buttons? Treat fetching of non-pages as special case?
jabgui/src/main/java/org/jabref/gui/importer/WebImportEntriesDialog.java
Outdated
Show resolved
Hide resolved
…favor of integrating pagination into ImportEntriesDialog.
jabgui/src/main/java/org/jabref/gui/importer/ImportEntriesViewModel.java
Outdated
Show resolved
Hide resolved
jabgui/src/main/java/org/jabref/gui/importer/ImportEntriesDialog.java
Outdated
Show resolved
Hide resolved
jabgui/src/main/java/org/jabref/gui/importer/ImportEntriesViewModel.java
Outdated
Show resolved
Hide resolved
jabgui/src/main/java/org/jabref/gui/importer/ImportEntriesViewModel.java
Outdated
Show resolved
Hide resolved
|
Thank you for working on this feature! I'm one of persons who wants it |
subhramit
left a comment
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.
Hey, sorry for keeping you waiting for so long.
The maintainers were a bit busy.
I have left some more comments - once you address them, IMO should be good to merge.
jabgui/src/main/java/org/jabref/gui/importer/ImportEntriesViewModel.java
Outdated
Show resolved
Hide resolved
jabgui/src/main/java/org/jabref/gui/importer/ImportEntriesViewModel.java
Outdated
Show resolved
Hide resolved
jabgui/src/main/java/org/jabref/gui/importer/ImportEntriesViewModel.java
Outdated
Show resolved
Hide resolved
jabgui/src/main/java/org/jabref/gui/importer/ImportEntriesViewModel.java
Outdated
Show resolved
Hide resolved
jabgui/src/main/java/org/jabref/gui/importer/ImportEntriesDialog.java
Outdated
Show resolved
Hide resolved
InAnYan
left a comment
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.
Great! It works!
Though, there is small UI problem, it shows new entry quicker than updating the "Fetching..." label. As soon as new entries are fetched, the label should be updated to "%0 of %1".
Overall, everything's good!
- Added status label to display loading messages and entry status. - Implemented bindings to enable/disable pagination buttons based on loading state and current page. - Updated next/previous page button actions to handle fetching more entries when on the last page. - Introduced initialLoadComplete property to manage the loading state more effectively.
There is no UI issue, I made fetching of entries when user is on last page, since 1st page is last page initially so dialog was showing 'Fetching...' and when user lands on last page it was made to auto fetch entries and show 'Fetching...' instead of 3 of 3 which is last page. I have enhanced the UI which will now provide comprehensive feedback to user when they are on last page and want to navigate further. PR description ss has been updated. |
InAnYan
left a comment
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.
Thanks for your dedication! I think after you'll address others comments, then we can merge this PR
- Refactor ImportEntriesViewModel and ImportEntriesDialog to use Optional for fetcher and query.
|
@trag-bot didn't find any issues in the code! ✅✨ |
Closes #5507
This PR introduce pagination for web-search results.
Steps to test
Mandatory checks
CHANGELOG.mddescribed in a way that is understandable for the average user (if change is visible to the user)