Paths

Table of Contentst

Differential D2047

show a confirm menu when unsaved-changes exist during open, open recent, and blend file drop
Needs RevisionPublic
Actions

Authored by David Jeske (jeske) on Jun 2 2016, 12:32 AM.

Details

Reviewers

Sergey Sharybin (sergey)

Maniphest Tasks

T48403: Dragging a .blend file into blender closes current file without save prompt.

Summary

This patch causes blender to display a confirm-menu, much like the current confirm-quit menu, if there are unsaved changes to the blend (but not unpacked images) when the user requests an open, open recent, or drag-drop open operation.

Note: This patch displays the blender style confirm menu on all platforms.

Note: Blender currently does not report image edits to un-packed external images as unsaved changes to wm.file_saved, nor does it save them when you file->save the file.

You can see a brief video of this in operation here...

https://www.youtube.com/watch?v=DxyentHv-rM&feature=youtu.be

how does it work?

The operator main_openfile INVOKE was changed to check for unsaved changes, and if so, present a confirm menu. If the user confirms, the main_openfile operator is re-invoked with a discard_unsaved_changes property set.

Previously, open-recent and a file-drop operation were treated as main_openfile:EXEC. This patch changes them to be INVOKE, which seems reasonable, since they are both user-initiated operations. The new INVOKE code checks to see if a filepath is supplied, and if not, conditionally displays the file chooser.

There are no checks for unsaved changes in EXEC. Existing python scripts which invoke main_openfile:EXEC to open a file will still open immediately, potentially discarding unsaved changes.

Diff Detail

Repository: rB Blender

Event Timeline

David Jeske (jeske) updated this revision to Diff 6841.Jun 2 2016, 12:32 AM

David Jeske (jeske) retitled this revision from to show a confirm menu when unsaved-changes exist during open, open recent, and blend file drop.

David Jeske (jeske) updated this object.

David Jeske (jeske) set the repository for this revision to rB Blender.

David Jeske (jeske) updated this object.Jun 2 2016, 12:35 AM

David Jeske (jeske) mentioned this in T48403: Dragging a .blend file into blender closes current file without save prompt..Jun 2 2016, 12:39 AM

David Jeske (jeske) updated this object.Jun 2 2016, 12:54 AM

David Jeske (jeske) updated this object.

David Jeske (jeske) updated this object.Jun 2 2016, 12:56 AM

David Jeske (jeske) updated this object.Jun 2 2016, 5:37 AM

David Jeske (jeske) updated this object.

i noticed in my new open_mainfile INVOKE code, I was checking only for whether "discard_unsaved_changes" was set at all. In my code it is normally not set the first time, and set to true the second time through the INVOKE. This extends the check to also check it's value, in case some code or script decides to set "discard_unsaved_changes=true" when originally triggering the operator INVOKE.

David Jeske (jeske) set the repository for this revision to rB Blender.Jun 2 2016, 5:52 AM

David Jeske (jeske) added a task: T48403: Dragging a .blend file into blender closes current file without save prompt..Jun 9 2016, 9:25 PM

David Jeske (jeske) updated this object.Jun 9 2016, 9:29 PM

David Jeske (jeske) added a reviewer: Sergey Sharybin (sergey).Jun 9 2016, 9:43 PM

This is more design decision, nevertheless, having a patch is handy to review functionality.

For drag&drop this change makes some sense.

However from the open-recent menu, and open menu, I found it immediately annoying.
This adds an extra step to a deliberate action which already allows you to cancel if you didn't mean it (open recent menu, file selector).

For example, when the open-recent menu is open, The menu title could inform the user that this replaces unsaved work "Open Recent (discard unsaved)", for eg, same with the file selector. (a bit awkward, but this is at least not adding an extra step).

Am wary of making this a preference (suggested in T48403) (even though on face-value it seems reasonable) this could lead to more annoyance when users run Blender with different defaults set.

The patch needed some minor edit to build here, P367.

source/blender/windowmanager/intern/wm_files.c
1535	Typically when invoke can execute immediately, we call the exec function, in this case `wm_open_mainfile_exec`, see P367

Campbell Barton (campbellbarton) added inline comments.Jun 16 2016, 8:15 PM

source/blender/editors/screen/screen_ops.c
4277	If we go ahead with this patch, we need to double check this doesn't break other operators that currently expect EXEC.

(see Ton's comments in the older-info on T48403)

In email, Campbell raised the question of whether this warning should occur only when there is "no chance for the user to cancel". This would be drop and open-recent.

In the open-case, if it is preferred, I could add a visible warning to the file manager if an open action was about to over-write unsaved data. This would prevent the need for the warning confirm dialog, but still warn the user if they were about to potentially lose data.

source/blender/editors/screen/screen_ops.c
4277	How would another operator trigger this? This is in open_file_drop_copy, the function called when a drop operation happens. I don't see anything else that uses open_file_drop_copy except for this line: WM_dropbox_add(lb, "WM_OT_open_mainfile", open_file_drop_poll, open_file_drop_copy); Operators can still EXEC WM_OT_open_mainfile, and in that case it won't do the UI warning for discard unsaved.
source/blender/windowmanager/intern/wm_files.c
1535	okay, no problem. I can update the patch to do this instead.

Campbell Barton (campbellbarton) added inline comments.Jun 17 2016, 8:49 AM

source/blender/editors/screen/screen_ops.c
4277	My mistake, assumed this was for generic drop operation, since its only impacting WM_OT_open_mainfile, its fine.

Campbell Barton (campbellbarton) added inline comments.Jun 17 2016, 8:55 AM

source/blender/windowmanager/intern/wm_files.c
1516	This should return `OPERATOR_CANCELLED`. since this operator isn't running.

In email Campbell brought up the excellent point that in Blender, non-substantive changes, such as just "selecting an object", "rotating the view", or "scrubbing the timeline" technically mark the file unsaved.

Warning the user before they discard changes doesn't make much sense if WRT blender they are always discarding changes.

It seems to me this patch should only warn the user when an open operation would discard a *substantive* change to the blend file. Non-substantive changes, such as "changing selection", "changing view transform", or "scrubbing the timeline" would not trigger this warning.

In order to achieve this, Blender will need to track two different modified flags. The current "wm->file_saved" will remain a true reflection of *any* change to the blend file. A new variable, like "wm->file_saved_mostly" would only be cleared when a *substantive* change to the blend file was made.

I'm going to record Campbell's advice about how to achieve that here until I get around to a new version of this patch:

If we want to have 2 kinds of *modified* settings, we can do this easily enough.
Currently we have ED_undo_push() calling WM_file_tag_modified()

This handles nearly all cases where file-modified tag is set.

For a *very* weak test, we could bypass all undo steps that contain
the word "select".

However to get this working longer term we would want some flag for operators.

So lines like...
    ot->flag = OPTYPE_REGISTER | OPTYPE_UNDO;

Can be made...

    ot->flag = OPTYPE_REGISTER | OPTYPE_UNDO | OPTYPE_NO_FILE_EDIT;

It's quite a kludge, though not sure how else to handle this.

Perhaps we could detect substantive changes through the dependency graph? A call to DAG_id_tag_update is a good indication that some data changed, it could mark the file as modified.

However there are things not covered by this currently. Some operators like grease pencil, physics baking and sculpting don't call this function. For some it would be harmless to add (and useful for scripts to detect changed datablocks anyway). For others we might need to explicitly set the file as modified, but probably that's just a handful of cases?