Improve search history by attaching change listener

[dkokkotas]

Fixes #9685

Essentially, we record the search query, only when the user completes the search process and "leaves" the search box. We ensure that, by checking whether the focus is lost or active on search bar and that helps us prevent storing redundant data (e.g prefixes)

What steps are followed?

1. User wants to search an entry
2. User types search query into search box --> focus is active on search box
3. User selects the desired entry from the search results --> user leaves search box --> focus
   changes to inactive / lost
4. Search query is recorded in search history

About the implementation

Resolving the issue, we attach a change listener to the search box node and call addSearchHistory method, only when:
- the focus property of search box is lost and
- the search query is not empty.
In case, the two aforementioned conditions are met, we record the query successfully.

Before that, the addSearchHistory method was called inside performSearch.
The performSearch was called every time the search box was idle for more than(SEARCH_DELAY = 400) ms, as a proper waiting time to perform the query and show search results, yet not proper for storing the search queries.
Consequently, there were unnecessary data (prefixes) recorded in history, as the user would not type the query that fast (<400ms/character typed). Therefore, we remove the call of addSearchHistory and place it under change listener's registration.

Change listener is NOT attached : Redundant prefixes are recorded

bug.mov

Change listener is attached : Recording query by the completion of search process

bug-fixed.mov

Note: In both cases I slowtype on purpose, so the search box could be idle for more than 400ms for every character typing.

Compulsory checks

Preview Give feedback

1. Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
2. Tests created for changes (if applicable)
3. Manually tested changed features in running JabRef (always required)
4. Screenshots added in PR description (for UI changes)
5. Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
6. 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.

[dkokkotas]

Commited modifications regarding the listener attachment.

1. Need to add some GUI tests corresponding to changes
2. About the implementation of string similarity and close timestamp concepts, could you provide feedback? Is it something I should work on and include. I have already stated my opinion on it's usefulness in the issue's comments.

[koppor]

You got partial feedback at #9685 (comment).

To judge the question, I need to check the current state. Maybe, you can summarize it? Not sure, when I have time to checkout your PR and check it. Not sure about the others though.

[dkokkotas]

Briefly, regarding the current state, the bug is fixed and there are no redundant recordings. I’m just wondering if we need the 2nd solution as an additional mechanism to optimize the search history.
The bug as introduced in issue's description, was the following (at my understanding). The addSearchHistory method call was embedded in performSearch, which was called every time the search box was idle for more than 400ms. That caused issues (e.g. prefixes were stored in case we slowtype). So, Houssem suggested two solutions regarding search history improvement.

First solution

Attach a change listener to focus property of search box node, so only when it gets inactive / lost (i.e. user completes search) then we record the query. Also removed the addSearchHistory method call from performSearch and placed it under listener's lambda expression. See pr's description for more details

Second solution

Each time a query is recorded, we loop the search history observable list and:

• Check StringSimilarity between the recently added and the previous ones, by calculating the edit distance, that needs to be within the preferred threshold.
• We also check if the time period between the two recordings is short enough.

In case both of them return the desired results, we merge the two queries

What I implemented?

I followed the first solution.
I will also upload some screenshots soon enough.

[Siedlerchr]

Totally fine if you don't need the similarity. I'm happy with the solution.
Try to make a test for it, if it's too complicated (gui tests are not easy, a manual test is also fine)

[koppor]

"Code" comments attached.

[koppor]

I tested it, but could not see any search history. What do I need to do to test this feature?

Is there any screenshot?

[dkokkotas]

@koppor I have just uploaded screenshots.
@Siedlerchr Will also add GUI tests so please don't merge.

[koppor]

Thank you for the video! -- I was expecting that there was a dropdown shown. Need to search the PR and check for the decision.

[koppor]

Improves the situation, so I would suggest to add a CHANGELOG entry and then we go for a merge. Tests can be done later.

[Siedlerchr]

From my side lgtm so far already

[Siedlerchr]

The last time I executed the code locally it worked in intellij. tests were passing.

[koppor]

Good to hear that it works on your macOS systems (you and @demetres12 use macOS). -- It does not work on my Windows and it also does not work on GitHub.

For GitHub, I already showed the links to the failing tests.

It is totally unclear to me why this happens.

@calixtus Can you run GlobalSearchBarTest.java on your Linux (and maybe Windows) machine?

On the one hand, it would be OK for me to disable the tests and to reserve it as "future work" to fix them. On the other hand, working with FXCollections in tests is very fundamental. Thus, we should devote some time to get this working on GitHub (Linux!) and on Windows machines.

[Siedlerchr]

change the Focus method to DefaultTaskExecutor.runAndWaitInJavaFXThread(() -> hBox.requestFocus());

that seemed to do the trick. Wating for the FX thread to execute

[Siedlerchr]

Yeah seems like it worked now 👍 💯

[dkokkotas]

Is there a reason the public access modifier for test methods is not used in several test classes?

[Siedlerchr]

Basically, this is a feature of junit5. Back in JUnit 4 they had to be public.

It is generally recommended to omit the public modifier for test classes, test methods, and lifecycle methods unless there is a technical reason for doing so – for example, when a test class is extended by a test class in another package. Another technical reason for making classes and methods public is to simplify testing on the module path when using the Java Module System.

https://junit.org/junit5/docs/snapshot/user-guide/

[koppor]

Thank you for the updates. I have three nitpicks on the testing. I commented - and also updated the code for myself to keep things going. If the tests are green, I will merge.

[Siedlerchr]

@koppor the assertFalse check was necessary to ensure the focus listener was working correctly. The assertEquals returned this weird collections type differences until this task executor waiting

[dkokkotas]

@koppor @Siedlerchr
Your expertise and enthusiasm were both appreciated during a time that can be challenging for my concerns!!!
Heart thanks and gratefulness