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.

Sign up for GitHub

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

Saving changes and exiting application #4729

Merged
merged 5 commits into from
Mar 11, 2019
Merged

Saving changes and exiting application #4729

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f4d8e09
[WIP] Saving changes and exiting application
CaptainDaVinci Mar 6, 2019
9c55ffa
Merge branch 'master' of https://github.com/JabRef/jabref into fix-is…
CaptainDaVinci Mar 7, 2019
b527768
Refactored confirmClose() method
CaptainDaVinci Mar 7, 2019
e6ba7a8
Display dialog in case of save exception, and remove runInJavaFXThread
CaptainDaVinci Mar 11, 2019
b0b8bd3
Merge branch 'master' of https://github.com/JabRef/jabref into fix-is…
CaptainDaVinci Mar 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/main/java/org/jabref/gui/JabRefFrame.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1334,10 +1334,10 @@ private boolean confirmClose(BasePanel panel) {
} catch (Throwable ex) {
return false;
}
} else {
Copy link
Member

@Siedlerchr Siedlerchr Mar 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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;

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@CaptainDaVinci CaptainDaVinci Mar 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@CaptainDaVinci CaptainDaVinci Mar 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

return !response.isPresent() || !response.get().equals(cancel);
// Exit.
return true;
}
return false;
return !response.isPresent() || !response.get().equals(cancel);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

You are right. Forgot that there is a third option

}

private void closeTab(BasePanel panel) {
Expand Down