Validates the file path of a TexGroup and fixes Texgroup's "Library has been modified by another program" by systemoperator · Pull Request #6586 · JabRef/jabref

systemoperator · 2020-06-04T21:51:35Z

Fixes #6420.
Fixes #6585.

Change in CHANGELOG.md described (if applicable)
Tests created for changes (if applicable)
Manually tested changed features in running JabRef (always required)
Screenshots added in PR description (for UI changes)
Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

…validationVisualizer

This reverts commit ad11243.

calixtus

Hi @systemoperator , thanks again for this PR and your work here.
I got some remarks about the codestyle and one suggestion with StringUtils.

…alidation

systemoperator · 2020-06-05T12:05:18Z

I don't know where to put import org.apache.logging.log4j.core.util.FileUtils;. Wherever I put it, checkstyle fails.

Siedlerchr · 2020-06-05T12:12:18Z

@systemoperator We have our own FileUtil class in model or logic, beware of the import.

calixtus · 2020-06-05T13:27:02Z

Checkstyle is failing because of now unused import io.file.

calixtus

Great! Looks good to me!

systemoperator · 2020-06-05T14:09:58Z

Apparently, this PR also fixes #6394. At least I could not reproduce it so far. I will test it onwards.

tobiasdiez

Thanks for your PR!

Just to make sure that I understand the changes correctly: the issue is fixed by parsing the groups last?

I have to minor comments concerning the code. It would be nice if you could also add a test so that we don't break it in the future again (especially since this concerns the parser of bib files, which is arguely the most important part of JabRef).

systemoperator · 2020-06-05T16:41:59Z

Just to make sure that I understand the changes correctly: the issue is fixed by parsing the groups last?

Yes, because the groups access data which need to be defined beforehand. Depending on the sequence in the entry set, this may work once but not every time.

systemoperator · 2020-06-05T17:56:17Z

Now without stream.^^

Any suggestion how to test the case mentioned at #6586 (review)?

tobiasdiez · 2020-06-05T18:58:38Z

The code looks good to me! For the test, it should be similar to the following:

jabref/src/test/java/org/jabref/logic/importer/fileformat/BibtexParserTest.java

Lines 1369 to 1389 in 7cc5747

    
           @Test 
        
           void integrationTestGroupTree() throws IOException, ParseException { 
        
               ParserResult result = parser.parse(new StringReader("@comment{jabref-meta: groupsversion:3;}" + OS.NEWLINE 
        
                       + "@comment{jabref-meta: groupstree:" + OS.NEWLINE + "0 AllEntriesGroup:;" + OS.NEWLINE 
        
                       + "1 KeywordGroup:Fréchet\\;0\\;keywords\\;FrechetSpace\\;0\\;1\\;;" + OS.NEWLINE 
        
                       + "1 KeywordGroup:Invariant theory\\;0\\;keywords\\;GIT\\;0\\;0\\;;" + OS.NEWLINE 
        
                       + "1 ExplicitGroup:TestGroup\\;0\\;Key1\\;Key2\\;;" + "}")); 
        
               GroupTreeNode root = result.getMetaData().getGroups().get(); 
        
               assertEquals(new AllEntriesGroup("All entries"), root.getGroup()); 
        
               assertEquals(3, root.getNumberOfChildren()); 
        
               assertEquals( 
        
                       new RegexKeywordGroup("Fréchet", GroupHierarchyType.INDEPENDENT, StandardField.KEYWORDS, "FrechetSpace", false), 
        
                       root.getChildren().get(0).getGroup()); 
        
               assertEquals( 
        
                       new WordKeywordGroup("Invariant theory", GroupHierarchyType.INDEPENDENT, StandardField.KEYWORDS, "GIT", false, ',', false), 
        
                       root.getChildren().get(1).getGroup()); 
        
               assertEquals(Arrays.asList("Key1", "Key2"), 
        
                       ((ExplicitGroup) root.getChildren().get(2).getGroup()).getLegacyEntryKeys()); 
        
           }

Create the string by hand (with groups before the other setting that makes troubles) and then verify that the groups in the parsed metadata has the valid data.

