Towards hierarchical keywords by tobiasdiez · Pull Request #1950 · JabRef/jabref

tobiasdiez · 2016-09-11T09:24:57Z

This PR is the first step towards supporting hierarchical keywords #628.

Refactor the keyword-related code to use new classes KeywordList and Keyword
Unified keyword delimiter to be a single character and in this way fixed the issues mentioned in Keyword separator #705 and Keyword separator behaviour (again, from a groups perspective) #1877
Move keyword delimiter from preferences to metadata (database properties) -> new PR
Add hierarchical delimiter and change parsing logic in KeywordList accordingly -> new PR
Change UI to support hierarchical keywords -> new PR

Note: I also moved the StringUtil class to model (in some sense it is JabRef's own String class) and removed EntryUtil (only had methods related to strings (-> StringUtil) or keywords (-> KeywordList) ).

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

lenhard

A few things need to be addressed before merging (especially StringUtil and the inheritance of KeywordList).

lenhard · 2016-09-16T07:38:48Z

    }

-    public BibDatabaseContext(Defaults defaults, DatabaseLocation location, String keywordSeparator) {
+    public BibDatabaseContext(Defaults defaults, DatabaseLocation location, Character keywordSeparator) {


Not that this is an error or anything the like. But there is hardly ever an advantage of using Character over String (except for illusive memory benefits). So what is the reason for this change and all the related changes in this PR?

There were some issues related to strings as keyword separators. For example, the default is , (with space) but some places tried to split strings only at the comma (i.e. recognize apple,orange). The space was only used to reformat the list as apple, orange. So I decided to force that only one character can be used as a separator and the final space is always adapted upon writing.

Ok, sounds reasonable. Let's hope that no one ever wants to use some weird multi-keyword separator.

lenhard · 2016-09-16T07:43:51Z

 import java.util.Collection;

-import net.sf.jabref.model.entry.EntryUtil;
+import net.sf.jabref.model.strings.StringUtil;


So if I get this correctly, you merged StringUtil into EntryUtil? We have had discussions about this before and I am not entirely happy with it. StringUtil has a high potential of becoming a god class that can do everything and is used everywhere. Please let's be careful with this.

In the end, however, I also see no point in arguing endlessly over it, so it is ok for me for now. However, please not that on current master there is the class model.util.ModelStringUtil. There is absolutely no change that I would tolerate two string util classes in model, so you'll have to merge that one as well

Yes we now have a ModelStringUtil class, which already moved some string-related methods to model. I would propose to merge everything together: logic.StringUtil, model.ModelStringUtil and model.EntryUtil, they all deal with the same thing. I will create a separate PR with this change (because it also makes merging easier).

Ok, I understand. Do this in a separate PR, but please do not forget it :)

lenhard · 2016-09-16T07:45:25Z

            }
        }
-        for (String s : sortedKeywordsOfAllEntriesBeforeUpdateByUser) {
+        for (Keyword s : sortedKeywordsOfAllEntriesBeforeUpdateByUser) {


Nitpick: Rename s to keyword :)

lenhard · 2016-09-16T07:46:13Z

+                        new ExplicitGroup(Localization.lang("Automatically created groups"),
+                                GroupHierarchyType.INCLUDING,
+                                Globals.prefs.getKeywordDelimiter()));
+                Set<String> hs;


Nitpick: Improve naming in the new code here.

lenhard · 2016-09-16T07:57:49Z

+ * Represents a list of keyword chains.
+ * For example, "Type > A, Type > B, Something else".
+ */
+public class KeywordList extends ArrayList<Keyword> {


I do not like that you extend the API class here. That way, KeywordList inherits all kinds of methods that it probably does not need and is prone to weird polymorphic misuses. Instead, KeyWordList should have an internal attribute of type ArrayList<Keyword> and just provide a limited subset of methods that are really needed to modify this attribute.

Also, the current implementation supports duplicates in the list. Is this reasonable for keywords? Wouldn't duplicate elimination make sense here?

Composition over inheritance, noted!

I wasn't sure how to handle duplicates. There was some code which tried to replace a keyword exactly at the same position: a, b, c -> a, replace, c. This was not easily possible with Set, but just one line with List. Not sure that this was the right decision through. What is your opinion?

Tough question, really. Personally, I cannot come up with a reason to have duplicate keywords, but users are weird, as we all know :) It is probably best if we just keep things as they were for the moment. Since the keywords were prior stored as a String, it was certainly possible to add duplicates. So let us keep this for now. My fear is that it may result in random bugs, where processing stops, because a keyword is found and another occurrence of the keyword is still in the list.

As a side note: You do not have to use a Set to avoid duplicates. You can also use a List internally and purge duplicates at the level of KeywordList.

I now changed it so that no duplicates are left upon parsing a string to a KeywordList. I feel we get more bugs if the list may actually contain duplicates.

