UI: Highlight Selected Enum #111074

Merged
Harley Acheson merged 12 commits from Harley/blender:SelectedEnum into main 2023-09-01 21:33:11 +02:00
Member

This PR highlights the currently-selected item in emum lists.


To show how this aids in consistency, consider the following four UI elements, all side by side. Currently three of those four show the hover highlight in gray, and show the currently-selected item in blue. But pivot point is different: it does not show selected at all and highlights in blue. This patch makes all four consistent:

EnumPivot.gif

Image Format selection:

EnumFormats.gif

Changing editor type:

EnumEditors.gif

This PR highlights the currently-selected item in emum lists. --- To show how this aids in consistency, consider the following four UI elements, all side by side. Currently three of those four show the hover highlight in gray, and show the currently-selected item in blue. But pivot point is different: it does not show selected at all and highlights in blue. This patch makes all four consistent: ![EnumPivot.gif](/attachments/1d21a0c9-cd9a-483b-b73f-3e1d00f74b3c) Image Format selection: ![EnumFormats.gif](/attachments/7f28f6df-7cb9-48f0-a4df-5b19f0fcac35) Changing editor type: ![EnumEditors.gif](/attachments/9514cd7d-a9cf-45c0-b843-38d4dc509ca0)
Harley Acheson added 1 commit 2023-08-12 20:18:51 +02:00
ff0d3a7b22 UI: Highlight Selected Enum
This PR highlights the currently-selected item in emum lists.
Harley Acheson added this to the User Interface project 2023-08-12 20:19:19 +02:00
Harley Acheson requested review from Pablo Vazquez 2023-08-12 20:19:26 +02:00
Member

Glad to see this one coming back!

The only issue I see with this at the moment is that the blue color for active items is not consistent in two cases:

  1. Datablock selectors. They seem to use the hover effect (gray) instead of active (blue).

  2. Shading nodes dropdown are missing the active item highlight. Not a biggie but wonder if it's possible to also have it working there.

Screenshot from 2023-08-17 19-07-38.png


A small note, I think using 0.2f instead of 0.3f might be closer to the other hover effects we have.

Test with 0.2f:

Thanks!

Glad to see this one coming back! The only issue I see with this at the moment is that the blue color for active items is not consistent in two cases: 1. Datablock selectors. They seem to use the hover effect (gray) instead of active (blue). <video src="/attachments/4ea92ace-b1de-4071-afc0-ead4b694e5a5" controls autoplay loop></video> 2. Shading nodes dropdown are missing the active item highlight. Not a biggie but wonder if it's possible to also have it working there. ![Screenshot from 2023-08-17 19-07-38.png](/attachments/3a7cda89-20a0-4f29-bb54-1b66ff78131b) ----- A small note, I think using `0.2f` instead of `0.3f` might be closer to the other hover effects we have. Test with `0.2f`: <video src="/attachments/8d1ce1e6-8336-44f5-a314-6a2e403b9df1" title="2023-08-17_19.04.51.mp4" controls></video> Thanks!
Harley Acheson added 3 commits 2023-08-17 21:59:50 +02:00
Author
Member

@pablovazquez - I think using 0.2f instead of 0.3f might be closer to the other hover effects we have.

I updated this PR for that change. But still looking for the code that affects the other areas not highlighting correctly. Will dive back in and look some more after I catch up with other things.

> @pablovazquez - I think using `0.2f` instead of `0.3f` might be closer to the other hover effects we have. I updated this PR for that change. But still looking for the code that affects the other areas not highlighting correctly. Will dive back in and look some more after I catch up with other things.
Author
Member

@pablovazquez

Only just noticed that UILists have a different selected color, but that is just a theme change. This is currently the only difference between wcol_list_item and wcol_view_item: the selected color. This difference would go away if we amalgamate those two, and make both list types use wcol_list_item then adjust the selected color.

Here is this PR with a UIList with selected color changed from default:

UIListSelected.gif

Here is this PR with a new ListView with default colors:

ListViewSelected.gif

@pablovazquez Only just noticed that UILists have a different selected color, but that is just a theme change. This is currently the only difference between `wcol_list_item` and `wcol_view_item`: the selected color. This difference would go away if we amalgamate those two, and make both list types use `wcol_list_item` then adjust the selected color. Here is this PR with a UIList with selected color changed from default: ![UIListSelected.gif](/attachments/152636e9-fbb1-4dc3-9b72-eb2005f69c74) Here is this PR with a new ListView with default colors: ![ListViewSelected.gif](/attachments/83950299-48ca-4636-b8f4-86c7e8295274)
Member

Great! All for consistency. +1

Great! All for consistency. +1
Contributor

