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

Search floating mode #11510

Merged
merged 65 commits into from
Aug 4, 2024
Merged

Search floating mode #11510

merged 65 commits into from
Aug 4, 2024

Conversation

LoayGhreeb
Copy link
Collaborator

@LoayGhreeb LoayGhreeb commented Jul 17, 2024

This PR Fixes #9073, Fixes #4237, Fixes #3176.

  • Improved performance when opening multiple libraries. Previously, updating the search term or selected groups in one library triggered filtering in all open libraries. Now, it only updates the current library. Each table now listens to the library groups/query instead of a global property from the StateManager.

  • Reintroduced floating search in the main table, with two new toggles to control which entries are shown:

    • Toggle to filter search results: When enabled and a search query is active, only entries matching the search are shown.
    • Toggle to filter selected groups: When enabled and a group is selected, only entries matching the group are shown.
    • In floating mode, entries are "ranked" based on their state and sorted by rank (1 = matches both search term and selected groups, 2 = matches search term, 3 = matches selected groups, 4 = does not match either). Each rank has a custom CSS style.
    • Added shortcut right, and left arrows to scroll to the next/previous rank.
  • Added a toggle to invert the selected groups, showing entries that do not match the selected group(s).

image

Mandatory checks

  • Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
  • Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

@Siedlerchr
Copy link
Member

On the first glimpse looks good, I am just not sure if the effect of union/intersection is kept : https://docs.jabref.org/finding-sorting-and-cleaning-entries/groups#display-union-or-intersection-of-groups

@Siedlerchr
Copy link
Member

@AEgit will be happy to hear that we are bringin back the floating mode!

@LoayGhreeb
Copy link
Collaborator Author

On the first glimpse looks good, I am just not sure if the effect of union/intersection is kept

Yes, still working

@LoayGhreeb
Copy link
Collaborator Author

@ThiloteE, @ryan-carpenter, @AEgit, could you test the build from this PR and provide your feedback?

@AEgit
Copy link

AEgit commented Jul 17, 2024

JabRef 5.16-PullRequest11510.31--2024-07-17--9fef618
Windows 10 10.0 amd64
Java 21.0.2
JavaFX 22.0.1+7

Wow, as @Siedlerchr said, I have long been waiting for the comeback of the floating mode ^^
Really glad to see it back, well done @LoayGhreeb

I don't have much time for testing these days, but might have more time over the weekend.

Based on first observations this comes close to what old JabRef 3.8.2 did, in fact it adds functionality that was previously not present.
(1) As far as I can tell, the main switch to turn on the floating mode is actually the "filter groups" icon in the group panel - maybe not intuitive first (but that could also just be me, still being used to the old behaviour). Once you understand that, though, it appears clear, what needs to be done to activate floating mode.
(2) I am not completely sure yet, how "filter search results" interacts with the floating mode - I will have to put more effort into checking this.
(3) To come closer to the old behaviour, it might be necessary to switch the positions of state 2 and state 3 in the float view, but I have to check this first properly to confirm it.
(4) For larger databases (>20k articles) there appears to be a performance penalty at the moment when performing the search (though I am not sure, whether this is even related to this commit, or was already the case before). Again, I will have to check this further to see, whether that is the case.

Anyway, awesome to see this feature re-introduced to JabRef! With that, I might be able to switch from good old JabRef 3.8.2 to the new JabRef for my actual work.

@calixtus
Copy link
Member

calixtus commented Jul 18, 2024

I might be able to switch from good old JabRef 3.8.2 to the new JabRef for my actual work.

Don't forget to make a backup of your data. 😁

@LoayGhreeb
Copy link
Collaborator Author

LoayGhreeb commented Jul 18, 2024

I don't have much time for testing these days, but might have more time over the weekend.

Based on first observations this comes close to what old JabRef 3.8.2 did, in fact it adds functionality that was previously not present.

Thanks for the quick reply and your feedback.

I managed to install the old version for comparison. I found there is no difference between the old version and this PR when using the "filter search" toggle.

The only difference between the two versions is the label showing the number of search results. In this PR, the number shown represents entries that match both the group and search query.

The "filter groups" toggle is an addition not present in the old version. In the old version, when you select a group, you still see all entries whether in float or filter mode, without the option to only show entries matching the selected group.

So, if you prefer the old behavior, you only need to toggle on/off "filter search" and disable "filter groups."