lenhard · 2016-09-16T08:02:04Z

+import org.junit.Test;
+import org.xml.sax.SAXException;
+
+public class GvkParserTest {


I don't quite get why this class is introduced in this PR.

lenhard · 2016-09-16T08:03:35Z

+
+import static org.junit.Assert.assertEquals;
+
+public class KeywordListTest {


Please add tests that document the behavior in case of duplicate keywords.

* upstream/master: Implemented Integrity NoBibtexFieldChecker (#2059) Implemented title and camel key modifiers (#1894) Fix localization task hints (#2031) Result of syncLang.py update Result of syncLang.py update (with manual correction of captial_letters, and The_BibTeX_entry...) Fix "large capitals" to "capital letters" Updated Menu_tr.properties (#2057) Updated jabref_tr.properties (#2056) fix ignore version (#2055) Towards hierarchical keywords (#1950) Fix failing test, replace \uxxx encoded chars with proper UTF8 chars. Import Italian patch

* Small code cleanup in SpecialFieldsUtils * Refactor code related to keywords * Move StringUtil to model and remove EntryUtil * Add a few more tests * Change keyword delemiter in groups to Character * Optimize imports * Removed unused keyword separator in shareddb ui manager * Fix build errors * Fix failing architecture tests * Reformat imports * Small renamings * Move from Inheritance to composition in KeywordList * Fix tests * ArXiv accepts import format preferences instead of keyword delimiter * Fix binding * Fix arXiv tests

* Small code cleanup in SpecialFieldsUtils * Refactor code related to keywords * Move StringUtil to model and remove EntryUtil * Add a few more tests * Change keyword delemiter in groups to Character * Optimize imports * Removed unused keyword separator in shareddb ui manager * Fix build errors * Fix failing architecture tests * Reformat imports * Small renamings * Move from Inheritance to composition in KeywordList * Fix tests * ArXiv accepts import format preferences instead of keyword delimiter * Fix binding * Fix arXiv tests # Conflicts: # src/main/java/net/sf/jabref/gui/importer/fetcher/EntryFetchers.java

tobiasdiez added 6 commits September 10, 2016 12:43

Small code cleanup in SpecialFieldsUtils

a45a193

Refactor code related to keywords

cca2923

Move StringUtil to model and remove EntryUtil

6b372e1

Add a few more tests

585cfe6

Change keyword delemiter in groups to Character

a6d9e79

Optimize imports

f508b74

tobiasdiez added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers dev: code-quality Issues related to code or architecture decisions component: keywords labels Sep 11, 2016

tobiasdiez changed the title ~~Towards~~ Towards hierarchical keywords Sep 11, 2016

Removed unused keyword separator in shareddb ui manager

6664b1f

tobiasdiez closed this Sep 11, 2016

tobiasdiez reopened this Sep 11, 2016

koppor mentioned this pull request Sep 13, 2016

Keyword separator behaviour (again, from a groups perspective) #1877

Closed

tobiasdiez added this to the v3.7 milestone Sep 13, 2016

lenhard suggested changes Sep 16, 2016

View reviewed changes

tobiasdiez added 7 commits September 24, 2016 11:44

Merge

56b25e9

Fix build errors

a24d0ce

Fix failing architecture tests

8ab2826

Reformat imports

79f1add

Small renamings

754bf76

Move from Inheritance to composition in KeywordList

33cd26c

Fix tests

2665d83

tobiasdiez force-pushed the hierKeywords branch from 629d6ad to 2665d83 Compare September 24, 2016 11:20

tobiasdiez added 3 commits September 24, 2016 13:30

ArXiv accepts import format preferences instead of keyword delimiter

e7b4706

Fix binding

477eb47

Fix arXiv tests

4697cca

tobiasdiez merged commit 6ae86ae into JabRef:master Sep 24, 2016

tobiasdiez deleted the hierKeywords branch September 24, 2016 12:04

tobiasdiez mentioned this pull request Sep 24, 2016

Keyword separator #705

Closed

tobiasdiez mentioned this pull request Sep 24, 2016

Feature: Hierarchical Keywords #628

Closed

sauliusg mentioned this pull request Sep 20, 2019

Unexpected interaction of keyword and static groups #5331

Closed


		import static org.junit.Assert.assertEquals;

		public class KeywordListTest {

Uh oh!

Conversation

tobiasdiez commented Sep 11, 2016

Uh oh!

lenhard left a comment

Choose a reason for hiding this comment

Uh oh!

lenhard Sep 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lenhard Sep 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tobiasdiez Sep 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lenhard Sep 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lenhard Sep 16, 2016 •

edited

Loading

lenhard Sep 16, 2016 •

edited

Loading

tobiasdiez Sep 16, 2016 •

edited

Loading

lenhard Sep 16, 2016 •

edited

Loading