Test case for ioob (and some code cleanup) #10399

koppor · 2023-09-18T21:27:00Z

This adds src/test/java/org/jabref/gui/maintable/MainTableDataModelTest for checking that in a "controlled" environment, all listeners are installed correctly. - It is a copy of the UI code. However, in JabRef, the UI does not behave the same.

While I was on it, some minor code improvements were made.

Follow up to #10389

More things added

Comparators fixed: null is smaller than something in
CRLF handling on non-Windows machines

Mandatory checks

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 developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

Comment was NOT matching the code (it was the other way round)

Co-authored-by: Christoph <siedlerkiller@gmail.com>

github-actions · 2023-09-18T21:28:44Z

src/test/java/org/jabref/gui/maintable/MainTableDataModelTest.java

+package org.jabref.gui.maintable;
+
+import java.util.List;
+import java.util.stream.Collectors;


🚫 [reviewdog] <com.puppycrawl.tools.checkstyle.checks.imports.UnusedImportsCheck> _{reported by reviewdog 🐶}
Unused import - java.util.stream.Collectors.

github-actions · 2023-09-18T21:28:44Z

src/test/java/org/jabref/gui/maintable/MainTableDataModelTest.java

+import java.util.stream.Collectors;
+
+import javafx.beans.InvalidationListener;
+import javafx.beans.Observable;


🚫 [reviewdog] <com.puppycrawl.tools.checkstyle.checks.imports.UnusedImportsCheck> _{reported by reviewdog 🐶}
Unused import - javafx.beans.Observable.

github-actions · 2023-09-18T21:28:44Z

src/test/java/org/jabref/gui/maintable/MainTableDataModelTest.java

+import javafx.beans.property.IntegerProperty;
+import javafx.beans.property.SimpleIntegerProperty;
+import javafx.beans.property.SimpleObjectProperty;
+import javafx.beans.value.ObservableValue;


🚫 [reviewdog] <com.puppycrawl.tools.checkstyle.checks.imports.UnusedImportsCheck> _{reported by reviewdog 🐶}
Unused import - javafx.beans.value.ObservableValue.

github-actions · 2023-09-18T21:28:44Z

src/test/java/org/jabref/gui/maintable/MainTableDataModelTest.java

+import com.tobiasdiez.easybind.EasyBind;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.*;


🚫 [reviewdog] <com.puppycrawl.tools.checkstyle.checks.imports.AvoidStarImportCheck> _{reported by reviewdog 🐶}
Using the '.' form of import should be avoided - org.junit.jupiter.api.Assertions..

github-actions · 2023-09-18T21:31:45Z

Your code currently does not meet JabRef's code guidelines. The tool reviewdog already placed comments on GitHub to indicate the places. See the tab "Files" in you PR. Please carefully follow the setup guide for the codestyle. Afterwards, please run checkstyle locally and fix the issues.

More information on code quality in JabRef is available at https://devdocs.jabref.org/getting-into-the-code/development-strategy.html.

Siedlerchr · 2023-09-19T04:10:43Z

src/main/java/org/jabref/gui/fieldeditors/AbstractEditorViewModel.java

