Validates the file path of a TexGroup and fixes Texgroup's "Library has been modified by another program"

[systemoperator]

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.

[calixtus]

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

[systemoperator]

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

[Siedlerchr]

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

[calixtus]

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

[calixtus]

Great! Looks good to me!

[systemoperator]

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]

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]

Now without stream.^^

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

[tobiasdiez]

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]

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]

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)

[calixtus]

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

[Siedlerchr]

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

[koppor]

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!

[systemoperator]

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.

[koppor]

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]

@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.