UI Code Quality: Rename UI_ACTIVE button flag to UI_HOVER #114113

Julian Eisel · 2023-10-24T17:12:25+02:00

Julian Eisel commented

2023-10-24 17:12:25 +02:00

(Note: This PR has been going back and forth between UI_HOVER and UI_HOVER_FOCUSED a bit.)

Calling this state "active" has been confusing for a long time for a number of reasons:

It's not clear that this is essentially a mouse hover state.
Easy to confuse with "select" state (UI_SELECT), both terms are vague.
Buttons can be "inactive" (UI_BUT_INACTIVE) which is totally related to this "active" state.
Button handling can consider a button as active that doesn't have this flag set (e.g. during text input).
Active and selected are well established terms in Blender, but they mean a different thing there.

See #112160.

(Note: This PR has been going back and forth between `UI_HOVER` and `UI_HOVER_FOCUSED` a bit.) Calling this state "active" has been confusing for a long time for a number of reasons: - It's not clear that this is essentially a mouse hover state. - Easy to confuse with "select" state (`UI_SELECT`), both terms are vague. - Buttons can be "inactive" (`UI_BUT_INACTIVE`) which is totally related to this "active" state. - Button handling can consider a button as active that doesn't have this flag set (e.g. during text input). - Active and selected are well established terms in Blender, but they mean a different thing there. See #112160.

❤️ 1

Julian Eisel added the

Module

User Interface

label 2023-10-24 17:12:25 +02:00

Julian Eisel added 1 commit 2023-10-24 17:12:39 +02:00

9e8e03430a UI Code Quality: Rename `UI_ACTIVE` button flag to `UI_HOVER_FOCUS`

Calling this state "Active" has been confusing for a long time, it's not
clear that this is essentially a mouse hover state. It's especially easy
to confuse this with the "Select" state (`UI_SELECT` button flag), which
is used to indicate a pushed button state (and should be renamed too).

The term "Hover Focus" was chosen because it both reflects how it
indicates the hovered state, but also the fact that such buttons can
receive input like shortcut events. A comment is added explaining this
state flag.

See #112160.

Julian Eisel referenced this pull request

2023-10-24 17:29:40 +02:00

UI: Replace UI_ACTIVE with UI_HOVER #112160

Julian Eisel requested review from Campbell Barton 2023-10-24 17:30:03 +02:00

Julian Eisel requested review from Brecht Van Lommel 2023-10-24 17:30:14 +02:00

Julian Eisel commented

2023-10-24 17:35:03 +02:00

Also see #112160 (comment).

Also see https://projects.blender.org/blender/blender/issues/112160#issuecomment-1051916.

Brecht Van Lommel requested changes 2023-10-24 17:58:28 +02:00

Brecht Van Lommel left a comment

UI_BUT_ACTIVE_LEFT and UI_BUT_ACTIVE_RIGHT should be renamed as well?

From what I can tell, this flag was meant only for drawing. Does it really determine which buttons can receive keyboard events?

Edit: I guess the interpretation is that it communicates to the user which button will receive input, not that it does so in the code. Ok.

`UI_BUT_ACTIVE_LEFT` and `UI_BUT_ACTIVE_RIGHT` should be renamed as well? From what I can tell, this flag was meant only for drawing. Does it really determine which buttons can receive keyboard events? Edit: I guess the interpretation is that it communicates to the user which button will receive input, not that it does so in the code. Ok.

Julian Eisel commented

2023-10-24 18:11:35 +02:00

From what I can tell, this flag was meant only for drawing. Does it really determine which buttons can receive keyboard events?

Hmm indeed, it doesn't actually affect behavior much. It is set by handling mostly when activating a button on hover. So maybe UI_HOVER is indeed better. There are still cases where it's set without actually hovering the button, but I think that's fine still and can be noted in a comment. I'd also still note the strong correlation with a "focus" state there, but it's indeed not triggered by the flag.

> From what I can tell, this flag was meant only for drawing. Does it really determine which buttons can receive keyboard events? Hmm indeed, it doesn't actually affect behavior much. It is set by handling mostly when activating a button on hover. So maybe `UI_HOVER` is indeed better. There are still cases where it's set without actually hovering the button, but I think that's fine still and can be noted in a comment. I'd also still note the strong correlation with a "focus" state there, but it's indeed not triggered by the flag.

Julian Eisel added 1 commit 2023-10-24 18:20:09 +02:00

1e71bacfe6 Back to `UI_HOVER`, update comment

Julian Eisel changed title from ~~UI Code Quality: Rename `UI_ACTIVE` button flag to `UI_HOVER_FOCUS`~~ to UI Code Quality: Rename `UI_ACTIVE` button flag to `UI_HOVER`

2023-10-24 18:22:14 +02:00

Julian Eisel commented

2023-10-24 18:29:36 +02:00

Edit: I guess the interpretation is that it communicates to the user which button will receive input, not that it does so in the code. Ok.

Yes but what you say quite clearly shows that this can be misinterpreted then. People might expect they can set the flag to focus a button. So now I do actually prefer UI_HOVER again :)

