Searchable/Filterable preferences by WtfJoke · Pull Request #4415 · JabRef/jabref

WtfJoke · 2018-10-27T00:22:21Z

Up so far I just created the textbox for searching and fixed a deprecation warning.
I added @paulKra as collaborator on my fork.

Last step so far: I was asking myself how I can access the text of the different Preferences-Categories and I was wondering if I need a new Interface (something like PrefTabContents or PrefTabContainer for it or extend existing PrefsTab Interface.

Change in CHANGELOG.md described
Tests created for changes
Manually tested changed features in running JabRef
Screenshots added in PR description (for bigger UI changes)
Ensured that the git commit message is a good one
Check documentation status (Issue created for outdated help page at help.jabref.org?)

Siedlerchr · 2018-10-27T09:01:37Z

First of all, thanks for your contribtion! 👍

If you look at the code fo the old PreferencesFilterDialog, you see that the data comes from the JabRefPreferencesFilter class in org.jabref.preferences.

If you have further questions you can also ask in out gitter chat. https://gitter.im/JabRef/jabref

Edit// Adjusted my comment

tobiasdiez · 2018-10-27T09:07:06Z

The easiest solution is probably to transverse the complete control node tree and take the text of all label controls.

Siedlerchr · 2018-11-11T15:57:03Z

@WtfJoke @paulKra What is the current status here? The code looks already promising. It would be really nice if you could finish this!

WtfJoke · 2018-11-11T19:40:52Z

@Siedlerchr Its still on my todo list. Havent found the time to investigate further and were stuck on how I display the filtered preferences.

I think @paulKra doesnt work on it, he still didnt accept my invitation link to my forked repo

Siedlerchr · 2018-11-11T20:51:30Z

Okay thanks for the udpate! Take your time. If you have any questions just ask here or in gitter and we try to help

tobiasdiez · 2018-11-11T23:00:47Z

Maybe the JFXHighlighter from https://github.com/jfoenixadmin/JFoenix/ works to highlight the matches

WtfJoke · 2018-11-20T20:55:04Z

@tobiasdiez Thanks for the hint, however I tested it locally but I didnt liked it that much (for example it did mark the buttons for importing/exporting/show/reset preferences also).
If I dont find alternatives I think about using it anways.

still work in progress mapping search to preference and start searching it still needs a way to filter the found items

also be explicit about method visibility

Verified this is not the case: label names dont contain tab names

…ablePreferences

tobiasdiez · 2019-03-13T21:18:02Z

@CaptainDaVinci is this ready for review or do you need to further work on it?
(The first remark you had concerning "Sending emails" is a nice suggestion for further improvement, but not necessary for merging I would say).

CaptainDaVinci · 2019-03-14T13:07:51Z

@tobiasdiez Yes, this is ready for review.

Siedlerchr · 2019-03-16T21:10:06Z

+
+        private final Labeled labeled;
+
+        LabeledWrapper(Labeled _labeled) {


Please do not use an underscore. In java it's more common to do:

Suggested change

LabeledWrapper(Labeled _labeled) {

LabeledWrapper(Labeled labeled){

this.labeled= labeled;

And maybe use a better name to indicate what the purpose of the class actually is. LabeledWrapper sounds a bit too generic for me.

Initial idea with the inner class was to handle Labeled being treated as a key to mapping from Labeled to PrefsTab. Now that the relation is reversed, I don't think the LabeledWrapper class is required anymore.

Siedlerchr

Thanks for the contribution!
In general looks good to me, just some minor code style things.
And also please add a screenshot

Siedlerchr · 2019-03-16T21:15:01Z

+        }
+    }
+
+    PreferencesSearchHandler(List<PrefsTab> preferenceTabs, StringProperty searchText) {


Please remove the StringProperty as constructor argument, instead expose the property through a method:
And initialize the variable with new SimpleStringProperty("")

public StringProperty searchTextProperty() { return this.searchText; }

As I understand, this should then be used to bind to searchBox.textProperty() in PerferencesDialog,
searchHandler.searchTextProperty().bind(searchBox.textProperty()); ?

Also, shouldn't tabsList.itemsProperty().bindBidirectional(searchHandler.filteredPreferenceTabsProperty());
have a unidirectional binding instead?

@Siedlerchr I am facing some issues on implementing this.
I have removed the StringProperty from the constructor argument as per your suggestion.
In PreferencesDialog, the searchHandler.searchStringProperty() is bound to searchBox.textProperty(). However, when the searchBox text changes, it doesn't trigger the ChangeListener for searchText. Unless, I add a statement to listen for changes on searchBox as well.

searchBox.textProperty().addListener((o, p, n) -> { // dummy reference to the text property. searchHandler.searchTextProperty(); });

I am not able to understand the reason for this, is it necessary to have listeners on both the properties which are bound?

Siedlerchr · 2019-03-16T21:16:51Z

+        TextField searchBox = new TextField();
+        searchBox.setPromptText(Localization.lang("Search"));
+
+        PreferencesSearchHandler searchHandler = new PreferencesSearchHandler(preferenceTabs, searchBox.textProperty());


Bind the searchBox.textProperty the same ways as you do with the itemsProperty below

Siedlerchr · 2019-03-16T21:18:20Z

+        return prefsTabLabelMap;
+    }
+
+    protected ListProperty<PrefsTab> getFilteredPreferenceTabsProperty() {


Just the wording, remove the "get" from the name., e.g. just FilteredPreviewPreferencesTabsProperty() to conform to the javafx code naming conventions

Siedlerchr · 2019-03-16T21:19:13Z

+
+        private final Labeled labeled;
+
+        LabeledWrapper(Labeled _labeled) {


And maybe use a better name to indicate what the purpose of the class actually is. LabeledWrapper sounds a bit too generic for me.

tobiasdiez

I've only two small suggestions in addition to the ones of @Siedlerchr.

tobiasdiez · 2019-03-14T20:22:03Z

 }
+
+*:search-highlight {
+	-fx-background-color: #ed7474;


Can you please reuse one of the colors defined in base.css. In case you find no suitable color there, then please define a new one there.

tobiasdiez · 2019-03-16T22:50:21Z

+                }
+            }
+
+            if (tabContainsLabel) {


As you suggested yourself, I think it is a good idea to move the logic of filterByTabName here (if (tabContainsLabel || tabNameIsMatchedBySearchQuery) instead of having two separate methods.

…ndler

CaptainDaVinci · 2019-03-17T12:09:17Z

Thank you for the suggestions @tobiasdiez and @Siedlerchr! I have refactored the code for the most part to account for the changes and will push the code once I have implemented all changes.

Screenshot showing the preferences window when searching:

…ablePreferences

Siedlerchr · 2019-03-20T19:25:36Z

@CaptainDaVinci Look at my patch: (Import via git am < file.patch)
I added the bidiretional binding and a listener in the Handler. This worked for me

0001-add-bidiretional-binding-to-searchProperty.patch.txt

CaptainDaVinci · 2019-03-20T20:01:16Z

@Siedlerchr Thank you for sharing the patch, however, this did not work for me. The change listener for searchText is never called. I had a similar implementation except for not using EasyBind to register the callback.

The only way I could make this work was to add a listener to searchBox as well and refer to the searchText property in that.

searchBox.textProperty().addListener((o, p, n) -> {
    searchHandler.searchTextProperty();     // required, so that changed are reflected.
});

Siedlerchr · 2019-03-20T20:15:28Z

Hm this is odd. It worked fine for me with this line:
searchHandler.searchTextProperty().bindBidirectional(searchBox.textProperty());
Whenever I changed the text in the GUI field, e.g. key to asdfasd the view was updated and the new findings were highlighted.
Maybe I understand you wrong, but I think this is enough?

Under the hood the bidirectional is nothing else than a wrapper around the changeLIstener stuff.

CaptainDaVinci · 2019-03-20T20:19:52Z

Also, note that its necessary to have a reference to searchText property accessed under the ChangeListener for searchBox. Could this be related to some sort of premature garbage collection?

Edit: For me changing the text in the GUI field did not update the view or highlight the labels.

Siedlerchr · 2019-03-20T20:26:21Z

Bindings are created using WeakListener, so they might get garbage collected. But I still don't understand why it works for me with the patch. I would propose you test again with my patch or at least commit the changes and let others test as well.
Unfortunately I am the next days busy at work and probably can't help much.

CaptainDaVinci · 2019-03-20T20:35:08Z

Okay, I have pushed changes, in the mean time I will try testing it again.

P.S: Since the PreferencesSearchHandler does not use the searchText property but only the query text, can we instead bind the searchBox to the filterTabs, i.e, not have to deal with binding the text property and string property?

tobiasdiez · 2019-03-22T10:11:53Z

+        searchBox.setPromptText(Localization.lang("Search"));
+
+        PreferencesSearchHandler searchHandler = new PreferencesSearchHandler(preferenceTabs);
+        tabsList.itemsProperty().bindBidirectional(searchHandler.filteredPreferenceTabsProperty());


I'm not sure if this is the reason for the problem, but it should suffice to replace the filteredPreferenceTabsProperty by an observable list (instead of an list property) and then use the bindContentsBidirectional method to bind tabsList.getItems() to this observable list.

Unfortunately, this did not work either. The problem is that searchHandler.searchText does not reflect the changes from the searchBox, unless an explicit listener is added to searchBox and searchHandler.searchText is simply accessed there.

searchBox.textProperty().addListener((o, p, n) -> { searchHandler.searchTextProperty(); // required, so that changed are reflected. });

tobiasdiez · 2019-03-22T10:13:25Z

+
+        PreferencesSearchHandler searchHandler = new PreferencesSearchHandler(preferenceTabs);
+        tabsList.itemsProperty().bindBidirectional(searchHandler.filteredPreferenceTabsProperty());
+        searchHandler.searchTextProperty().bindBidirectional(searchBox.textProperty());


Another try would be to pass the searchBox.textProperty() as the constructor argument and then add the listener directly (I think you had something similar before). I think both approaches are valid although it's strange that the current code does not work for you.

Indeed, that was the previous approach.
How about another approach where we add a listener on searchBox, which makes call to searchHandler.filterTabs directly, without the need of having StringProperty in between?

Yes, that sounds like the easiest solution. Please go ahead and make the corresponding changes. Thanks!

@tobiasdiez I have implemented the required changes in the recent commit.

tobiasdiez · 2019-03-27T17:59:17Z

Thanks to both @WtfJoke and @CaptainDaVinci for joining forces!

WtfJoke force-pushed the searchablePreferences branch from 9fb4e3e to 1f16301 Compare October 27, 2018 14:41

WtfJoke force-pushed the searchablePreferences branch 2 times, most recently from 488540f to 3ac59ef Compare November 18, 2018 18:01

Siedlerchr requested changes Nov 18, 2018

View reviewed changes

Comment thread src/main/java/org/jabref/gui/preferences/PreferencesSearchHandler.java Outdated

Siedlerchr reviewed Nov 19, 2018

View reviewed changes

Comment thread src/main/java/org/jabref/gui/preferences/PreferencesSearchHandler.java Outdated

Siedlerchr reviewed Nov 20, 2018

View reviewed changes

Comment thread src/main/java/org/jabref/gui/preferences/PreferencesDialog.java Outdated

WtfJoke added 17 commits November 20, 2018 23:42

add search box to preferences

0c8504a

replace deprecated method call

29907be

Fix codestyle/checkstyle

dd410d4

Add Preferences Search Handler

1956581

still work in progress mapping search to preference and start searching it still needs a way to filter the found items

Remove unused variable

c3223f7

Add a list of filtered preftabs instead of filtered preftabnames

cbc10f2

🎉 Add the actual filtering of the elements

2fdf875

Favor SimpleListProperty over SimpleObjectProperty

3f1833c

Fix Imports

20e79c0

also be explicit about method visibility

Add filtering of labels

43ad5ff

Remove some unneeded member variables

2eb9892

extract filterBy methods

22112a4

Fix Type of ListProperty

ee690ec

Remove todo

20f358f

Verified this is not the case: label names dont contain tab names

Reformat code

6e0358f

Favor regular List over ObservableList

71feb3a

Use existing translated text for prompt

851d807

CaptainDaVinci added 5 commits March 12, 2019 01:35

Replacing map by multimaps and changing the mapping relation

e4e75b0

Merge branch 'master' of https://github.com/JabRef/jabref into search…

8d169b3

…ablePreferences

Merge branch 'master' of https://github.com/JabRef/jabref into search…

69fa5f3

…ablePreferences

Fixing checkstyle error

c0df62d

Added Locale.ROOT when converting to lower case

0c8e103

Removed redundant comments

12613e6

tobiasdiez added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Mar 14, 2019

tobiasdiez changed the title ~~[WIP] Searchable/Filterable preferences~~ Searchable/Filterable preferences Mar 14, 2019

Siedlerchr reviewed Mar 16, 2019

View reviewed changes

Siedlerchr requested changes Mar 16, 2019

View reviewed changes

tobiasdiez requested changes Mar 16, 2019

View reviewed changes

Removed LabeledWrapper inner class and refactored PreferencesSearchHa…

642acaa

…ndler

CaptainDaVinci added 2 commits March 20, 2019 12:07

Merge branch 'master' of https://github.com/JabRef/jabref into search…

b931c4d

…ablePreferences

Fixed checkstyle error

7215296

add bidiretional binding to searchProperty

7229cb1

tobiasdiez reviewed Mar 22, 2019

View reviewed changes

Added change listener on searchBox and removed search text property

b13ebda

tobiasdiez approved these changes Mar 26, 2019

View reviewed changes

Siedlerchr approved these changes Mar 27, 2019

View reviewed changes

tobiasdiez merged commit ef5710c into JabRef:master Mar 27, 2019


		private final Labeled labeled;

		LabeledWrapper(Labeled _labeled) {

	LabeledWrapper(Labeled _labeled) {
	LabeledWrapper(Labeled labeled){
	this.labeled= labeled;

Uh oh!

Conversation

WtfJoke commented Oct 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Siedlerchr commented Oct 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tobiasdiez commented Oct 27, 2018

Uh oh!

Siedlerchr commented Nov 11, 2018

Uh oh!

WtfJoke commented Nov 11, 2018

Uh oh!

Siedlerchr commented Nov 11, 2018

Uh oh!

tobiasdiez commented Nov 11, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

WtfJoke commented Nov 20, 2018

Uh oh!

tobiasdiez commented Mar 13, 2019

Uh oh!

CaptainDaVinci commented Mar 14, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Siedlerchr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CaptainDaVinci Mar 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CaptainDaVinci Mar 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tobiasdiez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CaptainDaVinci commented Mar 17, 2019

Uh oh!

Siedlerchr commented Mar 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CaptainDaVinci commented Mar 20, 2019

Uh oh!

Siedlerchr commented Mar 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CaptainDaVinci commented Mar 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Siedlerchr commented Mar 20, 2019

Uh oh!

CaptainDaVinci commented Mar 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

WtfJoke commented Oct 27, 2018 •

edited

Loading

Siedlerchr commented Oct 27, 2018 •

edited

Loading

CaptainDaVinci Mar 17, 2019 •

edited

Loading

CaptainDaVinci Mar 20, 2019 •

edited

Loading

Siedlerchr commented Mar 20, 2019 •

edited

Loading

Siedlerchr commented Mar 20, 2019 •

edited

Loading

CaptainDaVinci commented Mar 20, 2019 •

edited

Loading

CaptainDaVinci commented Mar 20, 2019 •

edited

Loading