[pprzcenter] save conf confirmation on quit

[gautierhattenberger]

If there are unsaved changes, ask if you want to save them on quit of Paparazzi Center.

This solves the "problem" of #1567 if you work on a local conf not under revision, but otherwise, you can still have changes due to checkout, so the startup confirmation will show up.

[flixr]

👍 from me

[kevindehecker]

This "solution" is of course an improvement, because it will make the backup-msgbox appear less often.

However, there are a couple of things amiss here:

1. The culprit has not been removed. E.g. if I do git stuff, I 'm still gonna get that msgbox. And its still incomprehensible (read: plain wrong).
2. Adding a save on quit msgbox is a solution that was very accepted in 1995 GUI design, today less so. It is diverting user attention to something that he's not concerned with at that moment.

I vote for complete removal of both these msgboxes, and the notion of manual saving your conf (i.e. remove the save menu item). Instead of manual saving: always autosave to current_conf.xml but never overwrite. Instead move the old current_conf.xml to conf.old_[time].
When starting and if no current_conf.xml exists, load conf.xml (or start start.py instead of center).

That way, no user interaction is required and users can never accidentally remove changes.

Ideally, there would be a file watcher running on conf.xml in center that automatically detects outside changes and asks the user immediately whether to discard or load those changes. (the user is concerned with those changes right at that moment, as he is causing them right at that moment)

[flixr]

I'm not in favour of completely removing the manual save or forcing autosave on quit:
E.g. often remove some settings, or change some other things for testing, but I don't want to save those in the end...

[flixr]

Maybe an enhancement could be a "autosave" checkbox/menu entry and that setting remembered to leave that choice to the user...

[kevindehecker]

I'm sorry to say, but you are not an example of a typical user :).
Software "testing" is not a high priority use case for most users. Sure sometimes they'll do that too, but most of the times not.

IMHO with my plan, in general, users will benefit. Adding extra options just to make the program work for the developers is a dangerous GUI design pattern.

[flixr]

Personally I HATE autosave, and I turn it of in all editors, etc. I use...

[martinmm]

Forced Autosave is not a good idea.

[kevindehecker]

I'm really seeing some good arguments here ;)

Center is, by the way, not an editor... Rather compare it with an IDE. Visual studio autosaves which property boxes are turned on, where they are etc etc... Why do you think that is?

[flixr]

Editor (or IDE) or not... the point is that not everyone likes autosave...

[kevindehecker]

I don't believe that is your point.

You, HATER of autosave, are maybe using it in several programs to satisfaction right now, but you don't recognize it as such because the workflow is setup different. I don't know which software you use, otherwise I would try to name examples. Are you using a browser? An IDE? Matlab? Google docs? Virtual Box? Blender? All these things are riddled with forced autosave. Props for the persuasion tactic on "forced" autosave though. Because, you know, if it is FORCED it must be bad.

I think your point is that changing your workflow is painful. This is a valid point. If we can agree to that, we can move forward.

[flixr]

Hate might have been a bit too strong of a word, but my point IS that not everyone likes autosave in all cases.
And it looks like I'm not the only one...

It's also really not about my workflow (since the confs I use are almost always committed).

Also to be clear what this PR does:
It shows the save dialog when you click the close window button just like it already shows this dialog if you use quit from the menu.

Maybe all this talk about saving and autosave is also misleading:

• As soon as you change a conf in Paparazzi Center the conf.xml is updated/saved and conf.xml~ "backup" created.
• Save (from menu or dialog) simply removes the conf.xml~ so there is no message about restoring this "backup" version on next start.
• "Discard changes" from the dialog simply throws away all changes you made in this session (restoring conf.xml from conf.xml~)

So to sum up, I think that the possibility to decide at the end if you want to keep the changes you just made or go back to the previous version (or view the difference) is useful. Especially for normal users who don't have their conf under version control.

[EwoudSmeur]

I LOVE autosave. I HATE dialogs. You could ask are you sure after every action. Are you sure you want to merge this?

[flixr]

With the latest changes the conf.xml~ backup will always be deleted on quit so you won't get the "restore backup" msg on next start:

