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

Show menu bar in full screen #3682

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

KartikSharma0
Copy link

@KartikSharma0 KartikSharma0 commented Jan 1, 2025

Fix: #3647
EDIT: The menu bar will be visible to users in full screen when the cursor hovers near the top of the screen.

  1. Added a mouse movement event to detect changes in the cursor's position.
  2. When the cursor's height (y value) is near 1 (top of the screen) the menu bar becomes visible.

1. Removed hide_menubar function when full screen is toggled on Windows and non-mac systems.
3. Added the show_menubar function in the on_toggle_full_screen function without conditional statement.

Added my name in CONTRIBUTORS as this is my first PR for this project.

@abdnh
Copy link
Collaborator

abdnh commented Jan 7, 2025

I can imagine some people complaining about this change. But right now, the menu bar actually re-appears in full-screen mode when you exit the review screen. Can we maybe fix that and show the menu bar only on hover instead?

@YukiNagat0
Copy link

I can imagine some people complaining about this change. But right now, the menu bar actually re-appears in full-screen mode when you exit the review screen. Can we maybe fix that and show the menu bar only on hover instead?

As discussed in the original issue (#3647 (comment) and subsequent comments), permanently displaying the menu bar is not a valid solution. After further discussion with the issue's author, we agreed that the menu bar should always follow a show on hover behavior, regardless of the Hide top bar setting. This will require refactoring the Hide top/bottom bar settings code to ensure it does not interfere with the menu bar's behavior.

Therefore, the changes introduced in this PR are not aligned with the desired direction.

@KartikSharma0
Copy link
Author

@abdnh @YukiNagat0 I've implemented the change for showing the menu bar when hovering near the top of the screen as discussed.
I also have a question as someone new to the repository: is there a file for declaring constant variables? For example, in my code I check if the y coordinate of the cursor position is less than a value to detect when the cursor is hovering at the top of the screen. I would like to declare the y coordinate value as a constant variable.

@YukiNagat0
Copy link

@KartikSharma0, I apologize for the quibble, but I believe the show on hover behavior is already implemented. However, it only works correctly when the Hide top/bottom bar options are enabled. You can verify this yourself by turning on these options and hovering at the top in full-screen mode from the main menu.
image

The flaw in the current (master branch) implementation is that the menubar (File Edit View ...) and toolbar (Decks Add Browse ...) are treated as a single entity. Specifically, the TopWebView.show/hide methods already include show/hide_menubar calls.

The goal is to decouple the menubar and toolbar behaviors so that the menubar always exhibits the show on hover behavior, regardless of the Hide top bar setting. This can be achieved without introducing new logic (such as checking the cursor position, as suggested in your case).

@KartikSharma0
Copy link
Author

Thanks for the thorough explanation. I will look into this issue from the approach you mentioned.

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
3 participants