UI: Show Selected Tool Item #111939

Harley Acheson · 2023-09-04T22:16:07+02:00

Harley Acheson commented

2023-09-04 22:16:07 +02:00

Show Toolbar items with selected highlight and hover highlight.

With prior versions of Blender, opening a list of tools initially highlights the selected item, but then you lose the highlight as soon as you start hovering.

With this PR we get both hover highlight and selected highlight:

Show Toolbar items with selected highlight and hover highlight. --- With prior versions of Blender, opening a list of tools initially highlights the selected item, but then you lose the highlight as soon as you start hovering. ![ToolItems36.gif](/attachments/ed2e8b0f-d539-4c4d-a3f9-d048ad4737a9) With this PR we get both hover highlight and selected highlight: ![ToolItems.gif](/attachments/38675fe1-6117-48ee-9284-ba436064c2f7)

ToolItems36.gif

96 KiB

ToolItems.gif

83 KiB

👍 2

Harley Acheson added 1 commit 2023-09-04 22:16:20 +02:00

2db37e7b6e UI: Show Selected Tool Item

Show Toolbar items with selected highlight and hover highlight.

Harley Acheson added this to the User Interface project 2023-09-04 22:16:45 +02:00

Harley Acheson requested review from Julian Eisel 2023-09-04 22:16:53 +02:00

Harley Acheson added 1 commit 2023-09-05 04:05:40 +02:00

6c739687c0 A version of widget_state_menu_item that works now and later.

Harley Acheson added 1 commit 2023-09-06 19:05:21 +02:00

26195f7998 Merge branch 'main' of projects.blender.org:blender/blender into ToolSelected

Harley Acheson merged commit d99af958d7 into main

2023-09-06 19:07:35 +02:00

Harley Acheson deleted branch ToolSelected

2023-09-06 19:07:36 +02:00

Harley Acheson referenced this issue from a commit

2023-09-06 19:07:37 +02:00

UI: Show Selected Tool Item

Stefan Werner referenced this issue from a commit

2023-09-07 15:02:40 +02:00

UI: Show Selected Tool Item

Julian Eisel commented

2023-09-07 17:13:03 +02:00

This seems like the wrong way to implement this, in fact it doesn't work in many cases. Some problems:

This only works if the menu was opened before in this Blender instance. For example, when opening a file with a tool active that is not first in its menu, the first tool is still highlighted.
Only after changing to a different tool the next time the menu is opened it highlights the correct one. This is because the implementation relies on the feature to remember the last used menu item (static memory).

I would expect this to be closer to layout definition, where we may know about the active item, not in generic popup code
ui_block_func_POPUP is called for many cases, not just the tool menus. So this can have unwanted side-effects in other cases.
ui_but_activate_over() is a general function that clearly states that it activates a button. This implementation also makes it draw a button as selected, which is neither clear from the function name nor from the function documentation (not even the context it's called in). This probably wouldn't always be the desired behavior of the function. It's just an arbitrary location that happens to work for what you want to achieve (although it actually doesn't).
The comment you added only explains what it does (which is pretty obvious from the code), not why it does this. If I see this code I'm confused.

I'd suggest reverting this and working on a proper implementation, especially since this is causing regressions (#112077).

This seems like the wrong way to implement this, in fact it doesn't work in many cases. Some problems: - This only works if the menu was opened before in this Blender instance. For example, when opening a file with a tool active that is not first in its menu, the first tool is still highlighted. ![Screenshot 2023-09-07 at 16.54.52.png](/attachments/7d9a57aa-5723-47c4-a417-3fa663450403)<br/>Only after changing to a different tool the next time the menu is opened it highlights the correct one. This is because the implementation relies on the feature to remember the last used menu item (static memory). <br/><br/>I would expect this to be closer to layout definition, where we may know about the active item, not in generic popup code - `ui_block_func_POPUP` is called for many cases, not just the tool menus. So this can have unwanted side-effects in other cases. - `ui_but_activate_over()` is a general function that clearly states that it activates a button. This implementation also makes it draw a button as selected, which is neither clear from the function name nor from the function documentation (not even the context it's called in). This probably wouldn't always be the desired behavior of the function. It's just an arbitrary location that happens to work for what you want to achieve (although it actually doesn't). - The comment you added only explains what it does (which is pretty obvious from the code), not why it does this. If I see this code I'm confused. I'd suggest reverting this and working on a proper implementation, especially since this is causing regressions (#112077).

Screenshot 2023-09-07 at 16.54.52.png

148 KiB

👍 1

Harley Acheson commented

2023-09-07 20:04:00 +02:00

@JulianEisel - when opening a file with a tool active that is not first in its menu, the first tool is still highlighted.

This specific issue exists before my PR. Following is with 3.6.1:

I'd suggest reverting this and working on a proper implementation, especially since this is causing regressions (#112077).

Regression is fixed. Was a (dumb) lack of a check for the active region being null.

I would expect this to be closer to layout definition, where we may know about the active item, not in generic popup code

Sounds like a better way to do it. Can you give any further pointers for this?

> @JulianEisel - when opening a file with a tool active that is not first in its menu, the first tool is still highlighted. This specific issue exists before my PR. Following is with 3.6.1: ![image](/attachments/d8be3aab-fc27-4047-bb60-25c5bfc516c5) > I'd suggest reverting this and working on a proper implementation, especially since this is causing regressions (#112077). Regression is fixed. Was a (dumb) lack of a check for the active region being null. > I would expect this to be closer to layout definition, where we may know about the active item, not in generic popup code Sounds like a better way to do it. Can you give any further pointers for this?

image.png

33 KiB

Julian Eisel commented

2023-09-07 20:43:19 +02:00

This specific issue exists before my PR. Following is with 3.6.1:

We're only now introducing the concept of properly highlighting the active item, before the highlight was much more loose. It's important that this works reliably now to not mislead users.

Regression is fixed. Was a (dumb) lack of a check for the active region being null.

Problem is we're adding a bunch of fixes on top of wrongly implemented features. At least this makes reverting harder. But worse, once we revert that or implement it properly, it's easy to forget to revert a specific fix, and then they remain in the code and soon nobody remembers what this is for and if it can be removed. I'm trying to keep track of what changes are being done now, but this is far from optimal.

Sounds like a better way to do it. Can you give any further pointers for this?

Not off the top of my head, this would have to be investigated as part of making a proper technical design for this feature.

> This specific issue exists before my PR. Following is with 3.6.1: > > ![image](/attachments/d8be3aab-fc27-4047-bb60-25c5bfc516c5) We're only now introducing the concept of properly highlighting the active item, before the highlight was much more loose. It's important that this works reliably now to not mislead users. > Regression is fixed. Was a (dumb) lack of a check for the active region being null. Problem is we're adding a bunch of fixes on top of wrongly implemented features. At least this makes reverting harder. But worse, once we revert that or implement it properly, it's easy to forget to revert a specific fix, and then they remain in the code and soon nobody remembers what this is for and if it can be removed. I'm trying to keep track of what changes are being done now, but this is far from optimal. > Sounds like a better way to do it. Can you give any further pointers for this? Not off the top of my head, this would have to be investigated as part of making a proper technical design for this feature.

Pratik Borhade referenced this pull request

2023-09-08 05:12:42 +02:00

Wrong tool highlighted #112103

YimingWu referenced this pull request

2024-04-01 10:51:52 +02:00

Wrong theme color assignment #120133

Sign in to join this conversation.

No reviewers