Page MenuHome

T37421/T37422 Remove Unnecessary Confirmation Popups
ClosedPublic

Authored by Emanuel Claesson (EClaesson) on Nov 23 2013, 6:46 PM.

Details

Summary

Ref T37421 T37422

Removed several confirmation pop-ups.
There are still a couple of such pop-ups, but some didn't make sense to remove as of now. I.e confirming to start a screencast.

Diff Detail

Event Timeline

I didn't go through them yet to see which ones are appropriate to remove, and would appreciate some feedback from other UI team members here. But for most of them I think we need at least some info message to indicate that something happened (see T37422).

That would be implemented with a line like:

BKE_reportf(op->reports, RPT_INFO, "Deleted %d objects", num_objects);

Other than the confirmation pop-ups removed in this diff, there are:

  • editors/screen/screendump.c : 521
  • editors/space_file/file_ops.c : 1152, 1575
  • editors/space_graph/graph_edit.c : 1080
  • editors/space_text/text_ops.c : 248, 398
  • windowmanager/intern/wm_operators.c : 1938, 1992, 2003, 2016, 2027, 2708

Agreed, info notifications are warranted I think. Aside from that, all the removals look good to me, except maybe the Vertex Parenting. Vertex parenting is a bit of an obscure workflow in my mind, and so I think it needs extra notification to the user on what's happening. It's one place where a pop is good I think. At least for the time being.

Emanuel Claesson (EClaesson) updated this revision to Unknown Object (????).Nov 23 2013, 8:30 PM

All removed pop-ups now gives feedback to the user via BKE_report.
Pop-up put back for vertex parent operations.

Sorry. Missed some build errors.
New diff coming up.

Emanuel Claesson (EClaesson) updated this revision to Unknown Object (????).Nov 23 2013, 9:09 PM

Errors fixed.
A few includes were missing and a few clipboard pastes seems to have snuck in to wierd places.
Sorry about that. Next time i will build BEFORE i submit the patch.

Brecht Van Lommel (brecht) requested changes to this revision.Nov 24 2013, 1:41 PM
Brecht Van Lommel (brecht) added inline comments.
source/blender/editors/animation/keyframing.c
1523

There should only be one info message for the operator I think, not one for each object. As only one message is shown at the time, it would only show one object as modified and that's misleading, so better don't show objects names at all, but rather something like "Keyframes removed from %d objects".

The other things is that this should check that some fcurve was in fact removed for this object.

source/blender/editors/space_clip/clip_graph_ops.c
484

Cases like these should ideally check that something was actually deleted.

source/blender/editors/space_view3d/view3d_ops.c
80

The particular filename that this was saved to is not important, just "Copied selected objects to buffer" should be ok.

Emanuel Claesson (EClaesson) updated this revision to Unknown Object (????).Nov 24 2013, 6:17 PM

Fixed request for changes from brecht.

Emanuel Claesson (EClaesson) updated this revision to Unknown Object (????).Nov 24 2013, 7:23 PM

OBJECT_OT_delete no longer shows "Deleted 0 objects" if nothing was deleted.

The change is fine with me.

Brecht Van Lommel (brecht) requested changes to this revision.Nov 25 2013, 1:17 AM

Some more comments.

Some of the functions like delete_action_keys should be made to return a boolean if they actually modified anything. This involves some work and should really have been done when the operator was initially implemented, but when you're showing messages it becomes more important to do this correct and not show a message when nothing actually changed.

source/blender/editors/animation/keyframing.c
1531

The correct message would be: "Deleted %d animation f-curves from selected objects.", as you aren't counting the objects here.

source/blender/editors/armature/armature_edit.c
1182

Might want to change Removed to Deleted for consistency, although I realize we aren't being very consistent with this terminology, still seems slightly better here.

source/blender/editors/mask/mask_ops.c
1038

Here we should check if anything was actually modified, in fact the calls to BKE_mask_update_display and WM_event_add_notifier can be skipped in that case too, and the operator can return OPERATOR_CANCELLED;

source/blender/editors/space_action/action_edit.c
841

Here we should also check if anything was actually modified.

source/blender/editors/space_clip/tracking_ops.c
353

Should this be "Deleted all selected markers"?

source/blender/editors/space_graph/graph_edit.c
925

Here we should also check if anything was actually modified.

Emanuel Claesson (EClaesson) updated this revision to Unknown Object (????).Nov 25 2013, 4:09 AM

Fixed request for changes from brecht.

Brecht Van Lommel (brecht) requested changes to this revision.Nov 25 2013, 4:24 AM

Thanks, the code looks good to commit, but ED_keyframes_edit.h and maybe other header files seem to be missing from the diff. So if you include those I'll test it out here locally, hopefully find no more issues and commit.

Emanuel Claesson (EClaesson) updated this revision to Unknown Object (????).Nov 25 2013, 4:40 AM

Thanks, looks all fine, will commit tomorrow.