systemoperator · 2020-06-05T21:16:02Z

The test was a bit tricky (access real file, relative, user name, hostname, ...), but it works now.

koppor

Code looks good; did not test it though.

Have some code nitpicks. Hope, you like these suggestions (moving the code to more modern Java 14).

koppor · 2020-06-16T14:50:34Z

-                default:
-                    // Keep meta data items that we do not know in the file
-                    metaData.putUnknownMetaDataItem(entry.getKey(), value);
+            } else if (entry.getKey().equals(MetaData.SAVE_ACTIONS)) {


Can we keep the switch (entry.getKey) somehow? Maybe nested in an else branch?

You can use the new Java14 switch: https://openjdk.java.net/jeps/361

(File needs to be added to

jabref/config/checkstyle/checkstyle.xml

Line 15 in ca06b71

<property name="fileNamePattern" value="AuthorAndsReplacer.java|Ordinal.java|EntryTypeView.java" />

then)

This is still a huge p.i.t.a.
Checkstyle really needs to keep up, people start getting tired of these workarounds.
Some background information, if you are interested: checkstyle/checkstyle#6615

The key problem is here that switch cases are not executed in order. That's the reason for the if else

In the concrete case here, it is always checked for entry.getKey(). Please check the diff:

The wrong order was at that diff - completely unrelated to my proposal!

Yes, the switch statement could be reintroduced (and simplified). Before this fix, the code was structured really confusing and looked error-prone. So I decided to create this consistent and simplified version, which I find quite acceptable.

I meant only for the lower part; not with the xxxx continue statements. Just for the endless chain of else if (entry.getKey().equeals(MetaData.X)) chains, where only X is different from the checks.

There is ALWAYS entry.getKey() compared from line 75 to line 92 (or did I see something wrong?).

This is just a matter of taste. Maybe, I'll just do it and open another PR to have the discussions there. Should not be a show stopper for us now.

koppor · 2020-06-17T19:33:11Z

+                input -> {
+                    if (StringUtil.isBlank(input)) {
+                        return false;
+                    } else {


@stefan-kolb Would review here: "Fail fast". Not endless nested if/else statements, but just exit early.

Therefore, I proposed:

if (StringUtil.isBlank(input)) { return false; }

Think, I will start another PR with this and the other code style thing and we can have the discussion there --> we need to get this fix in soon.

* upstream/master: Validates the file path of a TexGroup and fixes Texgroup's "Library has been modified by another program" (#6586) Bump postgresql from 42.2.12 to 42.2.14 (#6610) Add markdown-link-check (#6542) Catch InaccessibleObjectException (#6519) Fix author formatter for unchanged names (#6552) Bump com.simonharrer.modernizer from 1.8.0-1 to 2.1.0-1 Bump org.beryx.jlink from 2.19.0 to 2.20.0 Bump classgraph from 4.8.83 to 4.8.86 Update FileUtilTest.java Update FileUtilTest.java Squashed 'src/main/resources/csl-styles/' changes from c5f14e2..716f635 Update FileUtilTest.java Update MoveFilesCleanupTest.java checkstyle Fix dowmloaded files moved to citaiton key dir

systemoperator added 9 commits June 4, 2020 13:47

process GROUPSTREE and GROUPSTREE_LEGACY at the very end

23557f9

changelog

2eac649

refactoring (part 1)

01c36a3

refactoring (part 2)

44b8896

validator for aux file added, checking for empty field and using the …

f36ece6

…validationVisualizer

changelog

f554c95

extension

53cd9ad

formatting file according to JabRef's formatter for Eclipse

ad11243

Revert "formatting file according to JabRef's formatter for Eclipse"

d20388f

This reverts commit ad11243.

calixtus requested changes Jun 5, 2020

View reviewed changes

more checks added + checkstyle improved

8b7063d

calixtus requested changes Jun 5, 2020

View reviewed changes

Comment thread src/main/java/org/jabref/gui/groups/GroupDialogViewModel.java Outdated

Comment thread src/main/java/org/jabref/gui/groups/GroupDialogViewModel.java Outdated

calixtus reviewed Jun 5, 2020

View reviewed changes

Comment thread src/main/java/org/jabref/gui/groups/GroupDialogViewModel.java Outdated

systemoperator added 6 commits June 5, 2020 13:15

refactoring

7b65150

Merge branch 'systemoperator-texgroup' into systemoperator-texgroup-v…

5b7d80e

…alidation

update

fe895cf

refactoring

db60939

refactoring

f593b13

l10n

586d55b

systemoperator added 2 commits June 5, 2020 14:25

update

6a82156

refactoring

f092108

calixtus mentioned this pull request Jun 5, 2020

Fixes Texgroup's "Library has been modified by another program" #6584

Closed

5 tasks

cleanup (checkstyle)

74f05ea

systemoperator changed the title ~~Validates the file path of a TexGroup~~ Validates the file path of a TexGroup and fixes Texgroup's "Library has been modified by another program" Jun 5, 2020

calixtus approved these changes Jun 5, 2020

View reviewed changes

Siedlerchr requested a review from tobiasdiez June 5, 2020 14:03

Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jun 5, 2020

tobiasdiez requested changes Jun 5, 2020

View reviewed changes

Comment thread src/main/java/org/jabref/logic/importer/util/MetaDataParser.java Outdated

Comment thread src/main/java/org/jabref/logic/importer/util/MetaDataParser.java Outdated

tobiasdiez added the status: changes-required Pull requests that are not yet complete label Jun 5, 2020

systemoperator added 2 commits June 5, 2020 18:56

refactor

ba367ad

refactor to sorting mechanism

c120c45

calixtus reviewed Jun 5, 2020

View reviewed changes

Comment thread src/main/java/org/jabref/logic/importer/util/MetaDataParser.java Outdated

refactor (without stream)

ec8933e

systemoperator added 3 commits June 5, 2020 21:26

remove brackets

01cdb44

test created

e7a3c3f

finalize test

49c3a2b

systemoperator added 3 commits June 5, 2020 23:21

formatting

26342ab

formatting

aac9ffc

checkstyle

f98466a

systemoperator requested a review from tobiasdiez June 13, 2020 08:36

koppor requested changes Jun 16, 2020

View reviewed changes

koppor removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jun 16, 2020

tobiasdiez approved these changes Jun 17, 2020

View reviewed changes

systemoperator added 2 commits June 17, 2020 16:05

refactoring

3f2f46f

Merge branch 'master' into systemoperator-texgroup-validation

5a3548a

koppor approved these changes Jun 17, 2020

View reviewed changes

koppor merged commit ef0a6bb into JabRef:master Jun 17, 2020

systemoperator mentioned this pull request Jun 18, 2020

Main table: Coloured group indicator for cited entries does not show up consistently anymore #6394

Closed

Uh oh!

Conversation

systemoperator commented Jun 4, 2020 • edited by koppor Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

calixtus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

systemoperator commented Jun 5, 2020

Uh oh!

Siedlerchr commented Jun 5, 2020

Uh oh!

calixtus commented Jun 5, 2020

Uh oh!

calixtus left a comment

Choose a reason for hiding this comment

Uh oh!

systemoperator commented Jun 5, 2020

Uh oh!

tobiasdiez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

systemoperator commented Jun 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

systemoperator commented Jun 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tobiasdiez commented Jun 5, 2020

Uh oh!

systemoperator commented Jun 5, 2020

Uh oh!

koppor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

koppor Jun 16, 2020

Choose a reason for hiding this comment

Uh oh!

calixtus Jun 16, 2020

Choose a reason for hiding this comment

Uh oh!

Siedlerchr Jun 16, 2020

Choose a reason for hiding this comment

Uh oh!

koppor Jun 16, 2020

Choose a reason for hiding this comment

Uh oh!

systemoperator Jun 17, 2020

Choose a reason for hiding this comment

Uh oh!

koppor Jun 17, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

koppor Jun 17, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

systemoperator commented Jun 4, 2020 •

edited by koppor

Loading

systemoperator commented Jun 5, 2020 •

edited

Loading

systemoperator commented Jun 5, 2020 •

edited

Loading