@@ -65,8 +65,7 @@ public void bindToEntry(BibEntry entry) {
                fieldBinding,
                newValue -> {
                    if (newValue != null) {
-                        // Controlsfx uses hardcoded \n for multiline fields, but JabRef stores them in OS Newlines format
-                        String oldValue = entry.getField(field).map(value -> value.replace(OS.NEWLINE, "\n").trim()).orElse(null);
+                        String oldValue = entry.getField(field).map(String::trim).orElse("");


The source field always uses \n as separator internally. We need this check to prevent the change detection

The source field always uses \n as separator internally.

I saw it in the debugger.

Therefore

value.replace(OS.NEWLINE, "\n")

is a no-op.

Reason:

Let's split up the cases.

OS.NEWLINE is \r\n. Since "**always uses \n **" is true, \r\n is never matched. So no replacement is made

OS.NEWLINE is \n. Since "**always uses \n **" is true, \n is replaced by \n. It is replaced by the same character. The identity. Thus, no change.

Did I miss something?

Side note: During the file write, the newlines are now normalized. The whole file does have the same newline separator. It came in with #8238.

Yes, we have to compare old entry and new Entry.
New entry is what controlsfx uses.
Old Entry however is what comes from the stored bib file, might have had OS.Newline -> Replaced with \n and compare with new entry from source tab so that we don't create a diff "database changed"

Note that I kept the trim.

Yes, but that does not unify OS.newline

If I have a bib file with OS.newline chars e.-g \r\n and now I change the bibtex source -> This will trigger a change

OMG. Thus, if you open the attached file (CRLF terminators), go into the comment field, the cursor will jump?

crlf.bib.txt

I now hard-coded CRLF in the code (instead of relying on OS.Newline)

That's why we replace the chars.

This reverts commit 8313ae4.

Siedlerchr · 2023-09-19T10:26:23Z

src/main/java/org/jabref/gui/fieldeditors/AbstractEditorViewModel.java

+                        // A file may be loaded using CRLF. ControlsFX uses hardcoded \n for multiline fields.
+                        // Normalizing is done during writing of the .bib file (see org.jabref.logic.exporter.BibWriter.BibWriter).
+                        // Thus, we need to normalize the line endings.
+                        String oldValue = entry.getField(field).map(value -> value.replace("\r\n", "\n").trim()).orElse(null);


OS.Newline please as mac only uses \r

I thought, that was Mac OS 9 and earlier: Source https://stackoverflow.com/a/13295575/873282. Mac OS 9 was EOL in 2001.

More sources: https://de.wikipedia.org/wiki/Zeilenvorschub#Computer links to https://de.wikipedia.org/wiki/Mac_OS_(Classic) for \r as line separator.

Please double check if we really need to support \r. I would really consider it being outdated!

My case illustrated by crlf.bib is as follows:

A Windows user edits a file using their favourit editor.

The editor stores CRLF

The file is sent to a Linux user

The Linux user opens the file and positions the cursor in the comment field

Autosave triggers

Cursor moves (this is described in the comments why we need these checks)

If I do not overlook something: OS.Newline does not help: OS.Newline is \n on Linux. The file uses \r\n (because it was created on Windows). - In other words: OS.Newline is dependent on the current operating system and not on the operating system the .bib file was created.

I could route through org.jabref.model.database.BibDatabase#getNewLineSeparator to org.jabref.gui.fieldeditors.AbstractEditorViewModel#bindToEntry. This seems to be too much effort for the gain that \r\n is supported.

Yes, but Excel export to CSV still uses CR. :-P

W.T.F...

For reference: https://answers.microsoft.com/en-us/msoffice/forum/all/csv-line-endings/2b4eedaf-22ef-4091-b7dc-3317303d2f71

In case we ever encounter this, we can use git magic to auto-fix: https://nicercode.github.io/blog/2013-04-30-excel-and-line-endings/

Siedlerchr · 2023-09-19T11:14:09Z

Error: eckstyle] [ERROR] /home/runner/work/jabref/jabref/src/main/java/org/jabref/gui/fieldeditors/AbstractEditorViewModel.java:16:8: Unused import - org.jabref.logic.util.OS. [UnusedImports]

github-actions · 2023-09-19T12:37:55Z

The build for this PR is no longer available. Please visit https://builds.jabref.org/main/ for the latest build.

Co-authored-by: Christoph <siedlerkiller@gmail.com>

koppor and others added 10 commits September 18, 2023 21:17

Rename variable

86bd232

More debug

305cdd1

No more newline issues (I debugged)

8313ae4

Comment was NOT matching the code (it was the other way round)

Remove obsolete workround

f23bfe1

Remove deprecation (and sort properties in the same way)

4fcf2ff

Add initial test

718b273

Fix comparator logic and tests

0e4bcb2

Refine test

60adf7e

Remove empty line

bb794e9

Remove comments

9055105

Co-authored-by: Christoph <siedlerkiller@gmail.com>

koppor added the dev: code-quality Issues related to code or architecture decisions label Sep 18, 2023

Merge branch 'main' into work-in-ioob

a9ecda1

github-actions bot reviewed Sep 18, 2023

View reviewed changes

Remove some empty lines

6c0c2bd

Siedlerchr reviewed Sep 19, 2023

View reviewed changes

koppor added 4 commits September 19, 2023 10:17

Fix imports

ee70cb6

Add comment

3745b3f

Revert "No more newline issues (I debugged)"

433c2ad

This reverts commit 8313ae4.

Refine comment (and use \r\n)

5e8c596

Siedlerchr reviewed Sep 19, 2023

View reviewed changes

Siedlerchr previously approved these changes Sep 19, 2023

View reviewed changes

koppor dismissed Siedlerchr’s stale review via 5755ac0 September 19, 2023 12:06

koppor added 2 commits September 19, 2023 14:07

Fix checkstyle

5755ac0

Merge branch 'main' into work-in-ioob

c2a8748

koppor enabled auto-merge September 19, 2023 12:07

koppor disabled auto-merge September 19, 2023 12:39

koppor merged commit aad9618 into main Sep 19, 2023

koppor deleted the work-in-ioob branch September 19, 2023 12:39

Siedlerchr mentioned this pull request Sep 19, 2023

index out of bounds - related to table sort order and biblatex source tab #10403

Open

2 tasks

lbenicio pushed a commit to lbenicio/jabref that referenced this pull request Sep 26, 2023

Test case for ioob (and some code cleanup) (JabRef#10399)

00854fc

Co-authored-by: Christoph <siedlerkiller@gmail.com>

Siedlerchr mentioned this pull request Oct 5, 2023

Adding bibliography entries will trigger uncaughted exception occurred in Thread #10420

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test case for ioob (and some code cleanup) #10399

Test case for ioob (and some code cleanup) #10399

koppor commented Sep 18, 2023 •

edited

Loading

github-actions bot Sep 18, 2023

github-actions bot Sep 18, 2023

github-actions bot Sep 18, 2023

github-actions bot Sep 18, 2023

github-actions bot commented Sep 18, 2023

Siedlerchr Sep 19, 2023

koppor Sep 19, 2023

Siedlerchr Sep 19, 2023 •

edited

Loading

koppor Sep 19, 2023

Siedlerchr Sep 19, 2023

koppor Sep 19, 2023

Siedlerchr Sep 19, 2023

Siedlerchr Sep 19, 2023

koppor Sep 19, 2023

Siedlerchr Sep 19, 2023 •

edited by koppor

Loading

koppor Sep 19, 2023

Siedlerchr commented Sep 19, 2023

github-actions bot commented Sep 19, 2023 •

edited

Loading

Test case for ioob (and some code cleanup) #10399

Test case for ioob (and some code cleanup) #10399

Conversation

koppor commented Sep 18, 2023 • edited Loading

Mandatory checks

github-actions bot Sep 18, 2023

Choose a reason for hiding this comment

github-actions bot Sep 18, 2023

Choose a reason for hiding this comment

github-actions bot Sep 18, 2023

Choose a reason for hiding this comment

github-actions bot Sep 18, 2023

Choose a reason for hiding this comment

github-actions bot commented Sep 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siedlerchr Sep 19, 2023 • edited Loading

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

Choose a reason for hiding this comment

Siedlerchr Sep 19, 2023 • edited by koppor Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siedlerchr commented Sep 19, 2023

github-actions bot commented Sep 19, 2023 • edited Loading

koppor commented Sep 18, 2023 •

edited

Loading

Siedlerchr Sep 19, 2023 •

edited

Loading

Siedlerchr Sep 19, 2023 •

edited by koppor

Loading

github-actions bot commented Sep 19, 2023 •

edited

Loading