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

Expose a method to get hovered Control in Viewport #85966

Merged
merged 1 commit into from
Dec 18, 2023

Conversation

Kimau
Copy link
Contributor

@Kimau Kimau commented Dec 9, 2023

Found myself needing this to resolve some UI stuff in a clean way.

@Kimau Kimau requested a review from a team as a code owner December 9, 2023 16:40
@AThousandShips AThousandShips added this to the 4.x milestone Dec 9, 2023
@AThousandShips AThousandShips changed the title GDScript Binding for Viewport Mouse Over Value Add GDScript binding for Viewport Mouse Over value Dec 9, 2023
@AThousandShips
Copy link
Member

AThousandShips commented Dec 9, 2023

You need to add documentation for this new method as well, see here

Also I think the name might need a discussion, the current name isn't very clear to me, maybe something gui_get_control_under_mouse, or gui_get_mouse_over_control, or something similar

@Kimau Kimau requested a review from a team as a code owner December 9, 2023 16:56
@Kimau
Copy link
Contributor Author

Kimau commented Dec 9, 2023

I was trying to keep the naming convention as close to the underlying code as possible.
I thought the fact the return type was Control was self documenting enough to show the behaviour.

I have tried to make it clear in the docs.

@AThousandShips
Copy link
Member

The return type isn't always obvious from the name in for example autocomplete, but I think the name works, we'll see what others think 🙂

@Kimau
Copy link
Contributor Author

Kimau commented Dec 9, 2023

Tried to elaborate more in the docs but I did still use the term leaf node and the likely function a user would want to use with it.

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

LGTM, someone with detail knowledge of the mouse over system should look at the docs to check the wording and claims

doc/classes/Viewport.xml Outdated Show resolved Hide resolved
@Kimau
Copy link
Contributor Author

Kimau commented Dec 9, 2023

Maybe control tree or stack is better than group 🤷

@AThousandShips
Copy link
Member

"Subtree" or "hierarchy" might be best, as they are pretty established

@AThousandShips
Copy link
Member

CC @Sauermann If I'm not mistaken you've worked with the viewport input area

@AThousandShips AThousandShips changed the title Add GDScript binding for Viewport Mouse Over value Expose method to get Viewport mouse over Control Dec 9, 2023
@AThousandShips
Copy link
Member

Updated the title as the binding isn't exclusive to GDScript

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.

Implementation looks correct, didn't test it however.

doc/classes/Viewport.xml Outdated Show resolved Hide resolved
doc/classes/Viewport.xml Outdated Show resolved Hide resolved
@YuriSizov
Copy link
Contributor

YuriSizov commented Dec 13, 2023

I was trying to keep the naming convention as close to the underlying code as possible.
I thought the fact the return type was Control was self documenting enough to show the behaviour.

I would say that in this case our internal naming convention is not great and should be adjusted alongside your changes if we seek consistency, or can be left as is to avoid making this PR bigger than it needs to be. But the public method needs to have a subject in its name. Internal properties and methods don't always have good names for public API because they are never considered from this perspective.

So something like gui_get_hovered_control would be ideal for a public method name.


Other than that, this seems fine, it's just exposing an existing value. Once you address the name and squash the commits this can be merged.

@YuriSizov YuriSizov modified the milestones: 4.x, 4.3 Dec 13, 2023
@YuriSizov YuriSizov changed the title Expose method to get Viewport mouse over Control Expose a method to get hovered Control in Viewport Dec 18, 2023
@YuriSizov YuriSizov force-pushed the claire/expose_mouseover branch from 696afb1 to fe77252 Compare December 18, 2023 13:55
@YuriSizov
Copy link
Contributor

Updated and rebased on your behalf.

@YuriSizov YuriSizov merged commit a4d7893 into godotengine:master Dec 18, 2023
15 checks passed
@YuriSizov
Copy link
Contributor

Thanks, and congrats on your first merged Godot contribution!

@Kimau
Copy link
Contributor Author

Kimau commented Dec 21, 2023

Thanks, and congrats on your first merged Godot contribution!

Thanks for making the changes and merging work for busy. Yeah funny this one beat #83138 😂

But yeah this was actually simple 👍

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.

6 participants