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

Added hysteresis for popup sub-menus #41851

Merged
merged 1 commit into from
Nov 16, 2020

Conversation

EricEzaM
Copy link
Contributor

@EricEzaM EricEzaM commented Sep 7, 2020

This adds a small lag effect when opening submenus which allow the user to move directly to an item on the submenu without worrying about avoiding the autohide regions.

I tested with Windows 10 submenu's and seems like their timer is just under 0.5 seconds, so that is what I used. The timer only starts when the user enters an autohide area for the first time. Again, this simulates the behaviour I observed on Windows 10. Please note this is a delay-only based implementation. I am not doing any fancy mouse-movement tracking or prediction - that's overkill imo.

Builds on #41640
Closes #11219
Supersedes/Closes #35487

CC @KoBeWi

Note how in the below GIF I actually enter autohide areas on my way to the submenu, but it does not close.
popup_hysteresis

@EricEzaM EricEzaM force-pushed the PR/popup-menu-hysteresis branch 2 times, most recently from 255813c to f15cbbc Compare September 7, 2020 13:38
@akien-mga akien-mga added this to the 4.0 milestone Sep 7, 2020
@akien-mga akien-mga requested a review from KoBeWi September 10, 2020 09:46
Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

If you are already doing it based on Windows, the options should get highlighted immediately after hover not after timeout. Otherwise they feel unresponsive.

Compare this
QvhBemwIbO

To this
3eZXS0jS8p

Also, this is less important, but double-nested submenus don't work correctly:
6vQsPB2Vqq
(this is the attached project from my old PR)

This is similar to the problem with my old PR which I never fixed. It's probably fine if you don't fix this either (i.e. leave for another PR), as editor doesn't use double-nested popups anyways.

Other than that the implementation is fine. The timeout feels a bit too long, but this might be because I'm using debug build.

scene/gui/popup_menu.cpp Outdated Show resolved Hide resolved
@EricEzaM
Copy link
Contributor Author

@KoBeWi I have fixed the first part. It was a bit of a mess about because of how a submenu would grab focus when it was opened. Now by default it doesn't. It only grabs focus when mouse enters it, which means that the behaviour in your first GIF is fixed. Some adjustments were also made in popup.cpp which defines if the popup should close when the parent is focused. If this was not done then the submenu would just flicker forever as it was activated and then closed due to the mouse being in the parent.

As for the second issue... it is really confusing. Sometimes it happens, sometimes it doesn't. It seems very inconsistent and I cannot determine the cause of why it happens. During debugging it seems that the top level popupmenu gets hide() called on it after the first submenu is hidden where delete_sub_window(window_id) is called in Window::_clear_window. It thinks that for some reason the main window was focused, which hides the main popupmenu. But this only happens when you have a nested submenu at the second level. It does not occur at all if you only have one submenu... really confusing.

I also reduced the delay time to 0.3 seconds, to match the submenu popup timer.

Fix 1:
popupmenu_hysteresis_fix_2

Issue 2, very inconsistent.
popupmenu_hysteresis_fixed

@EricEzaM EricEzaM force-pushed the PR/popup-menu-hysteresis branch from f15cbbc to c5ccb56 Compare September 11, 2020 02:55
@EricEzaM EricEzaM requested a review from KoBeWi September 11, 2020 02:55
@EricEzaM EricEzaM force-pushed the PR/popup-menu-hysteresis branch from c5ccb56 to d471044 Compare September 11, 2020 12:28
Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

The get_close_on_parent_focus() is probably unnecessary as it's never used. Also not sure if you should mix style changes into this PR (the p_over stuff), at least in one commit.

Other than that, this looks fine now.

This adds a small lag effect when opening submenus which allow the user to move directly to an item on the submenu without worrying about avoiding the autohide regions.
@EricEzaM EricEzaM force-pushed the PR/popup-menu-hysteresis branch from d471044 to c482e8e Compare November 8, 2020 03:28
@EricEzaM
Copy link
Contributor Author

EricEzaM commented Nov 8, 2020

Updated. Yes, the get method isn't used but it needs to be there if bound to scripting languages.

@akien-mga akien-mga merged commit 94875f5 into godotengine:master Nov 16, 2020
@akien-mga
Copy link
Member

Thanks!

@EricEzaM EricEzaM deleted the PR/popup-menu-hysteresis branch October 5, 2021 12:24
@Maran23
Copy link
Contributor

Maran23 commented Jan 7, 2023

From my testing it looks like this introduced #70361.
Although even if this is reverted the behaviour still feels a bit sluggish, so I think this just made it more noticable.

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.

Hysteresis (delay) in hierarchical dropdown menus
4 participants