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

Fix hovering on toggled link and texture buttons #22780

Merged

Conversation

samuelpedrajas
Copy link
Contributor

@samuelpedrajas samuelpedrajas commented Oct 5, 2018

It seems that DRAW_HOVER_PRESSED introduced a small bug in the TextureButton class. When one of these buttons is toggled (toggle mode being active) the button disappears when the mouse is over it.

Steps

  1. Download this project: TextureButtonBug.zip
  2. Play it.
  3. Toggle the button and see how it disappears while the mouse is over it.

I basically copied the code from the DRAW_HOVER case to the DRAW_HOVER_PRESSED one and removed the if (hover.is_null()) conditional. Let me know if this is the right way to do it!

@samuelpedrajas samuelpedrajas force-pushed the fix_texture_button_hovering branch from dc8cc8a to cac1d40 Compare October 5, 2018 22:53
@Piet-G
Copy link
Contributor

Piet-G commented Oct 5, 2018

Wouldn't it be cleaner to move the check for HOVER_PRESSED above the check for PRESSED and just not break?

@samuelpedrajas samuelpedrajas force-pushed the fix_texture_button_hovering branch from cac1d40 to e048086 Compare October 5, 2018 23:06
@samuelpedrajas
Copy link
Contributor Author

Yes you're right @dualmatrix. I just changed my commit.

@Piet-G
Copy link
Contributor

Piet-G commented Oct 5, 2018

So I looked back to #22384 where I added the HOVER_PRESSED mode and I'm unsure this is the best solution. IMO HOVER_PRESSED should't even be called when Pressed automatically implies Hovering like in regular Buttons. I thought this was already the case but apperenlty I implemented it a bit differently. I think I should move the check that tests whether hover_pressed is possible to where the notification in sent ,not where it is recieved. This would be the cleaner solution I think. What do you think?

@samuelpedrajas
Copy link
Contributor Author

I suppose it depends if we want to allow a "hover pressed texture" for TextureButtons in the future (I think it could make sense). If so, I think we can leave it like this. If not, I think you're right, the notification shouldn't even get there. What do you think?

@akien-mga akien-mga added this to the 3.1 milestone Oct 6, 2018
@akien-mga
Copy link
Member

The same bug likely happens in LinkButton where DRAW_HOVER_PRESSED is also discarded.

fix hovering on toggled link buttons
@samuelpedrajas samuelpedrajas force-pushed the fix_texture_button_hovering branch from e048086 to cf2bdcb Compare October 6, 2018 11:07
@samuelpedrajas samuelpedrajas changed the title Fix hovering on toggled texture buttons Fix hovering on toggled link and texture buttons Oct 6, 2018
@samuelpedrajas
Copy link
Contributor Author

samuelpedrajas commented Oct 6, 2018

You're right, I tested it with the "Font Color Hover" and "Font Color Pressed" properties and it is happening. I've updated the PR title and commit (although @dualmatrix may come with a better solution).

@samuelpedrajas
Copy link
Contributor Author

samuelpedrajas commented Oct 6, 2018

If it makes sense (I think it does) to have this "hover pressed" functionality for TextureButton and LinkButton (including being able to select a texture or a font color for it in the inspector) we could merge this as it is right now and open an "enhancement" issue (I can do it myself) about this new functionality. What are your thoughts on this?

@Piet-G
Copy link
Contributor

Piet-G commented Oct 6, 2018

@samuelpedrajas I don't see how HOVER_PRESSED makes sense for regular buttons, since it being pressed automatically means it's being hovered over at the same time right?

@samuelpedrajas
Copy link
Contributor Author

@dualmatrix If toggle mode is active then the button stays pressed even when the mouse is not over it, that's why I thought it was a good idea, but I may be missing something.

@Piet-G
Copy link
Contributor

Piet-G commented Oct 6, 2018

@samuelpedrajas Oh, I wasn't aware of that, in that case the HOVER_PRESSED functionallity should probably be extended to all buttons that offer that mode

@WelpThatWorked
Copy link

Came here from an issue relating to this bug... It solves the bug, but Should there not be an option to add a texture for HOVER_PRESSED, and have it default to the PRESSED texture if not defined?

@Piet-G
Copy link
Contributor

Piet-G commented Oct 22, 2018

@JaLu1 Agreed yeah, thats also what I had in mind

@WelpThatWorked
Copy link

the entire switch statement can be reworked to be more efficient, working on that now

@WelpThatWorked
Copy link

Alright, I rebuilt the structure of the switch statement so it is cleaner and includes support for the extra texture. Can I add to the fork?

@akien-mga akien-mga merged commit aec8ea4 into godotengine:master Nov 1, 2018
@akien-mga
Copy link
Member

I'll merge this for now, if you have a reworked version for the DRAW_HOVER_PRESSED mode, that could go in a new PR for further discussion :)

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.

4 participants