Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Proper copying of entries #1677

Merged
merged 1 commit into from
Feb 15, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/main/java/net/sf/jabref/logic/auxparser/AuxParser.java
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ private void resolveTags(AuxParserResult result) {
// Copy database definitions
if (result.getGeneratedBibDatabase().hasEntries()) {
result.getGeneratedBibDatabase().copyPreamble(masterDatabase);
result.getGeneratedBibDatabase().copyStrings(masterDatabase);
result.insertStrings(masterDatabase.getUsedStrings(result.getGeneratedBibDatabase().getEntries()));
}
}

Expand Down
17 changes: 16 additions & 1 deletion src/main/java/net/sf/jabref/logic/auxparser/AuxParserResult.java
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
package net.sf.jabref.logic.auxparser;

import java.util.ArrayList;
import java.util.Collection;
import java.util.HashSet;
import java.util.List;
import java.util.Set;

import net.sf.jabref.logic.l10n.Localization;
import net.sf.jabref.model.database.BibDatabase;
import net.sf.jabref.model.entry.BibtexString;

public class AuxParserResult {

Expand All @@ -17,6 +19,7 @@ public class AuxParserResult {
private final BibDatabase auxDatabase = new BibDatabase();
private int nestedAuxCount;
private int crossRefEntriesCount;
private int insertedStrings;

public AuxParserResult(BibDatabase masterDatabase) {
this.masterDatabase = masterDatabase;
Expand All @@ -42,6 +45,10 @@ public int getUnresolvedKeysCount() {
return unresolvedKeys.size();
}

public int getInsertedStringsCount() {
return insertedStrings;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to add a test case covering that?

}

/**
* Query the number of extra entries pulled in due to crossrefs from other entries.
*
Expand All @@ -59,6 +66,13 @@ public void increaseNestedAuxFilesCounter() {
nestedAuxCount++;
}

public void insertStrings(Collection<BibtexString> usedStrings) {
for (BibtexString string : usedStrings) {
auxDatabase.addString(string);
insertedStrings++;
}
}

/**
* Prints parsing statistics
*
Expand All @@ -73,7 +87,8 @@ public String getInformation(boolean includeMissingEntries) {
.append(Localization.lang("resolved")).append(' ').append(getResolvedKeysCount()).append('\n')
.append(Localization.lang("not_found")).append(' ').append(getUnresolvedKeysCount()).append('\n')
.append(Localization.lang("crossreferenced entries included")).append(' ')
.append(crossRefEntriesCount).append('\n');
.append(crossRefEntriesCount).append(Localization.lang("strings included")).append(' ')
.append(insertedStrings).append('\n');

if (includeMissingEntries && (getUnresolvedKeysCount() > 0)) {
for (String entry : unresolvedKeys) {
Expand Down
40 changes: 35 additions & 5 deletions src/main/java/net/sf/jabref/model/database/BibDatabase.java
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

/**
* Get all strings used in the entries.
*/
public Collection<BibtexString> getUsedStrings(Collection<BibEntry> entries) {
List<BibtexString> result = new ArrayList<>();
Set<String> allUsedIds = new HashSet<>();

// All entries
for (BibEntry entry : entries) {
for (String fieldContent : entry.getFieldValues()) {
resolveContent(fieldContent, new HashSet<>(), allUsedIds);
}
}

// Preamble
if (preamble != null) {
resolveContent(preamble, new HashSet<>(), allUsedIds);
}

for (String stringId : allUsedIds) {
result.add((BibtexString) bibtexStrings.get(stringId).clone());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do you use clone here?

}

return result;
}

/**
Expand Down Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

for (BibtexString string : bibtexStrings.values()) {
if (string.getName().equalsIgnoreCase(label)) {
// First check if this string label has been resolved
Expand All @@ -413,11 +439,14 @@ private String resolveString(String label, Set<String> usedIds) {
}
// If not, log this string's ID now.
usedIds.add(string.getId());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (allUsedIds != null) {
allUsedIds.add(string.getId());
}

// Ok, we found the string. Now we must make sure we
// resolve any references to other strings in this one.
String result = string.getContent();
result = resolveContent(result, usedIds);
result = resolveContent(result, usedIds, allUsedIds);

// Finished with recursing this branch, so we remove our
// ID again:
Expand All @@ -439,7 +468,8 @@ private String resolveString(String label, Set<String> usedIds) {

private static final Pattern RESOLVE_CONTENT_PATTERN = Pattern.compile(".*#[^#]+#.*");

private String resolveContent(String result, Set<String> usedIds) {

private String resolveContent(String result, Set<String> usedIds, Set<String> allUsedIds) {
String res = result;
if (RESOLVE_CONTENT_PATTERN.matcher(res).matches()) {
StringBuilder newRes = new StringBuilder();
Expand All @@ -457,7 +487,7 @@ private String resolveContent(String result, Set<String> usedIds) {
// We found the boundaries of the string ref,
// now resolve that one.
String refLabel = res.substring(next + 1, stringEnd);
String resolved = resolveString(refLabel, usedIds);
String resolved = resolveString(refLabel, usedIds, allUsedIds);

if (resolved == null) {
// Could not resolve string. Display the #
Expand Down
51 changes: 51 additions & 0 deletions src/test/java/net/sf/jabref/model/database/BibDatabaseTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,10 @@
import java.io.IOException;
import java.io.InputStreamReader;
import java.nio.charset.StandardCharsets;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.List;

import net.sf.jabref.Globals;
import net.sf.jabref.importer.ParserResult;
Expand Down Expand Up @@ -317,4 +320,52 @@ public void resolveForStringsOddHashMarkAtTheEnd() {
database.addString(string);
assertEquals(database.resolveForStrings("AAA#AAA#AAA#"), "AAAaaaAAA#");
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

BibtexString string2 = new BibtexString(IdGenerator.next(), "BBB", "Some more text");
BibtexString string3 = new BibtexString(IdGenerator.next(), "CCC", "Even more text");
database.addString(string);
database.addString(string2);
database.addString(string3);
database.insertEntry(entry);
List<BibtexString> usedStrings = (List<BibtexString>) database.getUsedStrings(Arrays.asList(entry));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

assertEquals(2, usedStrings.size());
assertTrue((string.getName() == usedStrings.get(0).getName())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

|| (string.getName() == usedStrings.get(1).getName()));
assertTrue((string2.getName() == usedStrings.get(0).getName())
|| (string2.getName() == usedStrings.get(1).getName()));
assertTrue((string.getContent() == usedStrings.get(0).getContent())
|| (string.getContent() == usedStrings.get(1).getContent()));
assertTrue((string2.getContent() == usedStrings.get(0).getContent())
|| (string2.getContent() == usedStrings.get(1).getContent()));
}

@Test
public void getUsedStringsSingleString() {
BibEntry entry = new BibEntry(IdGenerator.next());
entry.setField("author", "#AAA#");
BibtexString string = new BibtexString(IdGenerator.next(), "AAA", "Some other text");
BibtexString string2 = new BibtexString(IdGenerator.next(), "BBB", "Some more text");
database.addString(string);
database.addString(string2);
database.insertEntry(entry);
List<BibtexString> usedStrings = (List<BibtexString>) database.getUsedStrings(Arrays.asList(entry));
assertEquals(string.getName(), usedStrings.get(0).getName());
assertEquals(string.getContent(), usedStrings.get(0).getContent());
}

@Test
public void getUsedStringsNoString() {
BibEntry entry = new BibEntry(IdGenerator.next());
entry.setField("author", "Oscar Gustafsson");
BibtexString string = new BibtexString(IdGenerator.next(), "AAA", "Some other text");
database.addString(string);
database.insertEntry(entry);
Collection<BibtexString> usedStrings = database.getUsedStrings(Arrays.asList(entry));
assertEquals(Collections.emptyList(), usedStrings);
}
}
3 changes: 2 additions & 1 deletion src/test/resources/net/sf/jabref/logic/auxparser/config.bib
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
@String {maintainer = "Stefan Kolb"}
@preamble {"Maintained by " # maintainer}
@String {einstein = "Einstein, Albert"}

@Book{Newton1999,
title = {The Principia: mathematical principles of natural philosophy},
Expand All @@ -19,7 +20,7 @@ @Book{Einstein1920
title = {Relativity: The special and general theory},
publisher = {Penguin},
year = {1920},
author = {Einstein, Albert}
author = einstein
}

@Comment{jabref-meta: databaseType:bibtex;}