@pablovazquez

Only just noticed that UILists have a different selected color, but that is just a theme change. This is currently the only difference between wcol_list_item and wcol_view_item: the selected color. This difference would go away if we amalgamate those two, and make both list types use wcol_list_item then adjust the selected color.

Here is this PR with a UIList with selected color changed from default:

UIListSelected.gif

Here is this PR with a new ListView with default colors:

ListViewSelected.gif

#111584 Should make it consistent. But the selected color is not currently blue because it clashes with the item name editing mode. If the selection color is blue, then when editing the item name, the cursor and text highlight is invisible, as it has the same blue accent color. So inner_sel property of list_item widget will have be the gray one, not the blue one.

> @pablovazquez > > Only just noticed that UILists have a different selected color, but that is just a theme change. This is currently the only difference between `wcol_list_item` and `wcol_view_item`: the selected color. This difference would go away if we amalgamate those two, and make both list types use `wcol_list_item` then adjust the selected color. > > Here is this PR with a UIList with selected color changed from default: > > ![UIListSelected.gif](/attachments/152636e9-fbb1-4dc3-9b72-eb2005f69c74) > > Here is this PR with a new ListView with default colors: > > ![ListViewSelected.gif](/attachments/83950299-48ca-4636-b8f4-86c7e8295274) > #111584 Should make it consistent. But the selected color is not currently blue because it clashes with the item name editing mode. If the selection color is blue, then when editing the item name, the cursor and text highlight is invisible, as it has the same blue accent color. So inner_sel property of list_item widget will have be the gray one, not the blue one.
First-time contributor

Highlighting selected could be especially useful for Studio shading and matcaps, at the moment it is hard to say which one is enabled.
image

Highlighting selected could be especially useful for Studio shading and matcaps, at the moment it is hard to say which one is enabled. ![image](/attachments/f304bfd6-6219-40cf-96c0-076badb9994e)
100 KiB
Harley Acheson added 4 commits 2023-08-30 21:09:29 +02:00
Author
Member

@1D_Inc - Highlighting selected could be especially useful for Studio shading and matcaps

Thanks for pointing that out. Fixed a mistake that shows those now:

image

> @1D_Inc - Highlighting selected could be especially useful for Studio shading and matcaps Thanks for pointing that out. Fixed a mistake that shows those now: ![image](/attachments/2f3fbe89-d4bd-4be5-931c-2e86bd2f665d)
114 KiB
Member

Testing the latest version. Love the MatCaps highlight!

On pulldown menus, having checkboxes highlighted is a bit jarring:

pulldowns

And also sub-menus probably shouldn't be highlighted as well. Especially with the slight delay on open it doesn't work well, in the video below I'm not clicking on the sub-menus, just hovering:

Testing the latest version. Love the MatCaps highlight! On pulldown menus, having checkboxes highlighted is a bit jarring: ![pulldowns](/attachments/1b8a8992-b77e-4257-93e4-51047ae44812) And also sub-menus probably shouldn't be highlighted as well. Especially with the slight delay on open it doesn't work well, in the video below I'm not clicking on the sub-menus, just hovering: <video src="/attachments/1d318f91-2c33-42f5-a3ab-49f18ab4e8a4" title="submenu_highlight.mp4" controls></video>
Author
Member

Love the MatCaps highlight! ...pulldown menus, having checkboxes highlighted is a bit jarring:

Oh sorry about that, I didn't notice that those are the same things, both menu item for some reason. Will have to figure something out there.

> Love the MatCaps highlight! ...pulldown menus, having checkboxes highlighted is a bit jarring: Oh sorry about that, I didn't notice that those are the same things, both menu item for some reason. Will have to figure something out there.
Harley Acheson added 2 commits 2023-08-31 21:54:48 +02:00
Author
Member

@pablovazquez

Fixed. We are back to having the exact same pulldown behavior as previous versions, but still with the added highlighting of the selected MatCap. In a nutshell the MatCap items are just menu items, so had to deal with those more specifically by checking that their flags include UI_BUT_ICON_PREVIEW.

@pablovazquez Fixed. We are back to having the exact same pulldown behavior as previous versions, but still with the added highlighting of the selected MatCap. In a nutshell the MatCap items are just menu items, so had to deal with those more specifically by checking that their flags include UI_BUT_ICON_PREVIEW.
Pablo Vazquez approved these changes 2023-09-01 12:15:12 +02:00
Pablo Vazquez left a comment
Member

Thanks! Works as expected now. 🎉

Thanks! Works as expected now. 🎉
Harley Acheson added 2 commits 2023-09-01 20:28:58 +02:00
Author
Member

@blender-bot build

