-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Highlight groups that match any/all of the entries selected in the main table. #2515
Conversation
@@ -2179,6 +1886,10 @@ public boolean isUpdatedExternally() { | |||
return updatedExternally; | |||
} | |||
|
|||
public void setUpdatedExternally(boolean b) { | |||
updatedExternally = b; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use a more speaking variable name instead of b, maybe the same name as for the variable
* | ||
* Guarded by "task" | ||
*/ | ||
private TimerTask timerTask = new TimerTask() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better use a SheduledExecuterService:
http://stackoverflow.com/questions/409932/java-timer-vs-executorservice
@@ -2374,60 +2116,261 @@ public SearchAndOpenFile(final BibEntry entry, final BasePanel basePanel) { | |||
} | |||
} | |||
|
|||
private class GroupTreeListener { | |||
|
|||
private final Runnable task = new Runnable() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could use a lamdba here, too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some minor things.
I know that it is hard to build a group-related PR with fewer LOC changes, but I still don't like reviewing a PR of this size ;-) On a very superficial scroll-over level, the code looks ok. Instead, I played around a little with the UI. It took me a while to understand how the highlighting features was meant to work, but once I did, I could not find bugs. And I tried hard. Just one thought:
From my user's perspective, the PR is ready to be merged. |
I will merge this right now as it is. @lenhard The all entries hits number is no longer highlighted. Thanks for the suggestion. Do you have any ideas how to make the feature more self-explanatory? |
@tobiasdiez Not really. Once you start thinking about it, you end up with a more or less similar solution. One minor thing maybe: When clicking a group I was expecting that all entries in the group become selected (that is: blue). It took me a while to understand that they were sorted to the top and the unselected entries were greyed out. But if it has been that way and nobody really complains about it, it is propably best to leave it that way (in the abscense of a better idea). Also, I did the testing with our gigantic testing database from Aegit. Propably the highlighting is more intuitive if you use it with a database where you built the grouping yourself. |
There are two modes for groups:
|
To the |
I prefer to collect all the options in the preferences dialog. I think it is also not a setting that a user changes very often. What do you think? |
In general I don't agree: Aspects as "Union vs. Intersect" and "Inverted" are more things that should be directly changeable in the groups UI. |
Of course you are right that the other settings (union, intersect, inverted, etc) would make sense as a popup-menu (as they are currently implemented) or as a toggle button. But these other options are not very high on my priority list as I don't really understand why one needs them (maybe they are better implemented as search commands like |
Yeah you need them to accomplish "searches" like the ones you proposed (Find all papers, a) which are in both selected groups, b) which are in one or both of the selected groups, c) which are not in the selected group. Maybe we could implement this a bit different as it is now using toggle buttons as for the normal search: Toggle button for "invert" + toggle button for "union", which can only be selected if multiple groups are selected (Side note: This is currently not possible in the FX version 😉). And, as this is basically just a search that is performed, we could consider that the "search term" will be automatically appended at the search field of the search bar. This could either educate the user how to formulate such searches manually - or confuse him 😉 This also might have been some side effects we should think about (e.g., if a "global search" is already active. |
* upstream/master: Fix error when path is no valid directory (#2527) French localization: translation of a string French menu: localization Highlight groups that match any/all of the entries selected in the main table. (#2515) Fix % sign cleanup (#2521) Revert "Fix repeated escaping of % sign" (#2520) Fix repeated escaping of % sign (#2519) fix for #2482 deadlock on PDF import (#2517)
If a group contains all selected entries, then the "hit counter" turns green (and if only a few of the selected entries are contained in the group, then yellowish).
This replaces the "highlight groups" feature under the "Groups" menu.
(Best part: removes code from BasePanel and JabRefFrame)
I'm sorry that this PR contains a lot of modifications that are essentially only code rearrangements.
gradle localizationUpdate
?