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.

Sign up for GitHub

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

Jump to bottom

Improve button behavior when multiple mouse buttons are used at the same time #93500

Merged
merged 1 commit into from
Sep 16, 2024
Merged

Improve button behavior when multiple mouse buttons are used at the same time #93500

merged 1 commit into from
Sep 16, 2024

Conversation

anniryynanen
Copy link
Contributor

  • To emit pressed, buttons require that the press was initiated while hovering.
  • Controls can't grab focus from a mouse click if they're not hovered.
  • Hovers are updated both before and after a handled mouse button event.

Fixes #93395.

An updated MRP that makes testing this even easier: button_mrp.zip

@anniryynanen anniryynanen requested review from a team as code owners June 23, 2024 04:44
@anniryynanen
Copy link
Contributor Author

anniryynanen commented Jun 23, 2024 •
edited
Loading

Achievement unlocked: Break first unit test ^^;;

Fixing.

@anniryynanen
Copy link
Contributor Author

I fixed the test failing in Windows, but I can't reproduce the linux Linux failure on Arch Linux #1 SMP PREEMPT_DYNAMIC Sun, 16 Jun 2024 19:06:37 +0000 - Tty - GLES3 (Compatibility) - Mesa Intel(R) Graphics (ADL GT2) - 12th Gen Intel(R) Core(TM) i5-1235U (12 Threads).

I wonder if I could somehow make the segfault happen on my machine... Maybe a VM similar to the cloud build or something? Is there a feasible way to do that?

@AThousandShips AThousandShips modified the milestones: 4.4, 4.3 Jun 23, 2024
KoBeWi
KoBeWi reviewed Jun 26, 2024
View reviewed changes
@anniryynanen anniryynanen requested a review from KoBeWi June 26, 2024 17:20
Calinou
Calinou approved these changes Jun 26, 2024
View reviewed changes
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.

Before

Notice how the hover stylebox lingers until I move the mouse again, even though the mouse is being blocked by the translucent control.

click_hover_before.mp4

After

click_hover_after.mp4

@akien-mga akien-mga modified the milestones: 4.3, 4.4 Jun 28, 2024
@akien-mga
Copy link
Member

I'm keeping this for 4.4 as changes to BaseButton and Viewport are historically prone to regressions, as this impacts a lot of the UI, both for the Godot editor and for games. We definitely need more unit and integration testing there to be able to make such changes with confidence.

So the change is probably good, but it can use a full dev cycle to confirm that, hence 4.4.

kitbdev
kitbdev reviewed Aug 12, 2024
View reviewed changes
@anniryynanen
Copy link
Contributor Author

anniryynanen commented Aug 12, 2024 •
edited
Loading

Thanks for the review! I'll look at this next week, when I'm no longer consumed by Godot Wild Jam.

@anniryynanen
Copy link
Contributor Author

Is there still something needed for this?

Sauermann
Sauermann reviewed Sep 14, 2024
View reviewed changes
scene/gui/base_button.cpp
Comment on lines -177 to -182
Ref<InputEventMouseButton> mouse_button = p_event;
if (mouse_button.is_valid()) {
if (!has_point(mouse_button->get_position())) {
status.hovering = false;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see, how this removal has an impact on the MRPs of the initial issue description and of this PR.

Also wouldn't this removal create for certain projects situations, where the hovering status is kept true, while it should be false?

Perhaps this change could be investigated in a separate PR. The other two modifications are sufficient to resolve this problem in the issue description.

Copy link
Contributor

Choose a reason for hiding this comment

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

See #93500 (comment)
This is because when clicking on a child with mouse filter pass, this can make the hovering state incorrect and then the button can no longer be clicked the second time.
So I think this should still be removed for this PR.

The appearance when pressed and hovering over child is also incorrect, but this can be looked into later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had forgotten about the MOUSE_FILTER_PASS case, I'll re-remove this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation. I agree.

@anniryynanen anniryynanen force-pushed the multi-press branch 2 times, most recently from 5515d42 to d389c88 Compare September 15, 2024 08:42
Sauermann
Sauermann approved these changes Sep 15, 2024
View reviewed changes
Copy link
Contributor

@Sauermann Sauermann left a comment

Choose a reason for hiding this comment

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

Code looks good.
Have tested with both available MRPs and didn't find any obvious problems.

@anniryynanen
Improve button behavior when multiple mouse buttons are used at the s…
2033510 
…ame time

- To emit `pressed`, buttons require that the press was initiated while hovering.
- Controls can't grab focus from a mouse click if they're not hovered.
- Hovers are updated both before and after a handled mouse button event.
@akien-mga akien-mga merged commit ac80ba7 into godotengine:master Sep 16, 2024
20 checks passed
@akien-mga
Copy link
Member

Thanks!

@anniryynanen anniryynanen deleted the multi-press branch September 16, 2024 11:48
@matheusmdx matheusmdx mentioned this pull request Feb 27, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akien-mga akien-mga akien-mga left review comments

@Sauermann Sauermann Sauermann approved these changes

@kitbdev kitbdev kitbdev left review comments

@AThousandShips AThousandShips AThousandShips left review comments

@Calinou Calinou Calinou approved these changes

@KoBeWi KoBeWi Awaiting requested review from KoBeWi

@bruvzg bruvzg Awaiting requested review from bruvzg

Assignees
No one assigned
Labels
bug topic:gui topic:input
Projects
None yet
Milestone
4.4
8 participants
@anniryynanen @akien-mga @Calinou @KoBeWi @Sauermann @kitbdev @AThousandShips @Chaosus