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 position of Tooltips #68627

Merged
merged 1 commit into from
Jan 26, 2023
Merged

Conversation

Sauermann
Copy link
Contributor

@Sauermann Sauermann commented Nov 13, 2022

resolve #65947

CanvasItem::get_screen_transform returns a transform from the CanvasItem to the coordinate system, where a Popup - created as a child of the CanvasItem - should be opened.
CanvasItem::get_screen_transform makes some simplifications, that work well, when used in the editor, but not in general cases.

Since Popups like Tooltips are now used more commonly in projects, it becomes necessary to correct these simplifications.

This solution introduces Viewport::get_popup_base_transform, which makes the necessary calculations.

The next time, user-facing-function-renames are possible CanvasItem::get_screen_transform should be renamed to CanvasItem::get_popup_transform.

MRP: BugPopupTransform.zip

Before and after:

BugHoverTooltip.mp4
SolutionHoverTooltip.mp4

@akien-mga
Copy link
Member

The next time, user-facing-function-renames are possible CanvasItem::get_screen_transform should be renamed to CanvasItem::get_popup_transform.

We can still do renames now if they're important enough. If this helps clean the API and reduce confusion, it's probably best done now than waiting for 5.0.

@Sauermann
Copy link
Contributor Author

Sauermann commented Nov 14, 2022

We can still do renames now if they're important enough. If this helps clean the API and reduce confusion, it's probably best done now than waiting for 5.0.

@akien-mga Confusion might arise between

  • Viewport.get_screen_transform: returns a transform to the actual Window-Manager screen
  • CanvasItem.get_screen_transform: returns a transform to the location of the popup (and is influenced by Viewports and if they are configured to embed windows) and might not even be on the Window-Manager screen

This name-change should include CanvasItem.get_screen_position and CanvasItem.get_screen_rect which both behave like CanvasItem.get_screen_transform.

If there is consensus, that these three name-changes (CanvasItem.get_screen_* to CanvasItem.get_popup_*) are important enough to make it into 4.0, then I would create a PR for that change.

@groud
Copy link
Member

groud commented Jan 18, 2023

Would this PR fix #71327 ?

@Sauermann
Copy link
Contributor Author

@groud I am not sure.
Your linked issue shows a problem with multiple displays and this PR doesn't explicitely adress multi-display-functionality.
At the moment I don't have access to multiple displays in order to give a verified answer.

@clayjohn clayjohn requested a review from reduz January 23, 2023 22:38
groud
groud previously requested changes Jan 26, 2023
Copy link
Member

@groud groud left a comment

Choose a reason for hiding this comment

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

Approved in PR meeting. Needs get_popup_transform to be renamed get_base_popup_transform though.

@akien-mga
Copy link
Member

Approved in PR meeting. Needs get_popup_transform to be renamed get_base_popup_transform though.

Or get_popup_base_transform? :D

CanvasItem::get_screen_transform returns a transform from the CanvasItem
to the coordinate system, where a Popup - created as a child of the
CanvasItem - should be opened.
get_screen_transform makes some simplifications, that work well, when used
in the editor, but not in general cases.

Since Popups like Tooltips are now used more commonly in projects,
it becomes necessary to correct these simplifications.

This solution introduces Viewport::get_popup_base_transform, which makes
the necessary calculations.
@Sauermann Sauermann force-pushed the fix-tooltip-position branch from f6f7082 to c4ed247 Compare January 26, 2023 14:16
@akien-mga akien-mga dismissed groud’s stale review January 26, 2023 14:59

Changes done.

@akien-mga akien-mga merged commit 13e20fe into godotengine:master Jan 26, 2023
@akien-mga
Copy link
Member

Thanks!

@Sauermann Sauermann deleted the fix-tooltip-position branch January 27, 2023 20:07
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.

Tooltip place at wrong place when embed subwindows is enabled
4 participants