Fixed some EDT issues #1725

oscargus · 2016-08-12T17:59:18Z

I looked into EDT bugs and found a few. Quite often they related to AbstractWorker it turned out.

If someone wants to find out more, add the following first in main or start in JabRefMain:

RepaintManager.setCurrentManager(new CheckingRepaintManager());

then it is just to work with JabRef until an exception happens.

The most prominent issue solved was on saving where the UI is frozen during save, but not from the EDT. It is possible that some of the AbstractWorker issues are better to solve by running the complete run in the EDT thread, but somehow I guess one wants to avoid arbitrary processing there and this at least solves the problems.

Manually tested changed features in running JabRef
Check documentation status (Issue created for outdated help page at help.jabref.org?)

oscargus · 2016-08-12T17:59:56Z

src/main/java/net/sf/jabref/external/FindFullTextAction.java

@@ -59,6 +58,7 @@ public void init() throws Throwable {
    public void run() {
        if (basePanel.getSelectedEntries().size() != 1) {
            basePanel.output(Localization.lang("This operation requires exactly one item to be selected."));
+            result = Optional.empty();


This is not related to EDT, but still solves a bug.

Probably better to initialize in the constructor.

I think it needs to be reset. If not, if you run it once and then try to run it with multiple entries selected, update will still be called and then result is not empty.

oscargus · 2016-08-12T20:51:58Z

src/main/java/net/sf/jabref/importer/EntryFromPDFCreator.java

@@ -64,7 +64,7 @@ public boolean accept(File f) {
        PdfImporter pi = new PdfImporter(JabRefGUI.getMainFrame(), JabRefGUI.getMainFrame().getCurrentBasePanel(), JabRefGUI.getMainFrame().getCurrentBasePanel().getMainTable(), -1);
        ImportPdfFilesResult res = pi.importPdfFiles(Collections.singletonList(pdfFile.toString()));
        if (res.getEntries().size() == 1) {
-            return Optional.of(res.getEntries().get(0));
+            return Optional.ofNullable(res.getEntries().get(0));


Bug fix so that one can click "Cancel" instead of selecting an entry type and still not get an NPE.

oscargus · 2016-08-12T21:46:51Z

src/main/java/net/sf/jabref/external/SynchronizeFileField.java

@@ -179,7 +179,8 @@ public void run() {
                            } else {
                                answer = JOptionPane.showOptionDialog(panel.frame(),
                                        Localization.lang("<HTML>Could not find file '%0'<BR>linked from entry '%1'</HTML>",
-                                                flEntry.link, aSel.getCiteKey()),
+                                                flEntry.link,
+                                                aSel.getCiteKeyOptional().orElse(Localization.lang("undefined"))),


NPE when no cite key is set...

tobiasdiez · 2016-08-13T06:58:37Z

src/main/java/net/sf/jabref/gui/FindUnlinkedFilesDialog.java

@@ -532,10 +533,12 @@ private void startSearch() {
                        @Override
                        public void stateChanged(ChangeEvent e) {
                            counter++;
-                            progressBarSearching.setString(counter + " files found");
+                            SwingUtilities.invokeLater(() -> progressBarSearching


This is hard to read. Maybe determine the localized string before calling SwingUtil.invokeLater()

tobiasdiez · 2016-08-13T07:06:50Z

A few remarks, but in general looks good to me. But I have no experience with this swing threading stuff, so it would be good if somebody else also has a look at the code.

koppor · 2016-08-15T07:03:00Z

src/main/java/net/sf/jabref/gui/entryeditor/EntryEditor.java

@@ -947,7 +947,11 @@ public void updateAllContentSelectors() {
    @Subscribe
    public void listen(FieldChangedEvent fieldChangedEvent) {
        String newValue = fieldChangedEvent.getNewValue() == null ? "" : fieldChangedEvent.getNewValue();
-        setField(fieldChangedEvent.getFieldName(), newValue);
+        if (SwingUtilities.isEventDispatchThread()) {


I assume that this if statement is necessary and one cannot just call invokeLater()?

simonharrer · 2016-08-18T09:34:34Z

For a hack, this is ok. I would prefer that a SwingWorker is used which can send updates of its progress. The AbstractWorker with its init/run/update methods is somewhat legacy which will not work with JavaFX at all. But the SwingWorker can be easily ported to the JavaFX equivalent, the stuff behind the AbstractWorker which is based on the spin library cannot. So I would recommend to invest the effort in slowly migrating to SwingWorker and then to JavaFx.

oscargus · 2016-08-18T14:30:15Z

Thanks for looking at it @simonharrer. I take it as merge (subject to the comments) and look at SwingWorker eventually. Much more work to restructure all AbstractWorker but still worth getting rid of the issues fixed here.

I'll have a look at the other comments later tonight.

oscargus · 2016-08-18T14:34:43Z

src/main/java/net/sf/jabref/gui/importer/EntryFromPDFCreator.java

@@ -64,7 +64,7 @@ public boolean accept(File f) {
        PdfImporter pi = new PdfImporter(JabRefGUI.getMainFrame(), JabRefGUI.getMainFrame().getCurrentBasePanel(), JabRefGUI.getMainFrame().getCurrentBasePanel().getMainTable(), -1);
        ImportPdfFilesResult res = pi.importPdfFiles(Collections.singletonList(pdfFile.toString()));
        if (res.getEntries().size() == 1) {
-            return Optional.of(res.getEntries().get(0));
+            return Optional.ofNullable(res.getEntries().get(0));


I got an NPE here at least. I do not remember the details (I think pressing cancel when asked about entry type) and I think I tested it after changing it. although I'm hindsight it looks more likely that the list is null than the list entry and that fixing it at the source may be better. emptyList() will break it as a null is better than index out of bounds. I'll have a new look later tonight.

oscargus · 2016-08-19T16:34:13Z

OK, I changed some things based on the comments and improved the code in a few places.

oscargus · 2016-08-20T09:09:07Z

I merge this in based on the previous reviews, so we have some time to find any unexpected consequences before release.

* master: Made output of MetaData.getEncoding() Optional (JabRef#1790) Some improvements to LayoutFormatterPreferences (JabRef#1793) Updated WireMock to 2.1.10 (JabRef#1792) Fixed some EDT issues (JabRef#1725) Remove commented InternalBibtexFields.getPreferredFieldLength() Readd package comments as class comments Remove GPL headers Moved preference constants to JabRefPreferences and renamed (JabRef#1786) Fix arxiv fetcher not working correctly (JabRef#1780)

* Fixed some EDT issues * More EDT issues solved, an odd bug and a few translations added * Rebased, added CHANGELOG, and a few Swedish translations * Fixed the comments and improved the code a bit * Fixed one more NPE * One more EDT issue

oscargus added bug Confirmed bugs or reports that are very likely to be bugs status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers type: code-quality Issues related to code or architecture decisions labels Aug 12, 2016

oscargus reviewed Aug 12, 2016
View reviewed changes

oscargus force-pushed the edtissues branch from 220db27 to a60b351 Compare August 12, 2016 20:50

oscargus reviewed Aug 12, 2016
View reviewed changes

oscargus added this to the v3.6 milestone Aug 12, 2016

oscargus force-pushed the edtissues branch from a60b351 to 8db530c Compare August 12, 2016 21:45

oscargus reviewed Aug 12, 2016
View reviewed changes

oscargus force-pushed the edtissues branch from 8db530c to 1993072 Compare August 12, 2016 23:32

tobiasdiez reviewed Aug 13, 2016
View reviewed changes

koppor reviewed Aug 15, 2016
View reviewed changes

koppor removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Aug 17, 2016

oscargus mentioned this pull request Aug 17, 2016

Empty Error setting field #1757

Closed

oscargus force-pushed the edtissues branch 2 times, most recently from 3a70e0b to 3f37fc9 Compare August 17, 2016 09:40

stefan-kolb self-assigned this Aug 18, 2016

stefan-kolb removed their assignment Aug 18, 2016

oscargus reviewed Aug 18, 2016
View reviewed changes

Fixed some EDT issues

5028c4f

oscargus added 2 commits August 19, 2016 17:48

More EDT issues solved, an odd bug and a few translations added

bc45df7

Rebased, added CHANGELOG, and a few Swedish translations

fe97059

oscargus force-pushed the edtissues branch 2 times, most recently from 15f99b6 to 61020da Compare August 19, 2016 16:29

Fixed the comments and improved the code a bit

a9a63d9

oscargus force-pushed the edtissues branch from 61020da to a9a63d9 Compare August 19, 2016 19:20

oscargus added 2 commits August 19, 2016 22:06

Fixed one more NPE

1b5f13b

One more EDT issue

39c3fd9

oscargus added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Aug 19, 2016

oscargus merged commit 61d4bd3 into JabRef:master Aug 20, 2016

oscargus deleted the edtissues branch August 20, 2016 09:09

koppor mentioned this pull request Sep 18, 2017

Reenable UI tests JabRef/jabref-koppor#223

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed some EDT issues #1725

Fixed some EDT issues #1725

oscargus commented Aug 12, 2016

oscargus Aug 12, 2016

tobiasdiez Aug 13, 2016

oscargus Aug 17, 2016

oscargus Aug 12, 2016

oscargus Aug 12, 2016

tobiasdiez Aug 13, 2016

tobiasdiez commented Aug 13, 2016 •

edited

Loading

koppor Aug 15, 2016

simonharrer commented Aug 18, 2016

oscargus commented Aug 18, 2016

oscargus Aug 18, 2016

oscargus commented Aug 19, 2016

oscargus commented Aug 20, 2016

Fixed some EDT issues #1725

Fixed some EDT issues #1725

Conversation

oscargus commented Aug 12, 2016

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

tobiasdiez commented Aug 13, 2016 • edited Loading

Choose a reason for hiding this comment

simonharrer commented Aug 18, 2016

oscargus commented Aug 18, 2016

Choose a reason for hiding this comment

oscargus commented Aug 19, 2016

oscargus commented Aug 20, 2016

tobiasdiez commented Aug 13, 2016 •

edited

Loading