Fix context menu of the search bar

[LoayGhreeb]

• Fixed the context menu of the search bar. (It was using the EditAction that sometimes copy/paste/cut the selected entries instead of the text of the search bar).
• Added a shortcut (Ctrl+Shift+F) to open the global search window.
• Stored the divider position of the global search window to the preferences.
• Updated the color of the modifier buttons in dark mode. Lucene search backend #8963 (comment)
• Fixes https://github.com/JabRef/jabref-issue-melting-pot/issues/12

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.

[koppor]

Human testing for tab (library) switching:

1. Open two libraries A and B
2. Search in library A using query q1
3. Switch to library B
4. Search results should match q1 (and not no query)

[Siedlerchr]

Test with the different modes, global search and search across libraries

[LoayGhreeb]

I've removed this validator because it is not listening for changes of the modifier buttons, an exception will be thrown in this case: write an invalid regex in the search bar, with the regex modifier disabled, then enable the regex button. will throw java.util.regex.PatternSyntaxException: Unclosed group near index 4 (k\)

Also, this visualizer throws an exception in the global search window. (Open the window, enable regex modifier, write an invalid query, close the window, and open it again will throw java.lang.IllegalArgumentException: DialogPane@30d0220[styleClass=dialog-pane]is already inside a scene-graph and cannot be set as root.

[HoussemNasri]

Have you tried EasyBind#combine to combine listening for searchField text property and change of selection of the modifier button? Overall, I think we should stick to one pattern to display errors to users, unless it is necessary to implement a workaround.

[LoayGhreeb]

Yes, using EasyBind#combine works, but for the second issue, the global search window will not open again if it was closed with the visualizer.

However, we already show a message indicating that the query is invalid.

With the visualizer, we only add an icon before the search bar.

[HoussemNasri]

Okay, I see. If it's necessary then let's keep it.

[LoayGhreeb]

org.jabref.gui.preview.PreviewViewer needs to be refactored to register the listener for the library's search query instead of the StateManager. This will be done in a follow-up PR.

jabref/src/main/java/org/jabref/gui/preview/PreviewViewer.java

Lines 171 to 176 in e6b5470

	if (!registered) {
	stateManager.activeSearchQueryProperty().addListener(listener);
	stateManager.activeGlobalSearchQueryProperty().addListener(listener);
	registered = true;
	}
	highlightSearchPattern();

This class is used in multiple locations, most of which don't have access to the LibraryTab to retrieve the SearchQueryProperty (maybe most of the PreviewViewer instances don't need to highlight the search pattern. Highlighting is needed only in the entry editor and the global search window).

[calixtus]

I noticed you make several changes and fixes to the ui. Please don't forget to create screenshots before and after the change.

[HoussemNasri]

Although the ctrl+shift+F shortcut does work, I couldn't find it in preferences:

[LoayGhreeb]

Although the ctrl+shift+F shortcut does work, I couldn't find it in preferences:

I can see it on my machine, but that might be because I cleared the preferences after adding the shortcut.

Is there a way to add it without clearing the preferences?

[calixtus]

maybe a preference migration must be added to test if there is already a shortcut registered, and if not, do so?

[HoussemNasri]

maybe a preference migration must be added to test if there is already a shortcut registered, and if not, do so?

Exactly.

Clearing preferences works because when you pass an empty list of bindings to KeyBindingRepository (which are meant to be loaded from preferences but prefs is cleared now) it would loop through all items in the KeyBinding enum and set the binding to their default.

We should have a migration strategy that would merge the bindings loaded from preferences and key bindings from KeyBinding, keeping the binding set by the user and loaded from preferences if it exists and setting the default value if it does not.

The migration can be added to JabRefPreferences#getKeyBindingRepository where it would synchronize the KeyBinding enum and preferences, by removing values from preferences that are not present in KeyBinding and adding values to preferences that are present in KeyBinding but not in preferences to their default value.

We should also consider versioning preferences because doing the migration could cause problems when running an older version of JabRef.

No version in the path of prefs.xml:

[calixtus]

Note that the current impl of the preferences default map will change soon

[LoayGhreeb]

Note that the current impl of the preferences default map will change soon

Should I make any changes to the preferences in this PR? This issue is related to the preference design, and it's not ideal to add a preference migration for every new shortcut.

[HoussemNasri]

Should I make any changes to the preferences in this PR? This issue is related to the preference design, and it's not ideal to add a preference migration for every new shortcut.

I think we should postpone it until the default map change is completed to avoid any conflicts. Plus I have noticed other keybindings are not present in preferences, so the issue goes beyond this PR. I created an issue to track it https://github.com/JabRef/jabref-issue-melting-pot/issues/471