Implemented title and camel key modifiers by oscargus · Pull Request #1894 · JabRef/jabref

oscargus · 2016-08-30T14:43:54Z

Implemented #1506

Change in CHANGELOG.md described
Tests created for changes
Check documentation status (Issue created for outdated help page at help.jabref.org?)

Siedlerchr · 2016-08-30T21:26:27Z

 ### Fixed
 - Fixed NullPointerException when opening search result window for an untitled database 
- Fixed #1757: Crash after saving illegal argument in entry editor
 - Fixed[#1663](https://github.com/JabRef/jabref/issues/1663): Better multi-monitor support


Space after fixed

Do you hear that @stefan-kolb ;-)

I think the missing space came from you. I'll change it though. ;-)

Siedlerchr · 2016-08-30T21:32:18Z

LGTM, just some misisng space in the changelog 👍

tobiasdiez · 2016-08-31T02:43:59Z

+    @Test
+    public void generateKeyTitleTitle() {
+        preferences = new BibtexKeyPatternPreferences("", "", false, true, false, pattern);
+        DatabaseBibtexKeyPattern bibtexKeyPattern = new DatabaseBibtexKeyPattern(pattern);


I think it makes sense to move preferences = ...; DatabaseBibtexKeyPattern bibtexKeyPattern = ... and metadata.setBibtexKeyPattern(bibtexKeyPattern); BibEntry entry = new BibEntry(); entry.setField("title",...)
to the setup method.

The preferences are different for different tests. There is a standard preferences in setUp, but in this case it is not the standard preferences. Adding set methods which are only used in the tests doesn't seem worthwhile to me.

Regarding entries, yes, one could have one in the preferences with some standard content, and edit as required in the tests.

(This also holds for your similar comment in the other PR, although I noted I used the standard preferences in some test and still redefined them...)

lenhard · 2016-09-05T15:56:45Z

        this.words.addAll(new TitleParser().parse(title));
    }

