[WIP] Proper copying of entries #1677

oscargus · 2016-08-04T18:57:02Z

I'm making a method to properly copy entries from one database to another, including crossref entries and used strings. This is useful in generating a database from an aux file and for generating a database based on an OO/LO document. It could also be useful for generating a subdatabase from selected entries (or copying relevant strings when copying entries).

There are some problems though related to the preamble. In OO/LO the entries may come from several databases and there it is not obvious how to deal with the preamble. However, a possibly solvable problem is to detect which strings are used in the preamble.

In AuxParserTest @stefan-kolb have generated a test file with a string in the preamble. JabRef doesn't parse this in a way such that the string can be resolved automatically. Earlier, the preamble and all strings were copied in AuxParser, but I was hoping to only copy the relevant strings. Any input on how the preamble issue should be solved? One way would simply be to parse the preamble for strings, I guess. Any other suggestions?

Change in CHANGELOG.md described
Tests created for changes
Screenshots added (for bigger UI changes)
Manually tested changed features in running JabRef

koppor · 2016-08-10T18:13:55Z

Is it possible to also copy the files if the absolute directory of the file differs? The relative path of the source should be resolved against the file path of the current bib file. Including directory creation.

oscargus · 2016-08-11T10:38:54Z

That is indeed another thing which I haven't considered. I was primarily thinking of generating a bib without files (for OO/LO and AUX), but clearly, for proper copying files should be included. Even better if everything can be added to a Zip archive as well.

koppor · 2016-09-13T12:15:06Z

@oscargus If the only thing is that the files aren't copied, this should be ready for review, isn't it?

oscargus · 2016-09-13T17:22:29Z

Maybe it is OK to add. It is not really what I aimed for (yet), but the string copying works for AuxParser (I think), so always something.

koppor · 2016-09-14T07:26:54Z

Could you point to the concrete file "with a string in the preamble". Are these comment strings? I think, you reference #900?

oscargus · 2016-09-14T14:16:19Z

https://github.com/JabRef/jabref/blob/master/src/test/resources/net/sf/jabref/logic/auxparser/config.bib

koppor · 2016-09-15T09:46:40Z

@lenhard You recently worked on reading and writing bib databases. Can you comment on parsing @preamble. As far as I understood @oscargus, he wants to know which strings are used at @preamble and thus only copy these fields.

I don't know @preamble. Any pointers to documentation? 😇

@oscargus: Leaving @preamble and file copying aside, this should be ready for review, isn't it? I would like to get this merged to minimize the LOC for PRs. This enables us to quickly review PRs in our spare time. Longer PRs cannot be quickly reviewed.

lenhard · 2016-09-15T11:30:31Z

The preamble is parsed as-is with formatting and everything and set as a String member of BibDataBase. I am unaware of any formatting or syntax restrictions within the preamble.

Honestly, I do not really get what it should be good for, it seems like a relic. Apparently it is part of Bibtex, and, therefore, we parse it, have an editor for it and serialize it. It seems that it is more relevant for programs that compile the bib file. Here are some, not very conclusive references on the preamble: first, second, third.

If you want to decide, which strings are used in a preamble, it would be best to parse the preamble string and look for occurrences of #. Best to keep this simple. If we miss edge cases of preamble usage, it is probably irrelevant, as hardly anybody uses it.

koppor · 2016-09-15T12:19:42Z

src/test/java/net/sf/jabref/model/database/BibDatabaseTest.java

