Add confirmation dialogue on exit with unsaved changes.

[AndyZe]

commit f7d6a77
Author: Andy Zelenak andyz@utexas.edu
Date: Mon Apr 11 17:07:36 2016 -0600

Add dialog box when exiting with unsaved changes. Issue #50

commit 37b7d9c
Author: Andy Zelenak andyz@utexas.edu
Date: Mon Apr 11 16:03:14 2016 -0600

Detecting all new joints/links, all deletes, and all changes. Reordering may not be detected.

[AndyZe]

This might not detect if the tree gets reordered, but I can't test that since it's not implemented yet (Issue #26). I'm thinking we should create a new issue for that

[gavanderhoorn]

Hi Andy, thanks for the PR.

I test it on top of #73 and it works as described. As you write, we can't test reordering, as it isn't there yet.

A general note: do you think it would be possible to change your implementation slightly and have the confirmation dialogue shown before the main UI is hidden? Functionally it would all remain the same, but without the main UI the dialogue seems to 'come out of nowhere', which seems a bit strange.

I'm guessing an implementation as shown in the QWidget documentation (by hooking into QWidget::closeEvent(..), also check the Application Example) would be the most efficient here.

[gavanderhoorn]

Another advantage of catching the close event would be that we can add the ability to cancel closing the application from the confirmation dialogue.

[gavanderhoorn]

is this a comment that should be removed, or do you feel the else branch should be removed?

[AndyZe]

That was just a comment, since this is the converse of "Add". But I'll remove it, since that is confusing

[gavanderhoorn]

Another general comment: would it make sense to define some additional slots (in urdf_property.h) for joint and link addition & deletion (and later: ordering, if not covered by valueChanged), have them emitted at the appropriate times (ie: upon addition and deletion of a link or joint), and then register a callback that sets unsavedChanges to true whenever it is called?

That way we'd avoid having to add lines that update unsavedChanges everywhere, and exploit the Qt support for the observer/observable pattern to end up with what we are after.

[AndyZe]

@gavanderhoorn I've talked to several people and they recommended removing the confirmation after "Save As." It was just one too many boxes to click through. I think I will include that in the next commit.

[AndyZe]

@gavanderhoorn Please clarify about QWidget::closeEvent(..). We already have URDFEditor::on_actionE_xit_triggered (sic). Is that where you want me to catch the close?

on_actionE_xit_triggered is only called from File-->Exit. It's not called if you quit by clicking the X.

[gavanderhoorn]

29a146b looks very nice, thanks.

As to closeEvent(..): the QWidget::closeEvent(..) shows what the idea would be: override the closeEvent(..) method on QMainWindow and check for unsavedChanges == true there. If the user selects cancel, you ignore the event, effectively cancelling the quit action. If the user selects 'save', you show the dialogue, and finally accept the event (perhaps it should also take a 'cancel' in the save dialogue into account).

The close event will also be triggered by on_actionE_xit_triggered (yeah, need to correct the typo).

A more complete example is shown in the Application Example. The MainWindow class is essentially our URDFEditor.

[gavanderhoorn]

@AndyZelenak wrote:

@gavanderhoorn I've talked to several people and they recommended removing the confirmation after "Save As." It was just one too many boxes to click through. I think I will include that in the next commit.

ok. I found the lack of confirmation more annoying, but perhaps we should only popup a dialogue when saving was not successful.

Wrt saveURDF_withConfirm(..) and saveURDF_noConfirm(..): there is quite some duplicated code there. Perhaps we could move the UI handling to URDFEditor::on_action_Save_triggered() and URDFEditor::on_actionSave_As_triggered() and use the return code from bool URDFProperty::saveURDF(..) in the if (..) else (..) that triggers the dialogue?

[gavanderhoorn]

Merging in #73 and #75 introduced some conflicts with this PR, but I'll fix that when merging this in, you don't have to take care of those (I'd appreciate it though ;)).

[AndyZe]

@gavanderhoorn I think this has everything. I'll rebase & squash tomorrow if you don't accept it before then.

[gavanderhoorn]

Indentation seems to be off here. ROS C++ style is 2 spaces per level.

[gavanderhoorn]

Thanks for rebasing @AndyZelenak, that saved me some work.

There is still one minor issue: cancelling the 'save as' dialogue opened by clicking Yes in the Save changes? dialogue will quit the application. I found that unexpected, as other applications typically return the user to their document/work in that case.

That should be just some shuffling around of the if/else clauses/bodies, so we can ticket that and fix it later.

[gavanderhoorn]

I've just manually merged this PR to include some minor changes and to rebase it on current indigo-devel.

Thanks for this @AndyZelenak, appreciated 👍.