Popup based confirm dialog on quit
Changes PlannedPublic

Authored by Diego Gangl (januz) on May 30 2014, 4:53 PM.

Details

Summary

This patch adds a confirm dialog when quitting a file with unsaved changes on platforms that don't supply a native dialog (currently Linux and BSD).

(the "Discard" text was changed to "Don't save" in the last revision)

Diff Detail

Repository
rB Blender
Branch
arcpatch-D566
Build Status
Buildable 1106
Build 1106: arc lint + arc unit

The reason why we choose platform (window manager) widgets was to make it always work, also when Blender's window is minimized, on another screen, or not in focus.

Some questions:

1- Should I make a task for this, so we can discuss about the design?
2- Is there any way to center the text in the buttons, like in the render properties?
3- If the user clicks in the window close button multiple times (or any other window manager exit-thing) the popup comes up again and starts piling up. I tried using a static var to check if it was already open before creating it, but if the user cancels the popup by clicking outside of it I'm not able to reset the var and the dialog never opens again. Is there any way to keep the popup open, even if the user clicks outside of it?

@Ton Roosendaal (ton) That makes sense. But how would it work on Linux where you can't anticipate what toolkit is installed?

This would be an improvement to the current exit dialog. But please don't invent when adding standard UI elements. There are multiple reasons why this dialog is harder to understand than the usual one:

Save changes to "MyProject" before closing?

Yes No Cancel

@Paweł Łyczkowski (plyczkowski) Yeah, that sounds a lot better. I'd still use verbs (save/discard) instead of yes/no though.

I'd still use verbs (save/discard) instead of yes/no though.

There is a problem with "Discard": When you have it as an option, there is no good answer to the question "Save changes to "MyProject" before closing?" - the user wants to answer "No, I don't want to save the file", but there is no such option. Discard is vague.

So, use this instead:

Save changes to "MyProject" before closing?
Save | Close without saving | Cancel

Alternatievely:

Save changes to "MyProject" before closing?
Save | Don't Save | Cancel

I originally chose "Discard" because it's short and explicit (doesn't make the button too big), but "Don't Save" works as well.

Diego Gangl (januz) planned changes to this revision.Jul 22 2014, 1:53 AM

Thinking of making this popup a sort of "fallback" for the confirm dialog instead of the single option.

Changes:

  • Add a method to GHOST_System to report wether the system supports native dialogs or not (simple bool return)
  • If the system supports native dialogs (Win and Mac), Blender calls Ghost::confirmQuit as usual
  • If the system doesn't, (X11 and SDL) Blender calls the popup dialog

Reworked the popup dialog to work as a fallback for systems with no native dialogs.

  • Improved text in dialog with Pawel's suggestions
  • Added a method in GHOST to declare if a system supports native dialogs
  • Use said method to decide which dialog to call
  • Some comment/code cleanups

Works in Linux, anyone can help me test in Windows/Mac?

Campbell Barton (campbellbarton) requested changes to this revision.Feb 4 2015, 8:28 PM

Marking as needing changes - from windows & mac devs.

This revision now requires changes to proceed.Feb 4 2015, 8:28 PM

@Diego Gangl (januz), could you update the patch? (Didn't test, but guess it won't apply to current master)
Would be an interesting project for 2.8.

Diego Gangl (januz) removed rB Blender as the repository for this revision.

Rebase

@Julian Eisel (Severin) Rebased, looking forward to 2.8!
Code is a little ugly in places, since I was way over my head with GHOST and blocks. Feel free to commandeer.

Thinking about this... Maybe I'm missing something, but can't we just open a regular window and draw into it? Why does it need to be a dialog window from the OS?

