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

Make PopupMenu hide after its parent window loss focus #98245

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Nikitf777
Copy link

@Nikitf777 Nikitf777 commented Oct 16, 2024

Previously PopupMenus were still on screen after their parent was minimized. It was quite annoying, especially in Editor, because it limits its fps to 1 per second when minimized, even if one of the popup menus is still visible. It's also worth mentioning that no other app I use behaves like this. Their popups always hide after minimizing the main window or they can't just be minimized while popup is visible.

Before

Screencast.from.2024-10-16.21-09-35.mp4

After

Screencast.from.2024-10-16.21-11-24.mp4

I decided to fix this by adding two flags which are also visible from scripts:

  • Window::return_to_transient (default: false)
  • PopupMenu::hide_on_parent_unfocused (default: true)

Also I had to add auxiliary const method Window::get_transient_parent() to be able to use the "focus_exited" signal of the parent window from its child popup.

Previous behavior can be achieved by setting return_to_transient to true and hide_on_parent_unfocused to false.

Three things I'm not sure about are:

  • naming of the PopupMenu::hide_on_parent_unfocused and related stuff (I chose this name because there is Popup::_parent_focused() method.
  • using thread guards in setter and getter of the Window::return_to_transient flag. Honestly, I don't even know what exactly do ERR_MAIN_THREAD_GUARD and ERR_READ_THREAD_GUARD_V macros do
  • Haven't tested on .NET version of Godot.

@Nikitf777 Nikitf777 requested review from a team as code owners October 16, 2024 18:53
@Chaosus Chaosus added this to the 4.4 milestone Oct 17, 2024
@KoBeWi
Copy link
Member

KoBeWi commented Oct 17, 2024

Can't reproduce the bug on Windows.

@Nikitf777
Copy link
Author

Can't reproduce the bug on Windows.

Yep, it seems to be a Linux only issue (and maybe macOS), but it needs to be fixed somehow. Looks like it's due to how different OSes handle popups. And I don't think it'll be changed by them in the future. These are different approaches.

@bruvzg bruvzg self-requested a review October 21, 2024 08:54
Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

  • The issue is not reproducible on Window and macOS, but both work fine with this change.
  • The issue is reproducible and fixed on Linux.

Please update documentation for new functions, see Contributing to the class reference and squash the commits, see Pull request workflow.

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