Saving changes and exiting application

[CaptainDaVinci]

Fixes #4728

When the user clicks Save changes option, the status of
saving is not shown unlike before. I am not sure what might be
the cause of that.

---

• Change in CHANGELOG.md described
• Tests created for changes
• Manually tested changed features in running JabRef
• Screenshots added in PR description (for bigger UI changes)
• Ensured that the git commit message is a good one
• Check documentation status (Issue created for outdated help page at help.jabref.org?)

[CaptainDaVinci]

The tearDownJabRef(filenames) executes before the files are actually saved, would this be responsible for the UI not being shown?

[Siedlerchr]

I don't have the code at hands, but this could be a reason. You could debug and see where the save happens and when the instance should be closed

[CaptainDaVinci]

Okay, so this is what I think is happening.

The SaveDatabaseAction class' doSave() updates UI on JavaFxThread, however the call to tearDownJabRefFrame executes before that and a blank window is shown instead of the UI.

I placed a lock before calling tearDownJabRefFrame, hence, blocking that thread. But the code under doSave() method, wasn't executed, shouldn't they be running on separate threads?

[Siedlerchr]

I'll take a look at it tomorrow Yash Kothari <notifications@github.com> schrieb am Mi., 6. März 2019, 19:09:

…

Okay, so this is what I think is happening. The SaveDatabaseAction class' doSave() <https://github.com/JabRef/jabref/blob/master/src/main/java/org/jabref/gui/exporter/SaveDatabaseAction.java#L141> updates UI on *JavaFxThread*, however the call to tearDownJabRefFrame executes before that and a blank window is shown instead of the UI. I placed a lock before calling tearDownJabRefFrame <https://github.com/JabRef/jabref/blob/master/src/main/java/org/jabref/gui/JabRefFrame.java#L516>, hence, blocking that thread. But the code under doSave() <https://github.com/JabRef/jabref/blob/master/src/main/java/org/jabref/gui/exporter/SaveDatabaseAction.java#L141> method, wasn't executed, shouldn't they be running on separate threads? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#4729 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AATi5IUO25BN99fnkHBJeATCdwABP-JYks5vUARIgaJpZM4bhHoJ> .

[Siedlerchr]

Your idea here was good, but not completely right:
You actually had to return true for the case that the save was okay:

   if (response.isPresent() && response.get().equals(saveChanges)) {
            // The user wants to save.
            try {
                SaveDatabaseAction saveAction = new SaveDatabaseAction(panel, Globals.prefs);
                if (!saveAction.save()) {
                    // The action was either canceled or unsuccessful.
                    output(Localization.lang("Unable to save library"));
                    return false;
                } else {
                    return true; //here the save was sucessful
                }
            } catch (Throwable ex) {
                return false;
            }
        }
        return false;

[Siedlerchr]

And while you are there, please add a LOGGER.errror statement for the exception

[CaptainDaVinci]

I agree, that is a better place to put the return true statement. However, wouldn't both work the same way?
In case the save fails, it returns false, or if an exception occurs it is caught and again returns false. If none of these cases occur, it returns true.

[Siedlerchr]

Even easier: you can remove all return false statements (except the one at the bottom of the function) and only have the return true in the else case. And I would switch the if/else for saveDatabase to

if(saveAction.save)`
{
return true; 
{
else
{ output....
}

So when no return true is called, the function will automatically return false per last statement.

[CaptainDaVinci]

But we would also have to take care of the case when the saveAction.save throws an exception.
Also, the last statement could return true as well if the user clicked cancel, but that is already taken care of.

[CaptainDaVinci]

I'll make the appropriate changes and push the code.

[Siedlerchr]

Here it is sufficient to return simply false, because you do the check already as the very first. And the user can only click save or cancel.

[CaptainDaVinci]

Aren't there 3 options, Save changes, Discard changes and Return to JabRef?

[CaptainDaVinci]

If only return false is used, then Discard changes will not quit the application, is that the intended behavior?

[Siedlerchr]

You are right. Forgot that there is a third option

[Siedlerchr]

lgtm

[tobiasdiez]

I'm not sure if I understand the changes correctly. (see remarks below)

As a solution, I would suggest to remove the runInJavaFXThread call in the doSave method (this should be done either way).

[tobiasdiez]

Please also add a dialogService.showError() call, because we also want to notify the user that we were unable to save the library.

[CaptainDaVinci]

I have a doubt, since the doSave() method handles SaveException, what exceptions are we trying to catch under confirmClose()?

[Siedlerchr]

Please remove the runInJavaFXThread call only when the doSave is called from GUI methods. It might that the doSave could be called from a background thread which will result in a not on fx thread exception.

[CaptainDaVinci]

Will adding a new method saveFromGUI() in SaveDatabaseAction be a good approach to solving this problem? This, I think, will lead to some redundant code or will require refactoring of the save method.

[tobiasdiez]

I had a (quick) look and couldn't find a caller invoking the save method from a background task. But could be that I overlooked something (there are quite a few indirect calls to this method). So I think it is safe to remove it (and if not than the method should be refactored anyway).