(4) For larger databases (>20k articles) there appears to be a performance penalty at the moment when performing the search (though I am not sure, whether this is even related to this commit, or was already the case before). Again, I will have to check this further to see, whether that is the case.

I tried to compare search performance before and after this PR with a larger library and found no difference. (This PR should improve performance when multiple libraries are opened).

@Siedlerchr
Copy link
Member

Overall looks good to me, just a question about the updateItem stuff, this looks suspicious to me

@LoayGhreeb
Copy link
Collaborator Author

There is an issue in floating mode where adding an entry to the selected group does not update the entry's rank or color.
This should be fixed by tobiasdiez/EasyBind#87.

@Siedlerchr
Copy link
Member

We plan to migrate the Easybind repo to JabRef org, but that might take some time to get it pushed to maven central

@AEgit
Copy link

AEgit commented Jul 21, 2024

JabRef 5.16-PullRequest11510.31--2024-07-17--9fef618
Windows 10 10.0 amd64
Java 21.0.2
JavaFX 22.0.1+7

I had a further look at the version - so far, looks good to me. As @LoayGhreeb mentioned, you can recreate the floating mode of the old version if you select the appropriate filter steps (tunring off "Filter by groups" and turning off/on - whatever you prefer - "Filter search results").

I note the following:

  1. Select a group containing some items/articles. Perform a search. Then select some items that match the search term but do not belong to the group. Assign them to the group. These items will not automatically be shown/coloured in the search panel as belonging to the group. Only if you select another group and then select the originally selected group again will these items/articles be shown/coloured accordingly. To put it differently: the search preview that is shown does not automatically update when items are assigned to new groups. It only happens after the group is selected a 2nd time. This is not a major problem to me, but I wanted to let you know about it.
  2. It might be worthwhile enhancing the colours of state 2 (contains search term but does not belong to group) and state 3 (does not contain search term but belongs to group), i. e. making them darker. At the moment, especially the greyish colour of state 2 is very similar to the grey in state 1 (state 1, which are items that contain both the search term and belong to the selected group, will display lines alternating white and grey colours when multiple elements are present).
  3. The current search comes with a performance penalty for large databases (>20,000 items) - again I cannot say whether this is related to this change or was already present before (I have not checked the current dev version in a while, so this issue might have been present for a while). When a search is performed, it will not be instant, but it will take some time, until the items conforming to the search term appear. Might be like 2 or 3 sec - the older version was definitely faster in this regard (though might also not be instant, but it definitely did not take as long, maybe max 0.5 to 1 sec).
    It also affects other parts of JabRef. When a search term is present and you select a different group in the groups panel, it takes again a few sec for the contents of the newly selected group (including the filtering approach as given by the still present search term) to appear. I reckon this is because the search is applied every time that a new group is selected. The outcome is correct and in fact matches the search + the group selection, but, as I said, it comes with a performance penalty. On the other hand, if you select a new group without any search terms entered, then the contents of the group appear nearly instantly.
    I wonder whether I should open a separate ticket for these performance issues.

Otherwise, though, seems really fine to me so far - very happy again to see this change implemented!

calixtus
calixtus previously approved these changes Aug 4, 2024
@calixtus calixtus enabled auto-merge August 4, 2024 20:12
@calixtus calixtus added this pull request to the merge queue Aug 4, 2024
@LoayGhreeb LoayGhreeb removed this pull request from the merge queue due to a manual request Aug 4, 2024
@LoayGhreeb LoayGhreeb dismissed stale reviews from calixtus and koppor via c6cbeb5 August 4, 2024 20:18
@koppor koppor mentioned this pull request Aug 4, 2024
7 tasks
koppor
koppor previously approved these changes Aug 4, 2024
@LoayGhreeb LoayGhreeb added this pull request to the merge queue Aug 4, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 4, 2024
@calixtus calixtus enabled auto-merge August 4, 2024 21:07
@calixtus calixtus added this pull request to the merge queue Aug 4, 2024
Copy link
Contributor

github-actions bot commented Aug 4, 2024

The build for this PR is no longer available. Please visit https://builds.jabref.org/main/ for the latest build.

Merged via the queue into main with commit db9f83c Aug 4, 2024
26 of 28 checks passed
@calixtus calixtus deleted the floating-mode branch August 4, 2024 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
project: GSoC search status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers type: feature
Projects
None yet
6 participants