Popup based confirm dialog on quit
AbandonedPublic

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_3
Build Status
Buildable 1301
Build 1301: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes

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)Jan 9 2018, 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.Jan 11 2018, 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.Jan 13 2018, 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.Jan 16 2018, 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.Jan 20 2018, 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

I still haven't been able to make this work through a macro. It fails even when making a macro through Python in a 2.79 build, the exit op is never called after save. However if I change the first op to something non-modal (like opengl rendering), then quit actually gets called. Looking around other macros in code, it seems the modal op is always the last one and not the first.

@Campbell Barton (campbellbarton) looks like this is a limitation of macros, or maybe a bug?

Really great progress. How possible is it to override the "Do you want to override the so and so file" pop up when we save?
Why do we need a confirmation dialogue for saving something we, out of our own will, want? I mean is no accident when someone intuitively
presses "Save" because they are about to close the laptop, or the library is about to close doors, or the bus has just arrived....
Those are some of the common day to day situations in wich a "do you want to overwrite..so and so" dialogue would prevent the entire
chain of events to happen (from saving correctly without damaging the file while laptop is going to sleep, and it won´t because there´s a dialogue waiting for confirmation).

Could we please also do something about the DELETE button dialogue? I´m pretty sure that UNDO is standard by now, and that Blender is more code-stable now
on Undoing things if they are deleted "by accident".

Is it possible to send these as a checkbox somewhere to user preference windows to override and make those "SAVE" and "DELETE" commands execute at once, with no additional dialogues?
Thanks.
Ps: Please don´t delete this post, if it´s misplaced please send it to the appropiate topic post. I apologize for the incovenience.

@Campbell Barton (campbellbarton) thanks for the free rebase/cleanup :)
@David (activemotionpictures) There's some ongoing debate about that at T37422, this patch is for the confirmation dialog on linux only

  • Merge branch 'master' into arcpatch-D566
  • revert changes applied by accident

Looked into making operator macros support this (see patch P607 which does work) however this is a bad hack, the macro system would need to be moved into the operator API for this to work properly - since currently a wrapper operator is used to execute macros - which causes problems with the file selector and is more of a problem to resolve then I'd expected.


For now, suggest to add an exit option to the save operator (instead of having multiple operators).
The option can be hidden from users with PROP_HIDDEN.

Removed the save and quit macro. Added an exit option to the save op.
This is working now!

  • Merge branch 'master' into arcpatch-D566_1
  • Skip-save for quit option
  • Cleanup: use tabs, sync w/ master
Campbell Barton (campbellbarton) requested changes to this revision.EditedFeb 16 2018, 12:43 AM

Looks close to being complete, one issue remains. When the confirm quit preference *isnt* set, there is no longer a confirmation.

source/blender/windowmanager/intern/wm_operators.c
2178

This changes behavior when USER_QUIT_PROMPT isn't set.

A wrapper invoke function for WM_operator_confirm should be used in this case.

This revision now requires changes to proceed.Feb 16 2018, 12:43 AM
Diego Gangl (januz) requested review of this revision.Mar 19 2018, 6:06 PM
Diego Gangl (januz) marked an inline comment as done.

Forgot to submit the inline comment here, that invoke() change was from an earlier revision.

source/blender/windowmanager/intern/wm_operators.c
2178

This isn't in the newer revision AFAIK. That's from when I was trying the macro way or earlier.

Campbell Barton (campbellbarton) requested changes to this revision.EditedMar 20 2018, 7:50 AM

Think this is ready for user-interface review & signoff, @Pablo Vazquez (venomgfx) could you check on this?

Note that there is a remaining bug. If you have multiple windows and press Ctrl-Q. The popup always shows in the first window, no matter which one is currently active.

source/blender/windowmanager/intern/wm_files.c
2079

Not sure about operators having to free themselves, seems like something WM_exit should handle.
OTOH, it's such an exceptional case, I'm not that fussed.

This revision now requires changes to proceed.Mar 20 2018, 7:50 AM
Diego Gangl (januz) marked 2 inline comments as done.
  • Don't free operator
  • Show popup on active window
source/blender/windowmanager/intern/wm_files.c
2079

Yup, looks like it's not needed