> Edit: I guess the interpretation is that it communicates to the user which button will receive input, not that it does so in the code. Ok. Yes but what you say quite clearly shows that this can be misinterpreted then. People might expect they can set the flag to focus a button. So now I do actually prefer `UI_HOVER` again :)

Julian Eisel added 1 commit 2023-10-24 18:30:48 +02:00

244c44468c Rename `UI_BUT_ACTIVE_LEFT`/`UI_BUT_ACTIVE_RIGHT` too

Julian Eisel requested review from Brecht Van Lommel 2023-10-24 18:34:31 +02:00

Brecht Van Lommel approved these changes 2023-10-24 18:40:23 +02:00

Campbell Barton approved these changes 2023-10-25 01:16:37 +02:00

Campbell Barton left a comment

Seems fine, although now uiBut::active is less obviously related to UI_HOVER.

We could rename this to uiBut::hover or uiBut::hover_data too.

Seems fine, although now `uiBut::active` is less obviously related to `UI_HOVER`. We could rename this to `uiBut::hover` or `uiBut::hover_data` too.

source/blender/editors/interface/interface_intern.hh

						
				@ -71,3 +71,3 @@

				  /** Temporarily hidden (scrolled out of the view). */

				  UI_SCROLLED = (1 << 1),

				  UI_ACTIVE = (1 << 2),

				  /**

Campbell Barton commented

2023-10-25 01:15:17 +02:00

This comment could be more explicit... e.g.

When enabled, this buttons #uiBut::active is non-null which handles interactions.

Also worth stating....

There should only ever be one UI_HOVER button per #uiBlock.

... this is implied with the term ACTIVE since Blender typically only ever has one active item, it's not so clearly the case once renamed.

This comment could be more explicit... e.g. ``` When enabled, this buttons #uiBut::active is non-null which handles interactions. ``` Also worth stating.... ``` There should only ever be one UI_HOVER button per #uiBlock. ``` ... this is implied with the term `ACTIVE` since Blender typically only ever has one active item, it's not so clearly the case once renamed.

Brecht Van Lommel commented

2023-10-25 12:24:33 +02:00

but->active means more than just hover, for example when tabbing between buttons or opening a menu, we can't really say the mouse is still hovering the button. But the same is true for UI_ACTIVE, so calling that hover is also not super clear.

`but->active` means more than just hover, for example when tabbing between buttons or opening a menu, we can't really say the mouse is still hovering the button. But the same is true for `UI_ACTIVE`, so calling that hover is also not super clear.

Julian Eisel commented

2023-10-25 18:17:58 +02:00

Seems fine, although now uiBut::active is less obviously related to UI_HOVER.

I think that's a good thing. Their relation can be communicated in a comment, but it's good to not confuse them too much, since the flag does not imply the active state, nor the other way around. Further, I think it makes sense to support multiple highlighted elements (list- and view-items do this now, the list/view item is always highlighted on hover, even if the mouse actually hovers a nested button; "extra icons" also have their own hovered state; also think of a button inside a highlight-able panel widget).

It seems good to move the flag towards being a drawing flag only (managed by handling), while "active" is rather independent.

We could rename this to uiBut::hover or uiBut::hover_data too.

I think active makes sense, or uiBut::focused since this will receive input. But there's no strict binding to the hover state, as said above. So I'd find hover too misleading.

> Seems fine, although now `uiBut::active` is less obviously related to `UI_HOVER`. I think that's a good thing. Their relation can be communicated in a comment, but it's good to not confuse them too much, since the flag does not imply the `active` state, nor the other way around. Further, I think it makes sense to support multiple highlighted elements (list- and view-items do this now, the list/view item is always highlighted on hover, even if the mouse actually hovers a nested button; "extra icons" also have their own hovered state; also think of a button inside a highlight-able panel widget). It seems good to move the flag towards being a drawing flag only (managed by handling), while "active" is rather independent. > We could rename this to `uiBut::hover` or `uiBut::hover_data` too. I think active makes sense, or `uiBut::focused` since this will receive input. But there's no strict binding to the hover state, as said above. So I'd find `hover` too misleading.

Julian Eisel added 2 commits 2023-10-25 18:35:57 +02:00

7f33f71186 Merge branch 'main' into temp-rename-uibut-active-flag

5ce98f8323 Improve comments

Julian Eisel merged commit 72a8851a95 into main

2023-10-25 18:36:38 +02:00

Julian Eisel referenced this issue from a commit

2023-10-25 18:36:38 +02:00

UI Code Quality: Rename `UI_ACTIVE` button flag to `UI_HOVER`

Julian Eisel deleted branch temp-rename-uibut-active-flag

2023-10-25 18:36:40 +02:00

Julian Eisel commented

2023-10-25 18:38:13 +02:00

Committed this now. We can further tweak the comments, or even rename this again if we see need to.

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

UI Code Quality: Rename UI_ACTIVE button flag to UI_HOVER #114113

UI Code Quality: Rename `UI_ACTIVE` button flag to `UI_HOVER` #114113