Make automatic groups a first-class citizien #2563

tobiasdiez · 2017-02-17T16:42:17Z

So after you guys had your fun in Lugano (and harvested a big number of changed lines), I want to have my share too. Thus here I'm with a new groups-related PR.

Previously there was the option to automatically generate a bunch of groups. For example based on the keywords contained in a certain field. However, this process was a one-time thing, which means that once the groups were created they were not keep in sync with changes to the entries.
In this PR, I introduce the automatic groups as a new type of groups that based on a certain criteria automatically create subgroups and keep them synchronized with the changes.

The current implementation has a few caveats which I would like to address in a new PR:

Automatic groups are not yet written to the bib file (and parsed).
The update process only happens if you switch between databases and the groups tree is reinitialized.
The UI in the groups edit dialog is probably not the greatest...

Remark: Our architecture tests threw a NPE instead of a meaningful message in case of an issue. This is also fixed.

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?

Siedlerchr · 2017-02-17T19:02:12Z

src/main/java/org/jabref/gui/groups/GroupDialog.java

+        } else {
+            if (editedGroup.getClass() == WordKeywordGroup.class) {
+                WordKeywordGroup group = (WordKeywordGroup) editedGroup;
+                nameField.setText(group.getName());


I would prefer a switch case here, but that is a minor thing

According to the Java language specifications

The type of the Expression must be char, byte, short, int, Character, Byte, Short, Integer, String, or an enum type, or a compile-time error occurs.

thus switch over a class type does not work.

Siedlerchr · 2017-02-17T19:11:17Z

src/main/java/org/jabref/gui/groups/GroupDialog.java

+                keywordGroupCaseSensitive.setSelected(group.isCaseSensitive());
+                keywordGroupRegExp.setSelected(false);
+                keywordsRadioButton.setSelected(true);
+                setContext(editedGroup.getHierarchicalContext());


Move set context out of the ifs as it is called always with the same parameter
Same for set Name

lenhard · 2017-02-21T17:59:40Z

You call a mere thousand changes "line harvesting"? Pathetic! ;-) Fun aside, please do not take this as a justification to blow up the number of LOC in your PR.

I have a more general comment: The handling of special fields is pretty much the same as you do here. Have a look at the (deprecated) class org.jabref.logic.specialfields.SpecialFieldUtils. This class synchronizes the keyword field with different other fields. You want the same here: To synchronize one field (groups) with some others (author, keywords). It would be really cool if we could extract this synchronization logic in a more abstract fashion, so that it is reused for the special fields and the automatic groups. Otherwise, we would have several competing implementations for the same thing in JabRef.

I haven't given much thought at how to unify these two features with the common logic, but maybe you could give it a try here? The special fields are much less ugly now than they used to be, promise!

tobiasdiez · 2017-02-22T19:57:54Z

I fear I cannot completely follow you @lenhard.

If I understand the special fields correctly, then they sync the content of some field (like rank = 1) with the keywords field (keywords = rank1).
The automatic groups take the contents of some field (e.g. keywords = foo, bar) and creates dynamic groups for each keyword (i.e. a group matching foo and one for bar). However, these groups are only created for the display and not written to groups tree in the bib file. Moreover, this features is completely independent of the groups field that is only used for the static groups.

Thus the only logic that seems to be shared by these features is the extraction of a list of keywords from a string and this is already encapsulated in the KeywordList class. What do I miss?

koppor · 2017-02-22T23:04:36Z

Side topic: @stfnklb and me discussed whether we should use jrkeywords as field. Reason: keywords are the keywords from the paper and users want to keep them. - By this, we separate paper data from meta data on bibliographical data... Am 22.02.2017 20:57 schrieb "Tobias Diez" <[email protected]>:

…

I fear I cannot completely follow you @lenhard <https://github.com/lenhard>. - If I understand the special fields correctly, then they sync the content of some field (like rank = 1) with the keywords field (keywords = rank1). - The automatic groups take the contents of some field (e.g. keywords = foo, bar) and creates dynamic groups for each keyword (i.e. a group matching foo and one for bar). However, these groups are only created for the display and not written to groups tree in the bib file. Moreover, this features is completely independent of the groups field that is only used for the static groups. Thus the only logic that seems to be shared by these features is the extraction of a list of keywords from a string and this is already encapsulated in the KeywordList class. What do I miss? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2563 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABTafpYet7gvjqVeXHYDg-xn92-46gbxks5rfJNDgaJpZM4MEfJ2> .

lenhard · 2017-02-23T08:58:22Z

@tobiasdiez From your answer, I think that you have understood me correctly.

It was just an idea and might not be worthwhile. Since the special fields synchronization already builds upon KeywordList, this might be as far as we can get. So, from my point of view you can proceed with this PR.

@koppor: Although what you say is true, I do not know of any user complaint about this behavior (which is telling). In contrast, changing the semantics of the keyword field in JabRef will confuse a lot of users, I would expect.

tobiasdiez · 2017-02-23T16:35:53Z

Ok, I merge now and will have a second look about the SpecialFields later.

* 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

tobiasdiez added 4 commits February 15, 2017 22:37

Add automatic groups

f90afdd

Add option for automatic groups in group dialog

5b52e62

Merge remote-tracking branch 'JabRef/master' into groupsAuto

ebb5d19

Fix build and language files

f1f0a3a

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

Fix architecture tests

1959991

Siedlerchr reviewed Feb 17, 2017

View reviewed changes

tobiasdiez added 2 commits February 23, 2017 12:14

Merge branch 'master' into groupsAuto

8fbeaa9

Update GroupNodeViewModel.java

34a3748

tobiasdiez merged commit 1973386 into master Feb 23, 2017

tobiasdiez deleted the groupsAuto branch February 23, 2017 16:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make automatic groups a first-class citizien #2563

Make automatic groups a first-class citizien #2563

tobiasdiez commented Feb 17, 2017 •

edited

Loading

Siedlerchr Feb 17, 2017

tobiasdiez Feb 23, 2017

Siedlerchr Feb 17, 2017

lenhard commented Feb 21, 2017

tobiasdiez commented Feb 22, 2017

koppor commented Feb 22, 2017 via email

lenhard commented Feb 23, 2017

tobiasdiez commented Feb 23, 2017

Make automatic groups a first-class citizien #2563

Make automatic groups a first-class citizien #2563

Conversation

tobiasdiez commented Feb 17, 2017 • edited Loading

Siedlerchr Feb 17, 2017

Choose a reason for hiding this comment

tobiasdiez Feb 23, 2017

Choose a reason for hiding this comment

Siedlerchr Feb 17, 2017

Choose a reason for hiding this comment

lenhard commented Feb 21, 2017

tobiasdiez commented Feb 22, 2017

koppor commented Feb 22, 2017 via email

lenhard commented Feb 23, 2017

tobiasdiez commented Feb 23, 2017

tobiasdiez commented Feb 17, 2017 •

edited

Loading