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

Implement #1904: filter groups #2588

Merged
merged 1 commit into from
Mar 2, 2017
Merged

Implement #1904: filter groups #2588

merged 1 commit into from
Mar 2, 2017

Conversation

tobiasdiez
Copy link
Member

Add textbox add the bottom of the groups panel which allows for filtering the groups by name. Implements #1904.

snipimage 002

  • Change in CHANGELOG.md described
  • Tests created for changes
  • Screenshots added (for bigger UI changes)
  • Manually tested changed features in running JabRef
  • Check documentation status (Issue created for outdated help page at help.jabref.org?)
  • If you changed the localization: Did you run gradle localizationUpdate?

@tobiasdiez tobiasdiez added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Feb 24, 2017
@Siedlerchr
Copy link
Member

Great idea. However, I would place the search bar on top of the screen and not at the bottom, or alternatively is there a way to reuse the existing search bar and add an option to search for groups?

@tobiasdiez
Copy link
Member Author

Maybe it's indeed a good idea to move the search bar and button to the top...as this is an easy change I would postpone it for the moment and see what kind of further user controls are necessary in the group pane.

@stefan-kolb
Copy link
Member

On the bottom looks good to me.

@lenhard
Copy link
Member

lenhard commented Feb 28, 2017

I think having the search box at the bottom is fine for now, since we can also change it easily.

The feature works nicely with normal-size bib files, but I just checked it with the large bib file and there the feature scales very badly. If I enter something into the search field, it freezes and nothing happens for one or two minutes until the rearrangement of the groups appears (as desired).

Do you see a chance to speed this up? Maybe some parallelism, using parallel() in streams, could help?

@lenhard
Copy link
Member

lenhard commented Mar 1, 2017

Looks like other people see a group-related performance degradation: #2561 (comment) So, this is most likely not specific to this PR and you could go for a merge.

However, we really need to follow up with a performance improvement PR.

@lenhard
Copy link
Member

lenhard commented Mar 2, 2017

Since, as indicated in #2561 (comment) the degraded performance for large bibs is expected at the moment and will be tackled separately, I think this is ready for merge (which I am doing right away).

@lenhard lenhard merged commit 48f5293 into master Mar 2, 2017
@tobiasdiez tobiasdiez deleted the groupAddFilter branch March 2, 2017 11:21
Siedlerchr added a commit that referenced this pull request Mar 2, 2017
* upstream/master: (21 commits)
  Implement #1904: filter groups (#2588)
  Braces checking followup (#2598)
  Improve braces checking (#2593)
  Fix pdf file import when only one file selected and no entry created (#2592)
  Fix test
  Imports
  Localization: MainFile: French: update (#2591)
  isPdf helper method
  Add asString with UTF-8 as standard encoding
  Fix names of groups in "add/move to group" dialog
  Make automatic groups a first-class citizien (#2563)
  Rename downloadToString method to asString
  Refactoring method names
  Inline Mime type detection
  Fix imports
  Extract SSL bypassing
  Bypass SSL functionality
  Unused methods and refactoring
  Always use browser user agent
  Monitored URLDownload is currently not use anywhere
  ...

# Conflicts:
#	CHANGELOG.md
@matthiasgeiger
Copy link
Member

One suggestion as I just stumbled over it: I think the searching should be case insensitive (although this might further slow down the searching ;-))

Siedlerchr added a commit that referenced this pull request Mar 7, 2017
* upstream/master: (91 commits)
  fixed #2613 (#2623)
  Add MathSciNet as ID-based fetcher (#2621)
  Add icon + color and description to groups (#2612)
  Fixed wrong logger import (#2618)
  Cleanup window has a scrollbar now. (#2614)
  Added the locale to a newly created class
  Move ExportComparator and CustomExportList to the correct package (only used in preferences)
  Fixes displaying of Mr DLib recommendations (#2616)
  Fix title-related key patterns in BibtexKeyPatternUtil (#2610)
  Remove OrdinalsToSuperscriptFormatter from recommended biblatex save actions (#2601)
  Update pgjdbc to new major version
  Update Dependenices String Similary log4j wiremock mockito
  Fix exception when parsing groups which contain a top level group (#2611)
  Add "Remove group and subgroups" option (#2587)
  Fix #1104: group selection is preserved under tab change
  Fix several File Cleanup + Rename issues  (#2415)
  Fixed a small error in the code comments
  Implement #1904: filter groups (#2588)
  Braces checking followup (#2598)
  Improve braces checking (#2593)
  ...
@AEgit
Copy link

AEgit commented May 18, 2017

Should the performance issue with the groups filtering (which is still massive) also be added to the 4.0 milestone?

@lenhard
Copy link
Member

lenhard commented May 19, 2017

@AEgit I am not exactly sure which issue you are referring to, but yes: We should try to improve performance before a final release. So if there is an open issue that should be in 4.0, please point me to it!

@AEgit
Copy link

AEgit commented May 19, 2017

@lenhard The performance issue I'm referring to happens with large databases (>10k entries, ~1k static groups) in conjunction with the groups filtering. When you try to find a group using the newly implemented groups search/filter, you have to wait for several minutes for the search to complete. During that time JabRef behaves as if it had crashed. You mentioned this problem yourself in this comment:
#2588 (comment)

Should I open a new ticket for this?

@lenhard
Copy link
Member

lenhard commented May 19, 2017

@AEgit Yes, please open a new ticket for this, so that we do not forget it.

@AEgit
Copy link

AEgit commented May 19, 2017

Done ;)
#2852

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants