Highlight groups that match any/all of the entries selected in the main table. #2515

tobiasdiez · 2017-02-03T15:28:07Z

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.

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?

…in table.

Siedlerchr · 2017-02-03T16:10:16Z

src/main/java/net/sf/jabref/gui/BasePanel.java

@@ -2179,6 +1886,10 @@ public boolean isUpdatedExternally() {
        return updatedExternally;
    }

+    public void setUpdatedExternally(boolean b) {
+        updatedExternally = b;


Please use a more speaking variable name instead of b, maybe the same name as for the variable

Siedlerchr · 2017-02-03T16:13:18Z

src/main/java/net/sf/jabref/gui/BasePanel.java

+         *
+         * Guarded by "task"
+         */
+        private TimerTask timerTask = new TimerTask() {


Better use a SheduledExecuterService:
http://stackoverflow.com/questions/409932/java-timer-vs-executorservice

Siedlerchr · 2017-02-03T16:16:23Z

src/main/java/net/sf/jabref/gui/BasePanel.java

@@ -2374,60 +2116,261 @@ public SearchAndOpenFile(final BibEntry entry, final BasePanel basePanel) {
        }
    }

+    private class GroupTreeListener {
+
+        private final Runnable task = new Runnable() {


You could use a lamdba here, too

Siedlerchr

Just some minor things.

…Edit

lenhard · 2017-02-05T15:06:39Z

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:

All entries seems to be always green, even if no entries are selected. Should it not be gray in this case? This is a sematics question, so just implement it in the same way as it has been before.

From my user's perspective, the PR is ready to be merged.

tobiasdiez · 2017-02-06T15:50:11Z

I will merge this right now as it is.
@Siedlerchr your comments concerned code that was just moved and not really modified (and I don't really want to touch the code in BasePanel and MainFrame since these components glue everything together but there are absolutely no tests in place...). But, of course, thank you for the review!

@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?

lenhard · 2017-02-06T19:29:51Z

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

tobiasdiez · 2017-02-06T20:03:12Z

There are two modes for groups:

Gray-out entries that are not matched
Filter, i.e. hide non-matched entries
I didn't changed anything at this behaviour.
One of the next PRs will move the option to switch between these modes to the preferences.

matthiasgeiger · 2017-02-06T20:33:54Z

One of the next PRs will move the option to switch between these modes to the preferences.

To the Options -> Preferences preferences? Or do you want to recreate the "old" groups settings menu?

tobiasdiez · 2017-02-06T20:36:55Z

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?

matthiasgeiger · 2017-02-06T20:42:27Z

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.
Switching between "hide" and "float" is more an edge case - as I cannot come up with an use case which requires switching the style - this is just a personal preference.

tobiasdiez · 2017-02-06T23:31:46Z

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 not in mygroup and in groupa and groupb)

matthiasgeiger · 2017-02-07T07:44:41Z

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.
Of course these are mere "shortcuts" for normal searches, but I'm not sure whether an ordinary JabRef user without some basic knowledge of formal logic can formulate this that easy.

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)

Highlight groups that match any/all of the entries selected in the ma…

791d1fa

…in table.

tobiasdiez added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Feb 3, 2017

Siedlerchr reviewed Feb 3, 2017

View reviewed changes

Siedlerchr approved these changes Feb 3, 2017

View reviewed changes

tobiasdiez added 2 commits February 5, 2017 15:01

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

8d5df03

…Edit

Update language

e99e9c9

Don't highlight all entries group

1c0c17a

tobiasdiez merged commit 2e9e7c8 into master Feb 6, 2017

tobiasdiez deleted the groupsEdit branch February 6, 2017 15:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Highlight groups that match any/all of the entries selected in the main table. #2515

Highlight groups that match any/all of the entries selected in the main table. #2515

tobiasdiez commented Feb 3, 2017 •

edited

Loading

Siedlerchr Feb 3, 2017

Siedlerchr Feb 3, 2017

Siedlerchr Feb 3, 2017

Siedlerchr left a comment

lenhard commented Feb 5, 2017

tobiasdiez commented Feb 6, 2017 •

edited

Loading

lenhard commented Feb 6, 2017

tobiasdiez commented Feb 6, 2017

matthiasgeiger commented Feb 6, 2017

tobiasdiez commented Feb 6, 2017

matthiasgeiger commented Feb 6, 2017

tobiasdiez commented Feb 6, 2017

matthiasgeiger commented Feb 7, 2017

Highlight groups that match any/all of the entries selected in the main table. #2515

Highlight groups that match any/all of the entries selected in the main table. #2515

Conversation

tobiasdiez commented Feb 3, 2017 • edited Loading

Siedlerchr Feb 3, 2017

Choose a reason for hiding this comment

Siedlerchr Feb 3, 2017

Choose a reason for hiding this comment

Siedlerchr Feb 3, 2017

Choose a reason for hiding this comment

Siedlerchr left a comment

Choose a reason for hiding this comment

lenhard commented Feb 5, 2017

tobiasdiez commented Feb 6, 2017 • edited Loading

lenhard commented Feb 6, 2017

tobiasdiez commented Feb 6, 2017

matthiasgeiger commented Feb 6, 2017

tobiasdiez commented Feb 6, 2017

matthiasgeiger commented Feb 6, 2017

tobiasdiez commented Feb 6, 2017

matthiasgeiger commented Feb 7, 2017

tobiasdiez commented Feb 3, 2017 •

edited

Loading

tobiasdiez commented Feb 6, 2017 •

edited

Loading