Page MenuHome

UI: Edit Menu - Undo History List and Operator Polling
Needs RevisionPublic

Authored by Harley Acheson (harley) on May 12 2019, 3:39 AM.

Details

Summary

This patch makes small changes to items on the Edit menu.

Undo History List

If you select “Undo History” from the Edit menu you get a list that doesn't read well to those unaccustomed to it. The eyeball icon does not read to me as the Current step in a list of steps. The list could appear to be a number of steps with the current one at top or bottom with one being special in some other way having to do with visibility. It is better to highlight the current step with something that does not carry any other meaning besides simple highlighting.

And the bottom item says "original", which doesn't properly indicate that it is the state before any changes were made. A better description of that item is "Earliest State"

How it looks now is shown on the left, and the right shows how it looks after this patch is applied.

Operator Polling

This patch also adds some better polling (that Enables and Disables the item) for Undo, Redo, Undo History, Repeat Last, Repeat History, & Adjust Last Operation. Note that these poll functions are purposely minimal and quick. Checking only to see if the undo queue is empty, whether there is a "next" after the active step, whether the operators list contains any items, etc. So does not check that an operator is actually repeatable or any more thorough checks that are best left to the exec or invoke. It errs on the side of enabling.

Therefore the app just seems a bit smarter and more responsive to your actions. Starting from scratch these items might all be disabled, but the enable and disable as you use it...

Diff Detail

Repository
rB Blender

Event Timeline

William Reynish (billreynish) accepted this revision.EditedMay 12 2019, 9:15 PM

From a UI point of view, this seems like an obvious improvement to me. Tested and everything seems to be working correctly.

Might want to get a 2nd opinion on the code from @Brecht Van Lommel (brecht), but I have no qualms with these UI changes.

This revision is now accepted and ready to land.May 12 2019, 9:15 PM

@Harley Acheson (harley) Can this list be inverted?
Usually every undo history panel, or any type of history really, it always starts from top to bottom. So the "earliest state" is always at the top, and the most recent entry at the bottom. It seems to be the other way around in blender, which looks weird.

@TheRedWaxPolice (TheRedWaxPolice) - for sure. I just normally avoid changing fundamental things, but that is what I would except as well.

Definitely a good question for the UI team so @William Reynish (billreynish) might chime in. D

I would guess that the original reason for the current order is to make the most recent (and more likely) items closer to the mouse.

What is the reason that the Undo / Redo operator in the menu does not use the pictograms that were designed for them (cells C14 and C15 in the icon set)?

What is the reason that the Undo / Redo operator in the menu does not use the pictograms that were designed for them (cells C14 and C15 in the icon set)?

Good Question and I'm not sure of the answer. Which items on the Edit menu do you think should have icons?

Personally I don't like it when every row has an icons (looking you Help menu!), because when they are sparse they help me find sections, and just the important items, a bit easier. So on the File Menu I like it sparse and would probably remove the "append" and "export" icons if it were up to me. For Edit menu I'd probably add icons for undo and redo, and probably "repeat last" and leave it at that.

@Harley Acheson (harley) Can this list be inverted?

+1

The current order doesn't make sense.

Whether the order stays the same or (better) were reversed, is there an obvious way to indicate the order?

Whether the order stays the same or (better) were reversed, is there an obvious way to indicate the order?

Top to bottom is pretty obvious, everything works like that, and it doesn't even need an indication really. Now, bottom to top shouldn't even be considered imo.

Top to bottom is pretty obvious, everything works like that, and it doesn't even need an indication really.

Seems reasonable.

Changing the order in "Undo History" would imply also changing the order in "Repeat History", wouldn't it? Don't you want the most recent one at the top for that? Asking for a friend... LOL

Changing the order in "Undo History" would imply also changing the order in "Repeat History", wouldn't it? Don't you want the most recent one at the top for that? Asking for a friend... LOL

The rules are the same. Recent entries always goes to the bottom. It's very rare to see otherwise.

In the end everything will be more correct. 👍

Brecht Van Lommel (brecht) requested changes to this revision.May 18 2019, 10:14 PM

Top to bottom makes sense if undo history was a panel. As a menu I doubt it, if you have a very long undo history then the most recent entries will be scrolled off screen or far away from the mouse. There would need to be some solution for that, but I can't think of an elegant one.

source/blender/blenkernel/intern/undo_system.c
340

Earliest State sounds a bit odd to me, it's not bad but I don't remember seeing that terminology in other applications. Is there a more standard name? Original State, Initial State, ..?

Also, this should be IFACE_("Earliest State") so it can be translated.

source/blender/editors/screen/screen_ops.c
3585

Make it static bool.

3654

Make it static bool.

3693

Make it static bool.

source/blender/editors/undo/ed_undo.c
403

Leave out the wm->undo_stack->step_active != wm->undo_stack->step_active->prev test, that can't happen as far as I know. If that would be the case the data structures would be quite broken.

404

This function could be simplified as:

if (!ed_undo_is_init_and_screenactive_poll(C)) {
  return false;
}
UndoStack *undo_stack = CTX_wm_manager(C)->undo_stack;
return ((undo_stack->step_active != NULL) && (undo_stack->step_active->prev != NULL);`
448

Leave out the wm->undo_stack->step_active != wm->undo_stack->step_active->next test.

This revision now requires changes to proceed.May 18 2019, 10:14 PM

Changes to this patch:

I have left the text used as the starting point in Undo History as "Original" or "Original Mode", only changing the case from "original" and "original mode" respectively. I was also not that pleased with "Earliest State" and could find no better alternative. I'd rather keep it as-is for now until something better comes up. I don't like change for the sake of change only.

Corrections as requested by @Brecht Van Lommel (brecht). Some functions were missing "Static" and some polls were simplified with his help. In testing it still works nicely as expected.

This revision is now accepted and ready to land.May 19 2019, 9:52 PM
This revision was automatically updated to reflect the committed changes.

I've reverted the polling in.
rBd525c76003b3: Revert "UI: Edit Menu Operator Polling"

This has implications for scripting which I didn't think of when I approved this, and now is not the right moment to deal with them.

This revision is now accepted and ready to land.Mon, May 27, 12:37 AM
Brecht Van Lommel (brecht) requested changes to this revision.Mon, May 27, 12:38 AM
This revision now requires changes to proceed.Mon, May 27, 12:38 AM

No, I should have caught it in code review. Just better to be conservative at this point in the release cycle.

@Brecht Van Lommel (brecht). No worries. I don't use scripting so didn't think of that. But can imagine that we should do without "ED_operator_screenactive()" in those polls.

But yes, best to deal with it later.