Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add hover highlight to main editor buttons #86378

Merged
merged 1 commit into from
Feb 20, 2024

Conversation

RobProductions
Copy link
Contributor

@RobProductions RobProductions commented Dec 21, 2023

Implements godotengine/godot-proposals#8450

This editor styling change aims to improve usability and accessibility for the most common menu buttons in editor_node.cpp by adding a slight background box when hovered:

Capture2023-12-20.19-13-07.mov

There are still a few places around the editor that I think could get the same treatment but this should tackle the most important buttons (namely the plugin buttons which previously had very light color changes on hover) and I can address those in a future PR if this styling looks good.

The only adjustment which might cause some confusion is that the FlatMenuButton theme presets now use a hover highlight background by default when previously the editor flat buttons did not have this. However, the alternative would be manually adjusting all of the icon buttons individually, so I felt this was preferable to create a smaller PR. Let me know if there's anything else I should account for or explain, thanks!

Update: this should now handle most of the buttons I can think of that share a similar style to the ones in the video. There might be exceptions here but my expectation is that they can be ironed out in future PRs; this can hopefully set a precedent for styling going forward.

@Mickeon
Copy link
Contributor

Mickeon commented Dec 22, 2023

Huh. Never even noticed they didn't exist. I see this as a net positive. How does this look in other themes?

@RobProductions
Copy link
Contributor Author

RobProductions commented Dec 23, 2023

How does this look in other themes?

Okay, this had me scratching my head for hours on end and I went down a rabbit hole to debug this, but here's what happened. I did notice an issue on one of the light themes which I thought I had resolved, but when I checked again after I read your comment there were still some oddities. Probably better to just address it now I thought, but no matter what I tried I couldn't get theme color overrides to work across both light and dark themes for the main "plugin" buttons (to select 2D, 3D, etc.).

That's when I discovered that add_editor_plugin() in editor_node.cpp was only ever getting run when Godot initialized, not when the user selects a new theme. Any theme style/color overrides happening there will only be updated when that function is run again, so the user will have to restart the editor for those changes to take effect. I managed to reduce the impact of that down to just one minor inconsistency: the background color for the hover_pressed stylebox. To reiterate, that color will not be updated until the user restarts the editor, though I believe this is a barely noticeable artifact and outside the scope of this PR to fix. Should also note that I believe this issue exists throughout the editor code (namely whenever a theme override is set within a constructor) and I only discovered it playing around with the colors myself, so I think a future PR should address that instead. I can make a Github issue for this once I track down the other problem cases. The reason this wasn't noticed before was because the plugin buttons didn't use theme overrides before, and now they do in order to make the hover effect consistent.

Anyways, with that out of the way I corrected/simplified the code using the above knowledge and it should now work as expected on all of the available themes:

Capture2023-12-22.23-52-57.mov

And here's another example:

Capture2023-12-22.23-55-48.mov

Again with the only caveat being the hover_pressed background color not updating correctly until an editor restart due to an underlying issue with theme overrides, but if this is too inconvenient I can simply remove that part and we can live without hover highlights on toggled plugin buttons for now :) Let me know if this looks good!

EDIT: Should not have spoken too soon lol I just found this: #11096 and the NOTIFICATION_THEME_CHANGED hook that should allow me to update the hover_pressed background color after it has been initialized. Would also indicate that this is not necessarily an issue so long as other areas throughout the editor code utilize it properly; will have to check this out and investigate further...

@RobProductions
Copy link
Contributor Author

Alright, apologies for unexpected updates to the branch but I believe I've finally sorted everything out now. The NOTIFICATION_THEME_CHANGED (and update_theme() in editor_node.cpp) was hiding in plain sight the entire time and moving my styling changes there allowed me to fix the issue of colors not getting corrected when the theme switched :) For good measure, I also moved the existing styling for the plugin buttons there to avoid future confusion.

Here's a quick demo showing that the colors get updated even after a theme switch:

Capture2023-12-23.14-10-27.mov

The other places where I updated button styling don't need to be updated after a theme switch because the values are constant (transparent), so I left them in the constructor to match the other areas of the editor code. I still think that a few places elsewhere in the code don't use theme_changed as expected and thus have their colors messed up so I will look into making a bug report to point those out.

As for these changes though, everything should work as expected now as far as I can tell with no issues or caveats! Feel free to let me know if I missed anything else or need to address anything further, and thank you again for checking it out!