• either you chose to keep your changes
• or you restored the backup (which now also doesn't remove symlinks anymore)
• or the backup was created but didn't actually differ on quit

The only case when you will still have a conf.xml~ is when you kill Paparazzi Center or stop it with CTRL-C.

A checkbox in that message (or the menu) to allow to remember the choice to always keep would still be nice.

[flixr]

@EwoudSmeur I'm also not a fan of dialogs, but I LOVE to have the choice to enable/disable "autosave" features.

[flixr]

So to sum up:
This feature would be more aptly described as "ask to review/keep/discard changes you made in this session" than "ask to save".
IMHO this is a nice feature to have, especially for new users which don't have their own conf under version control...
But maybe I'm wrong and everyone would rather not have something like this.

For me the pain point is the stupid dialog at startup, which also appeared way too often because conf.xml~ was not deleted even if it was identical with the real conf.xml.

If we combine this with a "always keep changes (save)" option that will be remembered, I think we will have the best of both worlds.
However if I'm wrong and nobody else likes this feature to review and potentially discard changes (restore backup from beginning of session), then we might as well remove it completely.

[kirkscheper]

Hey all,
I think that the backup is useful to have just in case you make temporary changes and would like to somehow revert but this is a minority case. Also the use can easily see that the current conf is not correct and doesn't have to be reminded about it. I would like to have the review changes (in a meld type way) but as an option in the pprz centre menu, something like "restore backup" or "review changes" but not a popup window.

In short, don't auto delete the conf.xml~ but replace the popup window with an option in the drop down menu.

This may be a bit different than what was previously mentioned but that's my 2 cents.

[flixr]

@kirkscheper adding a "review changes" entry could of course be added to the menu...

In short, don't auto delete the conf.xml~ but replace the popup window with an option in the drop down menu.

I don't quite understand what you mean... if you "(auto)save" the config, all it does is delete conf.xml~

[flixr]

Added checkboxes to "always keep changes"...

[gautierhattenberger]

Looks like everybody can be happy with this.
@flixr did you solve all your ocaml issues or does it still need some work ?

[flixr]

@gautierhattenberger after wrestling with lablgtk a bit, I managed to do what I wanted (add checkbox to the message dialog as well).
So from my side this is fine to merge.

IMHO we have two choices:

• merging something like this PR that allows to "always keep changes" (or not)
• completely removing the creation of the temporary/backup conf.xml~ (hence "view changes" and "discard changes" features)

[gautierhattenberger]

I like better to option to choose if you want confirmation box or always keep the latest changes

[flixr]

@kevindehecker @martinmm @EwoudSmeur @kirkscheper your feedback on this would be appreciated...

[kevindehecker]

I'm looking for time to react to this, hopefully in the weekend.

[kevindehecker]

1. The original msgbox is still there. In total however, with the optional autosave, this PR has become an improvement. I'm on board merging this now
2. I don't fully grasp why you suggest the second choice in your comment (remove backup functionality). Could you elaborate?
   If I interpret myself I'm voting for the second option (no conf.xml~), as it kind of aligns with the general idea from my first reaction. First, the msgbox where this all started from disappears for good. Secondly, as I understand it, most users should commit their changes to their personal confs. In that case there's really no need for another backup, since we have git for that. For super-safety you can still move conf.xml to conf.xml_timecode each time changes are about to be overwritten, but people would to have manually restore the back up. (no extra options in the GUI, for sake of clarity, maybe only like Kirk says: restore defaults)

[flixr]

Curious when you still get the original startup msgbox? It should only be the case if you forcibly closed pprz center...

A lot of end-users don't really put their conf.xml under version control ;-( At least that is my impression from the mailing list and the chat...

[kevindehecker]

You also get it from external changes (e.g. when checking out another git branch), or did that change in this PR (sorry don't have the time to analyse all the commits) ?

[flixr]

You only get the msgbox if conf.xml~ exists, but since it is now removed on any normal quit (not needed any more, choice is already made) you also shouldn't get the msgbox if you switch branches or do anything like that...