@blender-bot build
Harley Acheson merged commit 9f4b28bba8 into main 2023-09-01 21:33:11 +02:00
Harley Acheson deleted branch SelectedEnum 2023-09-01 21:33:12 +02:00
Member

While I'm in general in support of the idea behind this change, I have some concerns with the state this landed in.

On the code side:

  • Why does this set the UI_BUT_LIST_ITEM for buttons that have nothing to do with list items? Almost all buttons in RNA menus in fact. This is abusing a flag that clearly has a completely different purpose, which can lead to unexpected consequences in the future.
  • Why does this use the UI_BUT_ACTIVE_DEFAULT flag? Same here, this flag has a different purpose. It doesn't only affect drawing but also handling. For example I cannot open an RNA menu anymore and use arrow keys + enter to activate a different item. Enter will always reactivate the already active (blue highlighted) item.

This should have gone through code review. Luckily I happened to look into the committed code.

On the UI design side:

  • This only actually works in limited cases (RNA enums). When this was initially proposed a while ago it was pointed out that making this consistent across Blender was a priority. Are there still plans to address this?
    • Doesn't work with ID selectors, they use the hover highlight color to display the active item now:
    • Doesn't work at all in plenty of common cases:
    • Doesn't work well with tools (active tool isn't highlighted in blue within the menu anymore): Screenshot from 2023-09-04 17-15-15.png
  • When activating an item I know get to active highlights while holding the mouse button down, this feels quite unpolished to me:
  • Keyboard navigation with Enter to confirm is broken, as noted above.

Sorry for the negativity here. But if I look at this from a user/product experience point of view, this just feels very unpolished with obvious consistency problems.

While I'm in general in support of the idea behind this change, I have some concerns with the state this landed in. **On the code side:** - Why does this set the `UI_BUT_LIST_ITEM` for buttons that have nothing to do with list items? Almost all buttons in RNA menus in fact. This is abusing a flag that clearly has a completely different purpose, which can lead to unexpected consequences in the future. - Why does this use the `UI_BUT_ACTIVE_DEFAULT` flag? Same here, this flag has a different purpose. It doesn't only affect drawing but also handling. For example I cannot open an RNA menu anymore and use arrow keys + enter to activate a different item. Enter will always reactivate the already active (blue highlighted) item. This should have gone through code review. Luckily I happened to look into the committed code. **On the UI design side:** - This only actually works in limited cases (RNA enums). When this was initially proposed a while ago it was pointed out that making this consistent across Blender was a priority. Are there still plans to address this? - Doesn't work with ID selectors, they use the hover highlight color to display the active item now: <img src="/attachments/1d85873f-997d-4937-b728-3fd633c330f6" width="300px"/> - Doesn't work at all in plenty of common cases: <br/><img src="/attachments/c93beba0-ebb3-407a-ab6a-11c2c128a3ed" width="300px"/> <img src="/attachments/13a12eea-2528-4ef6-b89d-84ecba3313f7" width="300px"/> <img src="/attachments/07ecdc76-1a85-4294-9ead-2076b878fcc4" width="200px"/> - Doesn't work well with tools (active tool isn't highlighted in blue within the menu anymore): ![Screenshot from 2023-09-04 17-15-15.png](/attachments/6a110383-b219-41fb-8819-10c71c2f60c2) - When activating an item I know get to active highlights while holding the mouse button down, this feels quite unpolished to me: <img src="attachments/0774b682-f748-42ec-ab40-3c1d95ded0a7" width="300px"/> - Keyboard navigation with Enter to confirm is broken, as noted above. Sorry for the negativity here. But if I look at this from a user/product experience point of view, this just feels very unpolished with obvious consistency problems.
Author
Member

@JulianEisel - Sorry for the negativity here

No worries! Let's fix it.

Why does this set the UI_BUT_LIST_ITEM for buttons that have nothing to do with list items?
Why does this use the UI_BUT_ACTIVE_DEFAULT flag?

AFAICT, that UI_BUT_LIST_ITEM was used to differentiate from dropdown menu behavior, but doesn't seem needed anymore. UI_BUT_ACTIVE_DEFAULT seemed like a fairly close fit. We could get rid of that UI_BUT_LIST_ITEM usage AND get swap the use of UI_BUT_ACTIVE_DEFAULT with UI_SELECT_DRAW, which seems an even closer fit - "Display selected, doesn't impact interaction"

diff --git a/source/blender/editors/interface/interface.cc b/source/blender/editors/interface/interface.cc
index ed4cb54e6ce..4a48f587e12 100644
--- a/source/blender/editors/interface/interface.cc
+++ b/source/blender/editors/interface/interface.cc
@@ -4470,9 +4470,8 @@ static void ui_def_but_rna__menu(bContext * /*C*/, uiLayout *layout, void *but_p
                              -1,
                              item->description);
       }
