#6057 Improve startup time

[Ali96kz]

#6057 Check string is number with regex instead of throwing exception. It will improve start time because when I have 3000+ entries it throws about ~170.000 NumberFormatException

• Change in CHANGELOG.md described in a way that is understandable for the average user (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.

[Siedlerchr]

I like the idea, but please check the org.jabref.gui.util.comparator.NumericFieldComparatorTest

[tobiasdiez]

Thanks, that's indeed a nice improvement. However, regex are also pretty expensive, so there is still room for improvement. For example, https://stackoverflow.com/questions/11875424/java-regular-expression-offers-any-performance-benefit discusses a neat way using character-wise checks. There are some issues with this approach e.g. with negative or decimal numbers, but that's not really something we care about in the main table, so we can ignore them.

Another huge improvement would be to not use the numeric comparator for all fields by default. For example, it doesn't make any sense to try comparing authors by converting them first to numbers. I think, only the year columns needs such a treatment...and custom columns for which we don't know the format #6349.

[Ali96kz]

@tobiasdiez I changed the number validation mechanism. I don't know how to implement the second suggestion because there is no way to get data type from MainTableColumnModel, what about creating a separate task?

[tobiasdiez]

Thanks for the quick follow-up. Two minor remarks from my side.

Concerning the installation of the compartor in the main table, you have the field available at

jabref/src/main/java/org/jabref/gui/maintable/columns/FieldColumn.java

Line 21 in c8f7be8

this.fields = FieldFactory.parseOrFields(model.getQualifier());

.
Thus you could add a check at

jabref/src/main/java/org/jabref/gui/maintable/columns/FieldColumn.java

Line 29 in c8f7be8

this.setComparator(new NumericFieldComparator());

if the field is a UnknownField or if it is numeric:

jabref/src/main/java/org/jabref/model/entry/field/Field.java

Line 35 in f7de0a6

default boolean isNumeric() {

[tobiasdiez]

The checkstyle fails; apart from this is good to go from my side.

[Ali96kz]

@k3KAW8Pnf7mkmdSMPHz27 Are you a second reviewer?

[k3KAW8Pnf7mkmdSMPHz27]

@Ali96kz unless someone else is taking a look at it earlier, I should be able to review it in a couple of hours.

[k3KAW8Pnf7mkmdSMPHz27]

It looks good to me! There is only one change I truly require (line 51 in NumericFieldComparator) and the rest are perhaps nitpicking.
Other than that, good changes. I am looking forward to the performance improvement 😍

(Sorry about the slightly later than promised review, I were less familiar with OrFields than I expected)

[k3KAW8Pnf7mkmdSMPHz27]

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.

[Ali96kz]

It will break a test

[k3KAW8Pnf7mkmdSMPHz27]

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

[Siedlerchr]

thanks for tackling this issue!