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

Adds search history #9659

Closed
wants to merge 7 commits into from

Conversation

paulinegarelli
Copy link

@paulinegarelli paulinegarelli commented Mar 8, 2023

A search history has been added, which consists of:

  • a list containing at most 10 searches with their date of search
  • a button to the right of the search bar to display it
  • possibility to remove a search from the history, or to reuse it by double-clicking on it in the history

This fixes issue #7906

We know that another PR has been made for this issue, but since we also worked on this issue in parallel, we offer our solution here.

image

  • 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.

@calixtus
Copy link
Member

calixtus commented Mar 11, 2023

Hi, thanks for your contribution and your interest in JabRef, especially thanks for implementing the test.
Something that could be improved would be the SearchHistory class to inherit from the ObservableList<> class and to override the methods you are wrapping within addSearch and remove. Because if it feels like a list, it works like a list and it looks like a list, then it should be a list, or a deque (hint!) or whatever!

About merging:
To be honest, although codewise everything already looks quite good to me on first glance, I strongly believe that a whole dialog for a search history of ten entries with the date and time of the search is way too much bloat and hurts the user experience badly. I'm trying really hard to imagine a user story where the date and time of one of the entries of the last ten lookups in your local database could have any real use for a user, especially if the search history is not stored within your database or in the preferences.

JabRef is not a webbrowser and the search is only for a lookup in your local database. Maybe a simple popup below the search field is a bit more appropriate, like @Siedlerchr suggested in the issue description

Also I would suggest to connect the search history as an AutoCompleter to the search field and add an option to clear the whole search history (if its more than 10 entries) either in the preferences or in the right click menu.

I'm a bit surprised that you and @MaryJml did not work together to create a common PR (see #9647), this way the whole work of one of you will be thrown away. Open source is all about collaboration.

@koppor
Copy link
Member

koppor commented Mar 13, 2023

We discussed this in our dev call. We thank you for your contribution. Although the code looked very good, we decided to move forward with #9647.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants