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

#6057 Improve startup time #7486

Merged
merged 11 commits into from
Mar 10, 2021
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve
- We changed the title of the window "Manage field names and content": to have the same title as the corresponding menu item [#6895](https://github.com/JabRef/jabref/pull/6895)
- We improved the detection of "short" DOIs [6880](https://github.com/JabRef/jabref/issues/6880)
- We improved the duplicate detection when identifiers like DOI or arxiv are semantiaclly the same, but just syntactically differ (e.g. with or without http(s):// prefix). [#6707](https://github.com/JabRef/jabref/issues/6707)
- We improved JabRef start up time [6057](https://github.com/JabRef/jabref/issues/6057)
- We changed in the group interface "Generate groups from keywords in a BibTeX field" by "Generate groups from keywords in the following field". [#6983](https://github.com/JabRef/jabref/issues/6983)
- We changed the name of a group type from "Searching for keywords" to "Searching for a keyword". [6995](https://github.com/JabRef/jabref/pull/6995)
- We changed the way JabRef displays the title of a tab and of the window. [4161](https://github.com/JabRef/jabref/issues/4161)
Expand Down
15 changes: 14 additions & 1 deletion src/main/java/org/jabref/gui/maintable/columns/FieldColumn.java
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,12 @@
import org.jabref.gui.maintable.MainTableColumnModel;
import org.jabref.gui.util.ValueTableCellFactory;
import org.jabref.gui.util.comparator.NumericFieldComparator;
import org.jabref.model.entry.field.Field;
import org.jabref.model.entry.field.FieldFactory;
import org.jabref.model.entry.field.OrFields;
import org.jabref.model.entry.field.UnknownField;

import com.google.common.collect.Iterables;

/**
* A column that displays the text-value of the field
Expand All @@ -26,7 +30,16 @@ public FieldColumn(MainTableColumnModel model) {
new ValueTableCellFactory<BibEntryTableViewModel, String>()
.withText(text -> text)
.install(this);
this.setComparator(new NumericFieldComparator());

if (fields.size() == 1) {
// comparator can't parse more than one value
Field field = Iterables.getOnlyElement(fields);

if (field instanceof UnknownField || field.isNumeric()) {
this.setComparator(new NumericFieldComparator());
}
}

this.setSortable(true);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

import java.util.Comparator;

import org.jabref.model.strings.StringUtil;

/**
* Comparator for numeric cases. The purpose of this class is to add the numeric comparison, because values are sorted
* as if they were strings.
Expand All @@ -10,51 +12,54 @@ public class NumericFieldComparator implements Comparator<String> {

@Override
public int compare(String val1, String val2) {
// We start by implementing the comparison in the edge cases (if one of the values is null).
if (val1 == null && val2 == null) {
return 0;
}
Integer valInt1 = parseInt(val1);
Integer valInt2 = parseInt(val2);

if (val1 == null) {
if (valInt1 == null && valInt2 == null) {
if (val1 != null && val2 != null) {
return val1.compareTo(val2);
} else {
return 0;
}
} else if (valInt1 == null) {
// We assume that "null" is "less than" any other value.
return -1;
}

if (val2 == null) {
} else if (valInt2 == null) {
return 1;
}

// Now we start the conversion to integers.
Integer valInt1 = null;
Integer valInt2 = null;
try {
// Trim in case the user added an unnecessary white space (e.g. 1 1 instead of 11).
valInt1 = Integer.parseInt(val1.trim());
} catch (NumberFormatException ignore) {
// do nothing
// If we arrive at this stage then both values are actually numeric !
return valInt1 - valInt2;

Choose a reason for hiding this comment

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

I know this isn't your code, but I'd rather see valInt1.compareTo(valInt2) here (even if it should not overflow with reasonable values). I believe it will help with avoiding auto-(un)boxing as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will break a test

Choose a reason for hiding this comment

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

For compareTwoNumericInputs I'd go either with 1 or changing it along the lines of https://junit.org/junit5/docs/snapshot/user-guide/index.html#writing-tests-test-interfaces-and-default-methods
e.g.,

assertTrue(comparator.compare("4", "2") > 0);

}

private static Integer parseInt(String number) {
if (!isNumber(number)) {
return null;
}

try {
valInt2 = Integer.parseInt(val2.trim());
return Integer.valueOf(number.trim());
} catch (NumberFormatException ignore) {
// do nothing
return null;
}
}

if (valInt1 == null && valInt2 == null) {
// None of the values were parsed (i.e both are not numeric)
// so we will use the normal string comparison.
return val1.compareTo(val2);
private static boolean isNumber(String number) {
if (StringUtil.isNullOrEmpty(number)) {
return false;
}

if (valInt1 == null) {
// We assume that strings "are less" than integers
return -1;
if (number.length() == 1 && (number.charAt(0) == '-' || number.charAt(0) == '+')) {
return false;
}

if (valInt2 == null) {
return 1;
for (int i = 0; i < number.length(); i++) {
char c = number.charAt(i);
if (i == 0 && (c == '-' || c == '+')) {
continue;
} else if (!Character.isDigit(c)) {
return false;
}
}

// If we arrive at this stage then both values are actually numeric !
return valInt1 - valInt2;
return true;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -42,4 +42,24 @@ public void compareStringWithInteger() {
public void compareIntegerWithString() {
assertEquals(1, comparator.compare("4", "hi"));
}

@Test
public void compareNegativeInteger() {
assertEquals(1, comparator.compare("-4", "-5"));
}

@Test
public void compareWithMinusString() {
assertEquals(-1, comparator.compare("-", "-5"));
}

k3KAW8Pnf7mkmdSMPHz27 marked this conversation as resolved.
Show resolved Hide resolved
@Test
public void compareWithPlusString() {
assertEquals(-1, comparator.compare("+", "-5"));
}

@Test
public void compareWordWithMinus() {
assertEquals(-1, comparator.compare("-abc", "-5"));
}
}