+        database.insertEntry(entry);
+        List<BibtexString> usedStrings = (List<BibtexString>) database.getUsedStrings(Arrays.asList(entry));
+        assertEquals(2, usedStrings.size());
+        assertTrue((string.getName() == usedStrings.get(0).getName())


Can't we put all strings in a set and just compare two sets?

That might indeed be a better solution. The problem at the moment is really that I do not know which order they are added in (or rather, I want the test to be independent of that).

Yeah, therefore I asked for a Set. The Set is independent of the order - or did I get your comment wrong?

oscargus · 2016-09-16T07:14:46Z

@koppor yes, the introduced functionality works, although it is not enough to obtain the complete purpose, so it can be reviewed and merged for now.

Personally, I do not really see the need for strings in the preamble, but since we have a test for that it felt wrong to break/remove that test just because I couldn't bother to handle it...

koppor

Some minor comments, then it is good to go.

koppor · 2016-09-22T05:45:05Z

src/main/java/net/sf/jabref/model/database/BibDatabase.java

@@ -400,7 +426,7 @@ public BibEntry resolveForStrings(BibEntry entry, boolean inPlace) {
     * care not to follow a circular reference pattern.
     * If the string is undefined, returns null.
     */
-    private String resolveString(String label, Set<String> usedIds) {
+    private String resolveString(String label, Set<String> usedIds, Set<String> allUsedIds) {


Please add Objects.requireNonNull(label);, Objects.requireNonNull(usedIds);, Objects.requireNonNull(allUsedIds);

Please add @paramter allUsedIds containing a short description of the parameter

koppor · 2016-09-22T05:45:20Z

src/main/java/net/sf/jabref/model/database/BibDatabase.java

@@ -342,7 +342,33 @@ public synchronized boolean hasStringLabel(String label) {
     */
    public String resolveForStrings(String content) {
        Objects.requireNonNull(content, "Content for resolveForStrings must not be null.");
-        return resolveContent(content, new HashSet<>());
+        return resolveContent(content, new HashSet<>(), null);


Please don't pass null. Pass an empty set.

koppor · 2016-09-22T05:45:54Z

src/test/java/net/sf/jabref/model/database/BibDatabaseTest.java

+        database.insertEntry(entry);
+        List<BibtexString> usedStrings = (List<BibtexString>) database.getUsedStrings(Arrays.asList(entry));
+        assertEquals(2, usedStrings.size());
+        assertTrue((string.getName() == usedStrings.get(0).getName())


Yeah, therefore I asked for a Set. The Set is independent of the order - or did I get your comment wrong?

koppor · 2016-09-22T05:46:15Z

src/main/java/net/sf/jabref/logic/auxparser/AuxParserResult.java

@@ -42,6 +45,10 @@ public int getUnresolvedKeysCount() {
        return unresolvedKeys.size();
    }

+    public int getInsertedStringsCount() {
+        return insertedStrings;


Is it possible to add a test case covering that?

tobiasdiez

Looks good in general.

tobiasdiez · 2016-09-22T09:18:28Z

src/main/java/net/sf/jabref/model/database/BibDatabase.java

@@ -400,7 +426,7 @@ public BibEntry resolveForStrings(BibEntry entry, boolean inPlace) {
     * care not to follow a circular reference pattern.
     * If the string is undefined, returns null.
     */
-    private String resolveString(String label, Set<String> usedIds) {
+    private String resolveString(String label, Set<String> usedIds, Set<String> allUsedIds) {


What is the difference between usedIds and allUsedIds? They seem to have the same purpose.

tobiasdiez · 2016-09-22T09:21:11Z

src/main/java/net/sf/jabref/model/database/BibDatabase.java

+        }
+
+        for (String stringId : allUsedIds) {
+            result.add((BibtexString) bibtexStrings.get(stringId).clone());


why do you use clone here?

tobiasdiez · 2016-09-22T09:23:14Z

src/main/java/net/sf/jabref/model/database/BibDatabase.java

@@ -413,11 +439,14 @@ private String resolveString(String label, Set<String> usedIds) {
                }
                // If not, log this string's ID now.
                usedIds.add(string.getId());


Here you get the id of the string and above in getUsedStrings you get the string from the id.
Thus I would propose to change Set<String> usedIds to Set<BibString> usedStrings.

tobiasdiez · 2016-09-22T09:26:26Z

src/test/java/net/sf/jabref/model/database/BibDatabaseTest.java

+    public void getUsedStringsSingleStringWithString() {
+        BibEntry entry = new BibEntry(IdGenerator.next());
+        entry.setField("author", "#AAA#");
+        BibtexString string = new BibtexString(IdGenerator.next(), "AAA", "Some other #BBB#");


Please move the string initalization and adding them to the db to the @setup method.

tobiasdiez · 2016-09-22T09:28:00Z

src/test/java/net/sf/jabref/model/database/BibDatabaseTest.java

+        database.addString(string2);
+        database.addString(string3);
+        database.insertEntry(entry);
+        List<BibtexString> usedStrings = (List<BibtexString>) database.getUsedStrings(Arrays.asList(entry));


create overload of getUsedStrings for one entry and use Collections.singletonList.

lenhard · 2017-02-15T10:50:52Z

As of 1684cdc this is integrated into master now. I had to do it on a separate branch since I have no commit access to the branch here. However, all commits are preserved.

I addressed the issues mentioned above that I agreed with, which were all rather minor quality things.

So this branch can be closed now.

First steps

f1838e7

oscargus added the type: enhancement label Aug 4, 2016

oscargus mentioned this pull request Aug 12, 2016

Copy "complete" BibEntry to a new database koppor/jabref#144

Open

koppor mentioned this pull request Aug 15, 2016

Resolve crossrefs and strings in main table #1644

Merged

2 tasks

koppor reviewed Sep 15, 2016

View reviewed changes

koppor requested changes Sep 22, 2016

View reviewed changes

tobiasdiez requested changes Sep 22, 2016

View reviewed changes

lenhard merged commit f1838e7 into JabRef:master Feb 15, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Proper copying of entries #1677

[WIP] Proper copying of entries #1677

oscargus commented Aug 4, 2016

koppor commented Aug 10, 2016

oscargus commented Aug 11, 2016

koppor commented Sep 13, 2016

oscargus commented Sep 13, 2016

koppor commented Sep 14, 2016

oscargus commented Sep 14, 2016

koppor commented Sep 15, 2016

lenhard commented Sep 15, 2016

koppor Sep 15, 2016

oscargus Sep 16, 2016

koppor Sep 22, 2016

oscargus commented Sep 16, 2016

koppor left a comment

koppor Sep 22, 2016

koppor Sep 22, 2016

koppor Sep 22, 2016

koppor Sep 22, 2016

tobiasdiez left a comment

tobiasdiez Sep 22, 2016

tobiasdiez Sep 22, 2016

tobiasdiez Sep 22, 2016

tobiasdiez Sep 22, 2016

tobiasdiez Sep 22, 2016

lenhard commented Feb 15, 2017

[WIP] Proper copying of entries #1677

[WIP] Proper copying of entries #1677

Conversation

oscargus commented Aug 4, 2016

koppor commented Aug 10, 2016

oscargus commented Aug 11, 2016

koppor commented Sep 13, 2016

oscargus commented Sep 13, 2016

koppor commented Sep 14, 2016

oscargus commented Sep 14, 2016

koppor commented Sep 15, 2016

lenhard commented Sep 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oscargus commented Sep 16, 2016

koppor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobiasdiez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lenhard commented Feb 15, 2017