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 menubar in fullscreen #3710

Merged
merged 7 commits into from
Jan 10, 2025
Merged

Conversation

YukiNagat0
Copy link
Contributor

Fixes: #3647

As alternative to: #3682

Demo:

1.mp4

@YukiNagat0
Copy link
Contributor Author

YukiNagat0 commented Jan 9, 2025

Notes:
The menubar hiding behavior has been implemented with two key considerations:

  1. To prevent flickering, the menubar only hides once when entering the deck browser, maintaining the current master branch behavior.
  2. A fullscreen check was added to _reviewState():

    anki/qt/aqt/main.py

    Lines 796 to 807 in c3a21f0

    def _reviewState(self, oldState: MainWindowState) -> None:
    self.reviewer.show()
    if self.fullscreen:
    self.hide_menubar()
    if self.pm.hide_top_bar():
    self.toolbarWeb.hide_timer.setInterval(500)
    self.toolbarWeb.hide_timer.start()
    else:
    self.toolbarWeb.flatten()

    to prevent a potential menubar softlock on entering reviewer because of 1. . Without this check, the menubar could become stuck until manually hovered over it in reviewer.
    The quirk with this approach is that is causes the menubar to hide instantly when entering review state, bypassing the usual 500ms delay.
    An alternative approach would be to modify the if self.pm.hide_top_bar() check to: if self.fullscreen or self.pm.hide_top_bar(), it will trigger timer which will call hide_menubar in the hide_if_allowed method.
    While this would maintain consistent timing with the default flow (hiding via timer trigger with 500ms delay), it would affect the toolbarWeb.flatten() behavior.
  3. Except small qurik in the 2., this PR preserves original logic and unifies menubar behavior, regardless of the Hide top bar setting.

@YukiNagat0
Copy link
Contributor Author

For more context, please read the original issue #3647 and these comments: #3682 (comment) and #3682 (comment)

@KartikSharma0
Copy link

@YukiNagat0 Just looked over your PR, I now understand what the intended functionality of the original code was. I appreciate your effort in trying to explain it to me though!

@dae
Copy link
Member

dae commented Jan 10, 2025

Thank you both! I find number 1 a bit confusing, but as you said, that seems to be existing behaviour. If @abdnh is happy with this, good to go.

Copy link
Collaborator

@abdnh abdnh left a comment

Choose a reason for hiding this comment

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

Looking good to me.

@dae dae merged commit 9460911 into ankitects:main Jan 10, 2025
1 check passed
@YukiNagat0 YukiNagat0 deleted the fix-menubar-in-fullscreen branch January 10, 2025 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide a way to exit full-screen mode using the mouse
4 participants