Details
- Reviewers
Brecht Van Lommel (brecht) Thomas Dinges (dingto) - Group Reviewers
User Interface - Maniphest Tasks
- T37422: Remove Unnecessary Confirmation Popups
T37421: Remove Unnecessary Confirmation Popups - Commits
- rBS3ca4387bc80b: UI: remove unnecessary confirmation popups
rB3ca4387bc80b: UI: remove unnecessary confirmation popups
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.
All removed pop-ups now gives feedback to the user via BKE_report.
Pop-up put back for vertex parent operations.
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.
| 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. | |
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. | |
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.
Closed by commit rB3ca4387bc80b (authored by @Emanuel Claesson (EClaesson), committed by @Brecht Van Lommel (brecht)).