Things might get a bit more complicated then since we couldn't use our regular screen/area/region system as it is, but probably it's worth implementing a pretty basic system to draw our own dialog boxes (I'm actually tempted to try ;D).

Julian Eisel (Severin) requested changes to this revision.Jan 27 2016, 5:37 PM

Just requesting changes to indicate that it's not ready for more review yet.

This revision now requires changes to proceed.Jan 27 2016, 5:38 PM
Diego Gangl (januz) edited the summary of this revision. (Show Details)Tue, Jan 9, 10:23 PM

@Julian Eisel (Severin), the issue with using multiple windows is it will push us to have better multi-window handling. Eg, prevent the dialog from going behind the main window. X11 window types for dialog/transient windows... for other platforms too.

Just seems like it will push us in the direction of having overlapping windows which then need to be managed - would rather avoid this if it's not essential.


Of course there is an argument that we *should* have better multi-window handling. Just pointing out this adds work for us - for quite a small gain.

I understand an actual window would be better than the internal popups for this, however the most important part of this patch is preventing Blender from closing with unsaved changes (exiting is aborted if the popup is closed). If putting this in a window is too much work (and complexity), I'd consider staying on the popup route.

Campbell Barton (campbellbarton) requested changes to this revision.Thu, Jan 11, 12:05 AM
Campbell Barton (campbellbarton) added inline comments.
source/blender/windowmanager/intern/wm_operators.c
3131–3143
3150–3164

This duplicates save which I'd rather avoid. Would try do this by calling regular save operator, check it works, then quit.

source/blender/windowmanager/intern/wm_window.c
373–375

Should use BLI_path_basename, can remove wm_path_separator

377–380

Would use single BLI_snprintf call.

432–434

Why is this needed. Shouldn't CTX_wm_window work?

source/blender/windowmanager/wm_window.h
35–37

not needed.

Since this seems to be moving forward i'll pile on from a platform perspective, but the ui team should have the final say on this (so wait for them before changing anything) @Julian Eisel (Severin) *poke*?

arc doesn't seem to want to apply this patch, so i'm working off the screenshot... i'm pretty sure on windows the 'native' dialog would be used so points 1+2 are not an issue, still it might be worth looking into what the ui guidelines for kde/gnome have to say about the subject.

  1. All dialogs with questions on windows have the buttons bottom right aligned in the dialog.
  2. The order of the buttons in any app is always yes/no/cancel (i think this applies to kde as well) people instinctively know the order from muscle memory, I'd really prefer if we'd stick to this.
  3. All applications ask not 'X has unsaved changes, do you really want to quit' but 'do you want to save changes to X? yes/no/cancel' once more muscle memory which I highly suggest we follow.

arc doesn't seem to want to apply this patch, so i'm working off the screenshot...

Sorry, I haven't rebased this patch in 2.5 years :P I'll get to that soon.

still it might be worth looking into what the ui guidelines for kde/gnome have to say about the subject.

KDE's HIG doesn't say much about dialogs. Gnome's mentions that cancel should go first (from the left) and be an imperative verb.

  1. All dialogs with questions on windows have the buttons bottom right aligned in the dialog.
  2. The order of the buttons in any app is always yes/no/cancel (i think this applies to kde as well) people instinctively know the order from muscle memory, I'd really prefer if we'd stick to this.
  3. All applications ask not 'X has unsaved changes, do you really want to quit' but 'do you want to save changes to X? yes/no/cancel' once more muscle memory which I highly suggest we follow.

"Do you want to save changes" sounds way better, however I would keep the buttons' texts as verbs. I think it's an improvement over the old yes/no/cancel (and both KDE and Gnome use them). About order and alignment: Gnome, Kde and Elementary have three different opinions on this. I wouldn't mind aligning them to the right with the KDE order, but I'd rather wait to hear comments from the UI team.

@Campbell Barton (campbellbarton) thanks for the inlines, I'll get to them soon.

Diego Gangl (januz) marked 4 inline comments as done.Sat, Jan 13, 1:27 AM
Diego Gangl (januz) added inline comments.
source/blender/windowmanager/intern/wm_operators.c
3150–3164

The problem with using the regular save op from the block callback is that it returns OPERATOR_RUNNING_MODAL (since it opens the file browser). Is there a way to wait and catch the return value once it's finished?

source/blender/windowmanager/intern/wm_window.c
432–434

Isn't CTX_wm_window a getter? I'm setting a window in the context. I honestly can't remember why this is needed, but without it the popup doesn't show up.

source/blender/windowmanager/intern/wm_operators.c
3150–3164

Using WM_operator_name_call with WM_OP_EXEC_DEFAULT execution context will run without immediately (used by python scripts to save blend files).

source/blender/windowmanager/intern/wm_window.c
432–434

It is a getter, I suppose this is needed because it can be called outside the normal operator event handling loop.

In that case - best just comment why it's needed since AFAIK no other operators do this and it's generally bad practice.

Diego Gangl (januz) marked an inline comment as done.Tue, Jan 16, 12:36 AM
Diego Gangl (januz) added inline comments.
source/blender/windowmanager/intern/wm_operators.c
3150–3164

WM_OP_EXEC_DEFAULT works ok when the user has already saved the blend before, but if the user hasn't saved it before it just writes an "untitled.blend" file and returns immediately instead of showing the file browser (as it does with INVOKE_).

source/blender/windowmanager/intern/wm_operators.c
3150–3164

I see, then I think this could be handled by a macro which runs save and quit operators.

See WM_operatortype_macro_define use.

If there are issues with saved/unsaved files needing different options, two different macros could be used.

  • Rebase on current master
  • Remove unused structs
  • Code style fixes
  • Use BLI_path_basename to get blend filename
  • Remove wm_path_separator()
  • Use BLI_snprintf to create the dialog's text
  • Add explanatory comment on wm_confirm_quit()
  • Replace save_and_quit op with a macro
Diego Gangl (januz) planned changes to this revision.Sat, Jan 20, 11:55 PM

This patch isn't ready yet, but it compiles and sorta works now (feel free to test).

To do:

  • The new save and quit macro shows the filebrowser but doesn't quit after the user saves. Still looking into this
  • Needs confirmation from Windows and Mac devs that this patch isn't changing anything on their platforms
  • The question of alignment and order of the buttons is still open (UI team, assemble!)
  • Lots of whitespaces fixes