-      item_but->flag |= UI_BUT_LIST_ITEM;
       if (item->value == current_value) {
-        item_but->flag |= UI_BUT_ACTIVE_DEFAULT;
+        item_but->flag |= UI_SELECT_DRAW;
       }
     }
   }
diff --git a/source/blender/editors/interface/interface_widgets.cc b/source/blender/editors/interface/interface_widgets.cc
index 05b4bd0eee2..e9a1d7c05f6 100644
--- a/source/blender/editors/interface/interface_widgets.cc
+++ b/source/blender/editors/interface/interface_widgets.cc
@@ -2737,8 +2737,7 @@ static void widget_state_menu_item(uiWidgetType *wt,
     /* Regular disabled. */
     color_blend_v3_v3(wt->wcol.text, wt->wcol.inner, 0.5f);
   }
-  else if ((state->but_flag & UI_BUT_LIST_ITEM) &&
-           state->but_flag & (UI_BUT_ACTIVE_DEFAULT | UI_SELECT))
+  else if (state->but_flag & UI_SELECT_DRAW)
   {
     /* Currently-selected list item. */
     copy_v4_v4_uchar(wt->wcol.inner, wt->wcol.inner_sel);

When activating an item I know get to active highlights while holding the mouse button down

This would be fixed with the above change.

Doesn't work well with tools (active tool isn't highlighted in blue within the menu anymore):

This I might need some help with. I can get this work really nicely, with separate hover from selection (so better than now).

ToolItems.gif

But only by adding UI_SELECT_DRAW inside ui_but_activate_over.

I can put these two things into separate PRs for you to review?

> @JulianEisel - Sorry for the negativity here No worries! Let's fix it. > Why does this set the `UI_BUT_LIST_ITEM` for buttons that have nothing to do with list items? > Why does this use the `UI_BUT_ACTIVE_DEFAULT` flag? AFAICT, that UI_BUT_LIST_ITEM was used to differentiate from dropdown menu behavior, but doesn't seem needed anymore. UI_BUT_ACTIVE_DEFAULT seemed like a _fairly_ close fit. We could get rid of that UI_BUT_LIST_ITEM usage AND get swap the use of UI_BUT_ACTIVE_DEFAULT with UI_SELECT_DRAW, which seems an even closer fit - "Display selected, doesn't impact interaction" ``` diff --git a/source/blender/editors/interface/interface.cc b/source/blender/editors/interface/interface.cc index ed4cb54e6ce..4a48f587e12 100644 --- a/source/blender/editors/interface/interface.cc +++ b/source/blender/editors/interface/interface.cc @@ -4470,9 +4470,8 @@ static void ui_def_but_rna__menu(bContext * /*C*/, uiLayout *layout, void *but_p -1, item->description); } - item_but->flag |= UI_BUT_LIST_ITEM; if (item->value == current_value) { - item_but->flag |= UI_BUT_ACTIVE_DEFAULT; + item_but->flag |= UI_SELECT_DRAW; } } } diff --git a/source/blender/editors/interface/interface_widgets.cc b/source/blender/editors/interface/interface_widgets.cc index 05b4bd0eee2..e9a1d7c05f6 100644 --- a/source/blender/editors/interface/interface_widgets.cc +++ b/source/blender/editors/interface/interface_widgets.cc @@ -2737,8 +2737,7 @@ static void widget_state_menu_item(uiWidgetType *wt, /* Regular disabled. */ color_blend_v3_v3(wt->wcol.text, wt->wcol.inner, 0.5f); } - else if ((state->but_flag & UI_BUT_LIST_ITEM) && - state->but_flag & (UI_BUT_ACTIVE_DEFAULT | UI_SELECT)) + else if (state->but_flag & UI_SELECT_DRAW) { /* Currently-selected list item. */ copy_v4_v4_uchar(wt->wcol.inner, wt->wcol.inner_sel); ``` > When activating an item I know get to active highlights while holding the mouse button down This would be fixed with the above change. > Doesn't work well with tools (active tool isn't highlighted in blue within the menu anymore): This I might need some help with. I can get this work really nicely, with separate hover from selection (so better than now). ![ToolItems.gif](/attachments/81a47e0a-3057-41d0-898a-a180c24f1c8d) But only by adding UI_SELECT_DRAW inside `ui_but_activate_over`. I can put these two things into separate PRs for you to review?
Contributor

Pre-highlight! Marvelous.

Pre-highlight! Marvelous.
Sign in to join this conversation.
No reviewers
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
6 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#111074
No description provided.