+    public List<Word> getWordsReadOnly() {


Why not immediately replace getWords with getWordsReadOnly, i.e. just have getWords return Collections.unmodifiableList(words); and skip the introduction of getWordsReadOnly? From what I see this should not break the existing implementations.

I think it will break it... See @tobiasdiez comment above. The thing is that forEach(Word::toUpperFirst) actually modifies the Title object, which it can't (I think) if Collections.unmodifiableList(words); is returned. On the other hand, I do not really see the use case of Title unless one is allowed to modify the content.

Ah, I get it. The real problem is that the Word objects are mutable, less that the list is.

However, that means that you can still return Collections.unmodifiableList(words) and execute forEach(Word::toUpperFirst). The unmodifiableList only ensures that the structure of the list will not change (and your lambda does not change it), the objects contained therein can still be modified (which is ugly, but anyway...). Therefore, I would ask you to implement my suggestion and skip the deprecated getter.

And please add at least a comment in Word that this class should be rewritten to be not mutable.

Makes sense. I'll add immutable getWords as default and add a comment. On the other hand it is quite convenient that Word mutable. Somehow the whole Title class depends on it as the toString() method really wouldn't make sense if you cannot change any Word in Title. You would just get the same string that you entered and there is no way to convert a stream to a new Title at the moment.

oscargus · 2016-09-14T14:27:43Z

Added lower_case, upper_case, sentence_case, capitalize, and title_case modifiers (the two first are just additional names).

Unmodifiable return from Title.getWords() but the function of the modifier rely heavily on mutable Word.

oscargus · 2016-09-16T08:16:08Z

 - Entries in the SearchResultDialog are now converted to Unicode
 - Suggestions in the autocomplete (search) are now in Unicode
 - Fixed NullPointerException when opening search result window for an untitled database 
- Fixed entry table traversal with Tab (no column traversal thus no double jump)


tobiasdiez

Just some small remarks about the code. Can be merged afterwards.

tobiasdiez · 2016-09-16T14:32:57Z

-                    resultingLabel = resultingLabel.toLowerCase(Locale.ENGLISH);
-                } else if ("upper".equals(modifier)) {
-                    resultingLabel = resultingLabel.toUpperCase(Locale.ENGLISH);
+                if ("lower".equals(modifier) || lowerCaseFormatter.getKey().equals(modifier)) {


Please refactor the code a bit: first convert the modifier to the corresponding formatter Formatter getFormatterForModifier(string modifier) and then apply it to the label.

tobiasdiez · 2016-09-16T14:34:20Z

+
+    @Test
+    public void generateKeyTitleCapitalize() {
+        DatabaseBibtexKeyPattern bibtexKeyPattern = new DatabaseBibtexKeyPattern(pattern);


The lines DatabaseBibtexKeyPattern bibtexKeyPattern = ... and metadata.setBibtexKeyPattern(bibtexKeyPattern); can be extracted to the setup method.

koppor · 2016-09-21T07:09:50Z

@oscargus Could you follow up so that we can merge?

# Conflicts: # CHANGELOG.md

tobiasdiez · 2016-09-23T14:56:57Z

@koppor / @oscargus pls also move metadata.setBibtexKeyPattern(bibtexKeyPattern); to the setup method

# Conflicts: # CHANGELOG.md

# Conflicts: # src/test/java/net/sf/jabref/logic/bibtexkeypattern/MakeLabelWithDatabaseTest.java

...and also all DatabaseBibtexKeyPattern bibtexKeyPattern = new DatabaseBibtexKeyPattern(pattern);

koppor · 2016-09-25T07:54:22Z

@tobiasdiez Sorry for overlooking your second move comment. I also created Formatters.getFormatterForModifier. Hope, this is good to go now.

The tests are locally OK. The failing test is unrelated to the update.

tobiasdiez · 2016-09-25T08:31:31Z

Yes, looks good now. Thanks @koppor for finishing this PR 😄

* 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

* Implemented title and camel key modifiers * Fixed comments * Added more modifiers * Move bibtexKeyPattern to constructor * Move metadata.setBibtexKeyPattern(bibtexKeyPattern); to @before ...and also all DatabaseBibtexKeyPattern bibtexKeyPattern = new DatabaseBibtexKeyPattern(pattern); * Introduce Formatters.getFormatterForModifier(String)

oscargus added [outdated] type: enhancement status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers component: citationkey-generator labels Aug 30, 2016

oscargus mentioned this pull request Aug 30, 2016

Update key generator description for #1894 JabRef/user-documentation#66

Closed

oscargus force-pushed the implement1506 branch 2 times, most recently from d5e5e64 to f0ae6ee Compare August 30, 2016 19:44

Siedlerchr reviewed Aug 30, 2016
View reviewed changes

tobiasdiez reviewed Aug 31, 2016
View reviewed changes

tobiasdiez removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Aug 31, 2016

oscargus force-pushed the implement1506 branch from f0ae6ee to 48c1e06 Compare August 31, 2016 17:56

oscargus added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Aug 31, 2016

lenhard reviewed Sep 5, 2016
View reviewed changes

tobiasdiez removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Sep 8, 2016

oscargus added 3 commits September 14, 2016 15:41

Implemented title and camel key modifiers

579f212

Fixed comments

aa489ac

Added more modifiers

5b05180

oscargus force-pushed the implement1506 branch from 48c1e06 to 5b05180 Compare September 14, 2016 14:25

oscargus added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Sep 16, 2016

oscargus commented Sep 16, 2016

View reviewed changes

tobiasdiez requested changes Sep 16, 2016

View reviewed changes

koppor removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Sep 21, 2016

koppor added 2 commits September 23, 2016 09:54

Merge branch 'master' into implement1506

3bdb5e4

# Conflicts: # CHANGELOG.md

Move bibtexKeyPattern to constructor

73871ae

Merge remote-tracking branch 'origin/master' into implement1506

8758687

# Conflicts: # CHANGELOG.md

koppor added 3 commits September 25, 2016 02:12

Merge branch 'master' into implement1506

22954b3

# Conflicts: # src/test/java/net/sf/jabref/logic/bibtexkeypattern/MakeLabelWithDatabaseTest.java

Move metadata.setBibtexKeyPattern(bibtexKeyPattern); to @before

2d595b9

...and also all DatabaseBibtexKeyPattern bibtexKeyPattern = new DatabaseBibtexKeyPattern(pattern);

Introduce Formatters.getFormatterForModifier(String)

45ab7ec

koppor force-pushed the implement1506 branch from 6da74b6 to 45ab7ec Compare September 25, 2016 07:37

tobiasdiez merged commit adf226f into master Sep 25, 2016

tobiasdiez deleted the implement1506 branch September 25, 2016 08:31

Siedlerchr mentioned this pull request Mar 12, 2017

title_case/capitalize modifiers do not work on all fields JabRef/jabref-koppor#237

Closed

2 tasks

matthiasgeiger mentioned this pull request Mar 5, 2019

Adding additional modifiers to be used in bibtex key generator #4722

Merged

6 tasks

Uh oh!

Conversation

oscargus commented Aug 30, 2016

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!

oscargus Aug 31, 2016 via email

Choose a reason for hiding this comment

Uh oh!

Siedlerchr commented Aug 30, 2016

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!

Choose a reason for hiding this comment

Uh oh!

oscargus commented Sep 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tobiasdiez left a comment

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!

koppor commented Sep 21, 2016

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

tobiasdiez commented Sep 25, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tobiasdiez commented Sep 23, 2016 •

edited

Loading

koppor commented Sep 25, 2016 •

edited

Loading