Automatically created groups with Field to group by as entrytype (#4539) by shubhamatlani · Pull Request #4555 · JabRef/jabref

shubhamatlani · 2018-12-27T13:44:51Z

Potential solution for #4539:

Made "entrytype" as a valid keyword for automatically created groups.
Added check in BibEntry to see if 'searchField' matches TYPE_HEADER - "entrytype"
Made change in WordKeywordGroup to collect entries for the entrytype groups
Screenshots reflecting the change attached -

All entries -

Automatically create groups. Field to group by is "entrytype" -

Result -

Change in CHANGELOG.md described
Tests created for changes
Manually tested changed features in running JabRef
Screenshots added in PR description (for bigger UI changes)
Ensured that the git commit message is a good one
Check documentation status (Issue created for outdated help page at help.jabref.org?)

Siedlerchr

Thank you very much for your contribution! In general lgtm, just a minor improvement

Siedlerchr · 2018-12-27T14:31:54Z

        if (onlySplitWordsAtSeparator) {
+            if (BibEntry.TYPE_HEADER.equals(searchField)) {
+                for (String searchWord : searchWords) {
+                    Optional<EntryType> entryType = EntryTypes.getType(entry.getType(), BibDatabaseMode.BIBLATEX);


I think you can move the EntryType.get... Before the loop

Then you could implement the for loop as stream with filter and in the end you can use map / and collectors.toSet

Thanks. I have a question - While fetching entryType, how do I decide which BibDatabaseMode to pass? BIBTEX or BIBLATEX? In both the committed files I have used BIBLATEX. Is that correct?

Siedlerchr

Biblatex is s superset of bibtex, has more and other fields the newer standard.
I am unsure if you need the actual mode,
There is a method get Bib database mode (sorry, I'm on mobile only)
Could you please test if that works also for custom entry types?

tobiasdiez

Thanks a lot for your contribution. The changes look good to me and I've only a minor suggestion concerning the case when the entry type is unknown.

tobiasdiez · 2018-12-27T19:54:16Z

        if (onlySplitWordsAtSeparator) {
+            if (BibEntry.TYPE_HEADER.equals(searchField)) {
+                Optional<EntryType> entryType = EntryTypes.getType(entry.getType(), BibDatabaseMode.BIBLATEX);
+                if (entryType.isPresent()) {


As above, entryType = EntryTypes.getType(entry.getType(), BibDatabaseMode.BIBLATEX).orElse(entry.getType()) is slightly better.

Type mismatch occurs. entry.getType() is String while we are assigning value for Optional<EntryType> here

tobiasdiez · 2018-12-27T19:57:16Z

+            if (BibEntry.TYPE_HEADER.equals(searchField)) {
+                Optional<EntryType> entryType = EntryTypes.getType(entry.getType(), BibDatabaseMode.BIBLATEX);
+                if (entryType.isPresent()) {
+                    return searchWords.stream().filter(sw -> entryType.get().getName().equals(sw)).collect(Collectors.toSet());


What's the reason for filtering the searchWords list? I think you can safely return the value of entryType (as a 1-item list).

searchWords is a 1-item set. Filter method lets me compare the content of this set with the entry type. Example, searchWord can be 'Book' but the entry type can be 'Article' and hence, this entry won't fall under Book subgroup. Other approach that I could think of -

if (entryType.isPresent() && searchWords.iterator().next().equals(entryType.get().getName())) {
return searchWords;
}

Please suggest.

Your solution sounds reasoable, I would let it as is.

Siedlerchr

From my point of view this looks good now!

shubhamatlani · 2019-01-09T12:31:30Z

Hi, is there anything else that needs to be done here before this PR can be merged with master ?

Siedlerchr · 2019-01-09T12:37:31Z

if @tobiasdiez is fine with the change, we can merge!

tobiasdiez

Sorry, that somehow disappeared from my radar.
The changes look good to me, so I'll merge now. Thanks again for your contribution.

* upstream/master: (21 commits) Bump applicationinsights-core from 2.3.0 to 2.3.1 (#4656) Bump applicationinsights-logging-log4j2 from 2.3.0 to 2.3.1 (#4657) Bump bcprov-jdk15on from 1.60 to 1.61 (#4653) Bump log4j-jcl from 2.11.1 to 2.11.2 (#4646) Bump log4j-api from 2.11.1 to 2.11.2 (#4650) Bump assertj-swing-junit from 3.8.0 to 3.9.2 (#4647) Bump log4j-jul from 2.11.1 to 2.11.2 (#4648) Bump log4j-slf4j18-impl from 2.11.1 to 2.11.2 (#4649) Bump log4j-core from 2.11.1 to 2.11.2 (#4651) Remove old code for PDF import (#4634) Reorder conditional checks Automatically created groups with Field to group by as entrytype (#4539) (#4555) Fix for issue 4641: Remove usage of TempDirectory extension from junit-pioneer (#4644) Edit localization file Add Integrity check for books with edition reported as 1 Use 'junit-jupiter' aggregator module (#4640) Bump junit-jupiter-params from 5.3.2 to 5.4.0 (#4638) Bump junit-vintage-engine from 5.3.2 to 5.4.0 (#4637) Bump junit-platform-launcher from 1.3.2 to 1.4.0 (#4636) Bump junit-jupiter-api from 5.3.2 to 5.4.0 (#4639) ... # Conflicts: # src/main/resources/l10n/JabRef_en.properties

…tofx * upstream/master: (30 commits) Bump applicationinsights-core from 2.3.0 to 2.3.1 (#4656) Bump applicationinsights-logging-log4j2 from 2.3.0 to 2.3.1 (#4657) Bump bcprov-jdk15on from 1.60 to 1.61 (#4653) Bump log4j-jcl from 2.11.1 to 2.11.2 (#4646) Bump log4j-api from 2.11.1 to 2.11.2 (#4650) Bump assertj-swing-junit from 3.8.0 to 3.9.2 (#4647) Bump log4j-jul from 2.11.1 to 2.11.2 (#4648) Bump log4j-slf4j18-impl from 2.11.1 to 2.11.2 (#4649) Bump log4j-core from 2.11.1 to 2.11.2 (#4651) Remove old code for PDF import (#4634) Reorder conditional checks Automatically created groups with Field to group by as entrytype (#4539) (#4555) Fix for issue 4641: Remove usage of TempDirectory extension from junit-pioneer (#4644) Edit localization file Add Integrity check for books with edition reported as 1 Use 'junit-jupiter' aggregator module (#4640) Bump junit-jupiter-params from 5.3.2 to 5.4.0 (#4638) Bump junit-vintage-engine from 5.3.2 to 5.4.0 (#4637) Bump junit-platform-launcher from 1.3.2 to 1.4.0 (#4636) Bump junit-jupiter-api from 5.3.2 to 5.4.0 (#4639) ... # Conflicts: # src/main/java/org/jabref/preferences/PreferencesService.java

Filter automatically created groups by entrytype (JabRef#4539)

2fa906b

Siedlerchr approved these changes Dec 27, 2018

View reviewed changes

Address review comments - replace for loop with stream (JabRef#4539)

4a8b3c8

Siedlerchr reviewed Dec 27, 2018

View reviewed changes

tobiasdiez requested changes Dec 27, 2018

View reviewed changes

return value in case of unknown entry type (JabRef#4539)

ae16446

Siedlerchr approved these changes Jan 4, 2019

View reviewed changes

tobiasdiez approved these changes Feb 9, 2019

View reviewed changes

Merge branch 'master' into fix-for-issue-4539

fdbfdb2

tobiasdiez merged commit 8e7b1f9 into JabRef:master Feb 9, 2019

Uh oh!

Conversation

shubhamatlani commented Dec 27, 2018

Uh oh!

Siedlerchr 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!

Choose a reason for hiding this comment

Uh oh!

Siedlerchr left a comment

Choose a reason for hiding this comment

Uh oh!

tobiasdiez left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Siedlerchr left a comment

Choose a reason for hiding this comment

Uh oh!

shubhamatlani commented Jan 9, 2019

Uh oh!

Siedlerchr commented Jan 9, 2019

Uh oh!

tobiasdiez left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants