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 more visual feedback for button states #8916

Merged
merged 1 commit into from Oct 12, 2019

Conversation

@Df458
Copy link
Contributor

commented Sep 8, 2019

Closes #8901

This adds extra visual feedback to buttons based on their state:

  • Hovered buttons are lighter
  • Pressed buttons are darker

These parameters are also exposed in styling, so that they can be configured on a per-button basis. Additionally, this PR adds a new optional argument to image_button for specifying a hovered background image, and an associated style parameter as well.

Personally, I think this extra feedback has a noticeable impact on the visual quality of formspecs.

How to test

Although the changes should be immediately noticeable in normal usage, I've also added hover images and hover/pressed colors to the test formspec in 'Minimal Development Test'. (To bring it up, just use the /formspec command)

You can also see an example of a button with a changed background (but default styling for its hovered/pressed state) in the delete world dialog.

Beyond that, further testing can be done by creating various formspec buttons with bgcolor/bgimage styling.

Copy link
Member

left a comment

Looks good apart from this. This won't conflict with other PRs on the formspec merge list.

You need to document the new style properties in lua_api.txt

games/minimal/mods/experimental/init.lua Outdated Show resolved Hide resolved
src/gui/guiFormSpecMenu.cpp Outdated Show resolved Hide resolved
src/gui/guiFormSpecMenu.cpp Outdated Show resolved Hide resolved
src/gui/guiButton.h Outdated Show resolved Hide resolved
@paramat

This comment has been minimized.

Copy link
Member

commented Sep 8, 2019

No objections from me. I was wrong in the issue, most buttons in applications do have this behaviour.

@paramat

This comment has been minimized.

Copy link
Member

commented Sep 8, 2019

Attends to #5810
EDIT: I think that could then be closed.

@TumeniNodes

This comment has been minimized.

Copy link

commented Sep 8, 2019

Along with visual indicators, audio indicators should be considered as well.
For a separate PR but, it needed to be mentioned here.
"Clickity" type button sounds are very easy to produce and or even find on the web, and easily added

@paramat

This comment has been minimized.

Copy link
Member

commented Sep 8, 2019

Visual is the more essential feedback, and adequate, a user may not be using sound.
Buttons making sounds can be a little annoying and unnecessary, most buttons in applications don't make sounds perhaps for these reasons.

@TumeniNodes

This comment has been minimized.

Copy link

commented Sep 8, 2019

But then there are those who do use sound and expect them.
By adding audio indicators, you are not shutting those types of players out, and those who do not use sound will simply use mute, or what ever.
You have to consider all types of end users.
There are methods to separate volume control between the game itself and the UI sounds as well

But I do not want to stray OT much more here, apologies.

@rubenwardy

This comment has been minimized.

Copy link
Member

commented Sep 8, 2019

I would like to add sounds for GUI responses, but I don't see if as urgent or important in priority. It's something that's nice for polish

@TumeniNodes

This comment has been minimized.

Copy link

commented Sep 8, 2019

I would like to add sounds for GUI responses, but I don't see if as urgent or important in priority

I view both, equally important in this aspect. They sort of go hand in hand.

@jastevenson303

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2019

@Df458 Df458 requested review from SmallJoker and rubenwardy Sep 8, 2019
@@ -2171,6 +2171,15 @@ Elements
* `drawborder`: draw button border or not
* `pressed texture name` is the filename of an image on pressed state

### `image_button[<X>,<Y>;<W>,<H>;<texture name>;<name>;<label>;<noclip>;<drawborder>;<pressed texture name>;<hovered texture name>]`

This comment has been minimized.

Copy link
@v-rob

v-rob Sep 9, 2019

Contributor

<hovered texture name> should go in style instead of adding a new part to an element, which is last-resort.

This comment has been minimized.

Copy link
@Df458

Df458 Sep 9, 2019

Author Contributor

I've made the changes you requested. Just to confirm for the future: I assume this means that style properties are the new way to go in general, rather than arguments on the element?

This comment has been minimized.

Copy link
@Df458

Df458 Oct 6, 2019

Author Contributor

Considering @kilbith's use-case, is it worth undoing this change?

src/network/networkprotocol.h Outdated Show resolved Hide resolved
@paramat

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

Also i'm sorry for making an excessive and unjustified fuss in the issue.

@kilbith kilbith referenced this pull request Sep 10, 2019
0 of 5 tasks complete
@Df458

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2019

Now that everything's been resolved, is there any chance of this getting re-reviewed and (hopefully) approved/merged before the feature freeze? I understand you're all probably very busy, but I think it would make for a worthy addition and it shouldn't be hard to merge.

@rubenwardy

This comment has been minimized.

Copy link
Member

commented Sep 15, 2019

Please rebase on top of master to obtain the formspec version code.

git remote add upstream https://github.com/minetest/minetest
git pull --rebase upstream master

You may find it easier to squash first to remove your FORMSPEC_API_VERSION changes

git rebase -i HEAD~8
# (use f to squash and discard messages)

This makes it easier to test this PR, but I don't think this should necessarily depend on formspec versions

@rubenwardy

This comment has been minimized.

Copy link
Member

commented Sep 15, 2019

This appears to remove visual feedback for focused buttons?

@rubenwardy rubenwardy added this to the 5.1.0 milestone Sep 15, 2019
@Df458 Df458 force-pushed the Df458:hover-feedback branch from 4e34b2d to 3e086c6 Sep 15, 2019
@Df458

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2019

This appears to remove visual feedback for focused buttons?

This is true. As far as I can tell, the feedback never existed until now. I'll push a commit to re-enable it, but personally I think the result reads more like a bug than anything else in normal use. If you think it's better this way, I'll squash it in.

@rubenwardy

This comment has been minimized.

Copy link
Member

commented Sep 15, 2019

This is true. As far as I can tell, the feedback never existed until now. I'll push a commit to re-enable it, but personally I think the result reads more like a bug than anything else in normal use. If you think it's better this way, I'll squash it in.

I think the best way to show focus is to draw a dotted border around the button, but if this feedback never existed then I'm fine with this PR not adding it

@Df458

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2019

I agree, that's pretty standard for most UIs. The reason I believe it never existed is that it previously used the hover image state, and that state just skipped straight to EGBIS_IMAGE_UP/EGBIS_IMAGE_DOWN anyway. If no-one else minds, I'll remove the commit and put focus feedback on my personal todo list for now

@Df458 Df458 force-pushed the Df458:hover-feedback branch from 082a10b to 18df838 Sep 21, 2019
Copy link
Member

left a comment

The code works well, but needs trivial changes.

@rubenwardy rubenwardy removed this from the 5.1.0 milestone Sep 23, 2019
@Df458 Df458 force-pushed the Df458:hover-feedback branch from 5154aa1 to 994687a Sep 25, 2019
@SmallJoker SmallJoker added this to the 5.2.0 milestone Sep 29, 2019
@kilbith

This comment has been minimized.

Copy link
Contributor

commented Oct 6, 2019

These features will be in use in craftguide once this PR's merged. ASAP please.

@rubenwardy

This comment has been minimized.

Copy link
Member

commented Oct 6, 2019

This is already approved, and will be merged as one of the first things in 5.2.0-dev

@kilbith

This comment has been minimized.

Copy link
Contributor

commented Oct 6, 2019

Why are image_button and item_image_button not taken in account for visual feedbacks? This is very important for craftguide.

@Df458 Df458 force-pushed the Df458:hover-feedback branch from 994687a to 2be33c5 Oct 6, 2019
@Df458

This comment has been minimized.

Copy link
Contributor Author

commented Oct 6, 2019

Why are image_button and item_image_button not taken in account for visual feedbacks? This is very important for craftguide.

You mean why they don't have parameters for hover feedback? If so, it was originally present but removed by request. However, you can make an image button with hover feedback using a style, take a look at test/formspec.lua in the minimal game.

@kilbith

This comment has been minimized.

Copy link
Contributor

commented Oct 6, 2019

You mean why they don't have parameters for hover feedback?

Yes.

you can make an image button with hover feedback using a style

That won't work with e.g. a nodebox with no inventory_image specified.

So please add this support for both of these elements.

@v-rob

This comment has been minimized.

Copy link
Contributor

commented Oct 7, 2019

By style, he means the style formspec element. Example:

style[one_btn1;bgcolor=red;bgcolor_hovered=orange;bgcolor_pressed=purple]
button[0,0;3,1;btn;Colored Button]

For an image, use bgimg as opposed to bgcolor.

If we added a parameter, it would break compatibility with older clients, and this is why the style element exists.

@SmallJoker SmallJoker added this to Highest priority on top in Formspec PR Priority List [POSSIBLE CONFICTS] Oct 9, 2019
- Add style properties for overriding the the hovered/pressed state
  - By default, hovered buttons are a lighter version of the base color
  - By default, pressed buttons are a darker version of the base color
- Add hovered bg image support for image buttons (style property)
- Update lua_api.txt accordingly
- Update test formspec (/formspec) of the minimal game to showcase button state styling
@Df458 Df458 force-pushed the Df458:hover-feedback branch from 2be33c5 to 3987e17 Oct 12, 2019
@Df458

This comment has been minimized.

Copy link
Contributor Author

commented Oct 12, 2019

Alright, I've rebased on the 5.2.0 commit. A quick test indicates that it still works fine.
Any chance of merging this once the CI checks are complete?

@SmallJoker SmallJoker merged commit 69a2099 into minetest:master Oct 12, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kilbith

This comment has been minimized.

Copy link
Contributor

commented Oct 12, 2019

That honestly blows that we can't do anything on item image buttons. And that the hover/press properties do not apply to an element without border.

This implementation is very incomplete and short-sighted, I'm sorry to say 👎

@Df458

This comment has been minimized.

Copy link
Contributor Author

commented Oct 13, 2019

Item image buttons are a bit of a special case, since the user doesn't directly define their content. In retrospect, borderless item image buttons are a real edge case that was missed and I apologize for missing that in your previous comment (For some reason, I only recall reading image_button at the time). Since there's no good solution from a user perspective (because no background texture is supplied, and changing the texture would make no sense in context), I can add a default behavior to handle this.

And that the hover/press properties do not apply to an element without border.

I'm not sure what you mean here. If you're saying that bgcolor_* doesn't change the background color of a borderless button, then this is an unrelated limitation of formspecs that already existed before this PR. Setting the 'background color' of a button has always required a border to my knowledge, if you want this behavior changed then you should open an issue about it. I assure you, you can change the hovered texture of a borderless button. Just for good measure, I updated your craftguide mod locally to double-check before writing this and I was able to successfully add hover-feedback to the navigation buttons.

"very incomplete and short-sighted" seems like an awfully harsh way to describe a single accidentally-missed case and an unrelated bug not being fixed.

@kilbith

This comment has been minimized.

Copy link
Contributor

commented Oct 13, 2019

Just for good measure, I updated your craftguide mod locally to double-check before writing this and I was able to successfully add hover-feedback to the navigation buttons.

Well, you have to show me because I can't figure out how to do this...

@Df458

This comment has been minimized.

Copy link
Contributor Author

commented Oct 13, 2019

It's about as simple as what @v-rob said above: Make a button with no label, and inject the images in a style element.

Right now, I'm considering our options for handling image_button and item_image_button. The former can be easily replaced with a regular button and style (which is easier to read in code and has better backwards-compatability), but the latter is actually pretty weird--it's actually 2 formspec elements overlayed ontop of each other.

I'm considering a proposal to turn these into subclasses of GUIButton with their image handled as 'content' rather than a 'background'. This is a departure from the current spec, but it would make setting custom background images sensible for both of these and give them a clear purpose in the API. Otherwise, I would be in favor of deprecating image_button and giving borderless item_image_buttons a specific form of highlighting like image-less buttons do.

@SmallJoker SmallJoker removed this from Highest priority on top in Formspec PR Priority List [POSSIBLE CONFICTS] Oct 13, 2019
@Df458 Df458 referenced this pull request Oct 13, 2019
0 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.