godotengine
Improve SpinBox interaction, split arrows, add theme attributes
Discussion here: https://github.com/godotengine/godot-proposals/discussions/9260
This improves the usability & styleability of the SpinBox control.
- Separate up and down buttons, each one having its own icon
- Interaction feedback when hovered, pressed and disabled arrow buttons
- When the value is at the extremity of the allowed range, one of the arrows is disabled
- Stylable arrow buttons & icons, separators, configurable minimum buttons width & spacing with the edit field. Each button support different styles, different backgrounds, icons & tinting (modulate) colors can be set for each interaction state
- The embedded LineEdit receives a theme variation name so that it is easy to style it a different way from the regular LineEdit using a global theme
Care is taken not to break the default theme & existing projects that depend on it.
- Slightly larger default width of the arrow buttons, enough to have a better usability with the mouse without risking breaking themes too much
- The single icon for both arrows is still supported; an empty icon is supplied for it in the proposed default theme change
- The behaviour to size the buttons width according to the button icons always works, although it is now possible to disable it in the theme properties.
With some styling, what is possible. This resembles what I want to achieve in my project:
Big buttons for touch screens
Another possible minimal look with the new separator theme option
Live in Godot in the color selector
Actually, there are two different widgets that render this UI element. The second one is a specialized cell of Tree, and seems to be used inside the inspector and project / editor options to set the various values. This PR only addresses the SpinBox standalone control. To see the result on Godot itself, use the color selector popup anywhere.
All of these are straight upgrades but I'm just ever so slightly concerned by how many theme items have been added by this single PR
IMO This amount of configurability should be the rule for Godot's interactive GUI, not the exception.
It's moreso the fact that some are not even utilized by the editor itself, which I feel like should be the main point of reference to base all of this customization from. For example, the field_and_buttons_separator and up_down_buttons_separator styleboxes. If needed these can also be achieved with a draw_style_box method call.
Okay, that's a fair point... I disagree about basing GUI customization off of the Godot editor, but I agree that these two theme items may be a bit overkill.
The configurability seems a little verbose, I agree. IMHO this is not a problem to have multiple theme attributes. They remain hidden in their own submenu when you do not actively edit those and drawing performance is not affected.
Let's not forget that Godot came from being a Nintendo DS game authoring tool AFAIK. There has been a long way since. I am proposing update to core widgets to add proper support for interaction feedback; the thing that gives that snappy feeling and improves usability.
The pattern to set styles for normal/hover/pressed/disabled/etc... states of a given customizable resource is pretty common to ensure customization of interaction feedback. Those multiple combinations of states and resources make up the large number of theme resources added.
I thought about something to address this verbosity when having those combinations of resources and states, that would be proposed separately. Would be state-aware theme resources for example, like ColorStateLists of the early days of Android, in which one single slot receives the resources that defines the customization for the desired states instead of having as many slots as there are different states. Would allow combining states together, defining only a few of those and inherit the rest automatically... For example, you would be able to define only a normal and pressed style and automatically inherit the normal style on other states instead of having to fill them all. Such a system would be a major change and would need to wait for a major Godot version upgrade where breaking changes can be added.
Regarding the separators, I think each graphic used to render parts of a control should be exposed to theming. For example, there are separators in ItemList but you can only set a color; not a graphic - something to fix in a different PR.
The solution I propose here is usable right now and does not break current projects.
When the value is at the extremity of the allowed range, one of the arrows is disabled
When both are at extremity, both arrows should be disabled:
It works correctly at runtime though.
So I looked at the list of added properties and I agree that there is too many of them. Configurability is nice, but do we even need that much?
Every up and down icon has its stylebox equivalent, so you can basically use styleboxes instead of icons. That's overkill. I'd just add a single stylebox for up/down background. set_min_buttons_width_from_icons can be replaced with buttons_width as I mentioned. Bool constants are awkward and negative width makes the buttons go outside the SpinBox, which might be useful hack, but it can break layout.
I'm not sure about modulates (although in Control context they should be called color), they can be kind of useful if you want to use a single icon, but aside from fast prototyping, there is little advantage over simply using colored icons.
That's what I think at least, the properties are still up to discussion. Keep in mind that it's easier to add a property than to delete it later.
So I looked at the list of added properties and I agree that there is too many of them. Configurability is nice, but do we even need that much? Every up and down icon has its stylebox equivalent, so you can basically use styleboxes instead of icons. That's overkill. I'd just add a single stylebox for up/down background.
With a single background resource used for both the up and down buttons, I would not be able to achieve the styles in my examples. The up button has a corner on the top, while the down button has a corner on the bottom, for example.
So far there is no "stacked" stylebox available. Therefore, in order to draw the arrow icon and the bg rectangle, with the arrow alignment performed by the control, it would require a stylebox and a texture. Being able to draw both the arrow and the entire rectangle of the button is important for some styles and usability.
There is consistency with other buttons in toolbars where you can set the icon and the bg stylebox separately.
It is also much easier to tweak colors if you have the arrow icon and the bg stylebox set as separate elements. Making the same in a one-stylebox operation would only work with StyleBoxTexture, defeating easy tweaking of colors of a StyleBoxRect without providing a new graphic. Customizing the editor theme is a scenario where easy tweaking matters.
set_min_buttons_width_from_iconscan be replaced withbuttons_widthas I mentioned. Bool constants are awkward and negative width makes the buttons go outside the SpinBox, which might be useful hack, but it can break layout.
Yup, there are no native bool types for theme items, I have seen the constant hack used elsewhere and it is not straightforward to understand it at first sight. Just replicated the technique.
This is for a case where you want, at the same time, to have a minimum width AND let the icon grow it larger if necessary. Because, in the proposed change to the default and editor theme, I gave a minimum width to the buttons, so that they are convenient to target with the mouse, and not break the current themes in projects that may rely on the icon alone to widen up the buttons.
Alternatively, it is possible to disable this icon-based-width behavior at all and be able to have smaller buttons than the icon would dictate. Cases where the icon has some white space on the edges or you need to have densely-packed controls in some admin window can benefit of this.
I'm not sure about modulates (although in Control context they should be called
color), they can be kind of useful if you want to use a single icon, but aside from fast prototyping, there is little advantage over simply using colored icons.
If you only want to change the color and not the graphic, can be more straightforward. Tinting icons is used extensively in controls, sometimes with hardcoded values that are not exposed to the theme, for example for the disabled state.
That's what I think at least, the properties are still up to discussion. Keep in mind that it's easier to add a property than to delete it later.
Sure, and I need to make sure not to break existing stuff. More thorough refactorings have to wait a major version upgrade. As I mentioned earlier in the discussion, this significant number of theme properties can be reduced by my idea of state-aware theme resources. For example, you may be able to set a single resource for the icon color, and it would manage on its own the normal/hover/pressed/etc... states without bloating up the properties list.
@davthedev Will you be able to do the style/code quality changes suggested by @KoBeWi?
Poking this PR for the above issues to be solved, as it pretty positively received overall. If you do not have time to develop it further, do say, and someone could take it from here.
Sure, I'll shortly get back to it.
I had to care about my health during the past weeks and had no energy to put on Godot dev for a while; nearly recovered now.
Rebased and code style changes are in - I believe this should be good to merge @KoBeWi @akien-mga
You didn't address all my comments though.
https://github.com/godotengine/godot/pull/89265#issuecomment-2004233077 is still relevant. Also https://github.com/godotengine/godot/pull/89265#discussion_r1528803125
Seems like this breaks EditorSpinSliders. Integer properties no longer show arrows (e.g. frame_coords in Sprite2D):
Correct:
Oops, thanks for pointing out the problem in EditorSpinSliders. Need fix before merge.
For the other comments, I already checked and:
Both arrows are greyed out when min and max are the same when running the project; works correctly. But I did not notice that the issue was when the control is rendered in edit mode. I have not worked with edit-time-specifics yet.
The presence of the "enlarge with icon size" option is explained above as well. To sum it up, here we have 3 cases:
- Fixed size when size is a fixed number, independent of the image width
- Size adjusted to image width when size is -1
- Whichever is greater between the two, similar as if you put a min-width in CSS (when the said option is enabled and size is a fixed number)
I have put a greater minimum button width than the icon itself in the editor & default themes to improve usability. For this purpose, I could have gone with the fixed size option. But this is going to break all projects in which the icon is set to a bigger one. This is why I added the third case.
We can remove the "enlarge with icon size" option, but we have to agree with having this possible project-breaking situation in this case.
I have put a greater minimum button width than the icon itself in the editor & default themes to improve usability.
Ah, I guess that justifies it.
Though it means this property exists only for the sake of compatibility. It should be somehow marked as such (e.g. with a comment, or #ifndef DISABLE_DEPRECATED). I think you can implement the alternate negative behavior anyway, so if the property is removed in the future, the functionality is preserved.
But I did not notice that the issue was when the control is rendered in edit mode. I have not worked with edit-time-specifics yet.
tbh I'm not sure why it works at runtime, but not in the editor, but the issue is still there. It's very minor though, so it does not block merging I think.
For the missing arrows in the EditorSpinSliders, found the culprit. The EditorSpinSlider directly references theme elements from the SpinBox instead of having its own theme variables. I need to make adjustments on that side before we can merge.
The missing arrows in EditorSpinSliders problem is now fixed; OK for merging on my side.