@RobProductions RobProductions requested a review from a team as a code owner January 12, 2024 23:37
@RobProductions
Copy link
Contributor Author

Update: Upon reflection I was thinking it might be better to have a more comprehensive styling change in one PR as opposed to leaving stuff on the to-do list for godotengine/godot-proposals#8450 , so I altered more buttons to add the hover highlight I showed above. While there's more code changed now, this should result in a more consistent UI from a single commit and we can definitively close the proposal if it's merged since most common buttons should be covered now. I've heard that this is the best way to handle editor UI updates too so that we don't end up with a half-implementation between commits. Here's a quick video to show the new buttons that received the styling update:

Capture2024-01-12.18-19-46.mov

And I rebased the branch for good measure :) as before, let me know if there's anything else I should take a look at, thanks!

@YuriSizov YuriSizov self-requested a review January 13, 2024 11:53
@RobProductions RobProductions requested review from a team as code owners January 25, 2024 00:11
@RobProductions
Copy link
Contributor Author

Rebased to conform to the new editor theme file structure

@RobProductions
Copy link
Contributor Author

Done. The naming scheme stemmed from the old editor_theme.cpp variables and I forgot to update it to match the new structure, thanks for catching that!

@YuriSizov YuriSizov self-requested a review January 26, 2024 10:20
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally, it works as expected. This is a nice usability improvement 🙂

Default

simplescreenrecorder-2024-01-26_16.55.50.mp4

Light

simplescreenrecorder-2024-01-26_16.56.10.mp4

Black (OLED)

simplescreenrecorder-2024-01-26_16.56.36.mp4

@RobProductions
Copy link
Contributor Author

Thanks for the review! I fixed the merge conflict in editor_node.cpp by rebasing and caught another button that was missing the hover styling (onion skinning options in the animation tab)

@AThousandShips
Copy link
Member

Needs another rebase

@RobProductions
Copy link
Contributor Author

Huh and here I thought Github was just slow to update but yeah thank you @AThousandShips it seems another change happened right as I pushed. Just rebased again and gave it a test so hopefully it lasts longer this time :')

@RobProductions
Copy link
Contributor Author

Update: It did not last long. Rebased again to handle the conflict

@Mickeon Mickeon requested a review from Calinou February 2, 2024 22:13
@AThousandShips AThousandShips requested review from a team and removed request for YuriSizov February 12, 2024 18:59
editor/editor_node.cpp Outdated Show resolved Hide resolved
editor/editor_node.cpp Outdated Show resolved Hide resolved
editor/scene_tree_dock.cpp Outdated Show resolved Hide resolved
Updates styling of the editor run bar, plugin, bottom panel, icon buttons, and main menu buttons for accessibility.
@RobProductions
Copy link
Contributor Author

Hi @KoBeWi , I implemented all of your suggestions and rebased again. Thanks for the review!

I had a bit of confusion because I nearly forgot that Button is not flat by default whereas MenuButton is, so I had to leave the setters for those button types. (Perhaps a sign to revisit #72418 ?) Also, I noticed that main_menu in editor_node.cpp already accesses "theme properties" inside the inspector before my commit, so I changed it to use a theme variation instead.

Feel free to take a look at the updated code, appreciate it :)

@KoBeWi
Copy link
Member

KoBeWi commented Feb 16, 2024

I'm not sure about using transparent style for pressed menu.

godot.windows.editor.dev.x86_64_n0qkCX9Oi9.mp4

Most apps seem to use more accented hover style for pressed, e.g. Notepad:

6hseKtcaDY.mp4

Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine code-wise.
Could use a check from @godotengine/usability to make sure the new style is fine and makes sense everywhere it was added (and whether it's missing somewhere).

@RobProductions
Copy link
Contributor Author

I'm not sure about using transparent style for pressed menu.

I would have used the FlatMenuButton's default "pressed" stylebox (which you can see in action on the smaller icon buttons now) but the color blended too much into the background for those top bar items, and it didn't seem consistent with the other drop down menus to use the highlight color. The transparent BG is identical to how it works now in 4.x so hopefully not too jarring.

Could definitely add in the highlight but would have to make sure all the other drop downs match too... probably a task for a future PR then? Thanks for the review :)

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Feb 20, 2024
@akien-mga akien-mga merged commit 9e9dcdb into godotengine:master Feb 20, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants