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

Fullscreen toggle rework #11566

Merged
merged 5 commits into from
Jan 24, 2024
Merged

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented May 17, 2023

Motivation

While working on #11304 I noticed that my window manager (xfwm4 on Ubuntu Studio) consumes the fullscreen hotkey F11 and MixxxMainWindow::slotViewFullScreen() never gets called, except when I click the menu checkbox.
This happens because the fullscreen hotkey in Mixxx and in my window manager are identical.
(so most of my fullscreen testing was kinda worthless... : \ )

Changes

  • MixxxMainWindow::slotViewFullScreen() does now just toggle fullscreen
  • MixxxMainWindow::eventFilter() catches ALL fullscreen changes (QWindowStateChangeEvent), no matter if triggered by OS or Mixxx, and also emits a signal to recreate & connect the menu bar (only on Linux desktops that use a global menu)

This works perfectly fine for me, and I'm curious how it performs on Windows and macOS. Please test!

TODO

  • ensure smooth startup (fullscreen or not), i.e. no flickering, static launch screen

Testing

  • clicking View > Fullscreen as well as any of the hotkeys printed next to it should toggle fullscreen without any unusual flickering
  • Mixxx should start in fullscreen when using the fullscreen command line arguments --f or --full-screen, regardless the preferences option "Start in fullscreen"
  • fullscreen main window should not flicker if any dialog is openened (About dialog, key wheel window, preferences window etc.)

@github-actions github-actions bot added the ui label May 17, 2023
@ronso0 ronso0 force-pushed the fullscreen-rework_auto-hide branch from 66144f0 to 5cd33ef Compare May 31, 2023 20:50
@github-actions github-actions bot added the build label May 31, 2023
@ronso0 ronso0 changed the base branch from 2.4 to main May 31, 2023 20:51
@ronso0 ronso0 mentioned this pull request Jun 1, 2023
1 task
@ronso0 ronso0 force-pushed the fullscreen-rework_auto-hide branch from 5cd33ef to 62aa802 Compare June 1, 2023 12:32
@github-actions github-actions bot removed the build label Jun 1, 2023
@ronso0
Copy link
Member Author

ronso0 commented Jun 1, 2023

Note: I consider this a bugfix (though we don't know who's affected), and if others agree I can rebase onto 2.4

@daschuer
Copy link
Member

daschuer commented Jun 7, 2023

A conflict has been developed.

Can we add the a "Show Menu bar" entry to the view menu as well? Firefox has it as well.

A difference compared to Firefox is that the file menu opens on Alt. This is convenient on one hand, but surprising.
An issue is that navigating to other menus opens the "Viny Control" submenu before it moves ti the help menu.
This coud be fixed by reordering the sub-menu.

Since I never use the file menu, we may consider to either not open a menu or store the last opened menu and open that.

There is an other issue, when pressing Alt-V to open the View menu and than alt gain to hide the menu bar, the menu bar disappears and reapers shot after. Is there the visible state not stored at the beginning?

@daschuer
Copy link
Member

daschuer commented Jun 7, 2023

I works the same with Qt6

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

It works on Unity as well. There is an issue that Mixxx initially does pops up the auto hide warning which does not apply on such devices with a Global Menu bar. Can we skip it in this case?

There is a conflict with ALT key which is used in Unity used to open the global search. I think we can ignore this issue.

@ronso0
Copy link
Member Author

ronso0 commented Jun 8, 2023

You refer to the changes of #11304 #11526, while this PR is about a consistent internal fullscreen state, and both can be merged independently.

Anyway, moving comments is cumbersome, so let's discuss it here.

Can we add the a "Show Menu bar" entry to the view menu as well? Firefox has it as well.

Sure, though when unticking the checkbox the menubar dialog shold be shown. And in this case

There is an issue that Mixxx initially does pops up the auto hide warning which does not apply on such devices with a Global Menu bar. Can we skip it in this case?

It should be delayed until Mixxx goes fullscreen.

@ronso0
Copy link
Member Author

ronso0 commented Jun 8, 2023

There is an other issue, when pressing Alt-V to open the View menu and than alt gain to hide the menu bar, the menu bar disappears and reapers shot after. Is there the visible state not stored at the beginning?

I'll take a look.

@daschuer
Copy link
Member

daschuer commented Jun 8, 2023

You mean the comments are for #11526
Yes. Let's stick to the original merge plan and merge that first.

@ronso0 ronso0 force-pushed the fullscreen-rework_auto-hide branch 2 times, most recently from 3734e6c to 0c65b7f Compare June 8, 2023 16:38
@ronso0 ronso0 changed the title Fullscreen toggle rework (based on menubar-auto-hide) Fullscreen toggle rework Jun 11, 2023
@ronso0
Copy link
Member Author

ronso0 commented Jun 11, 2023

I rebased this onto 2.4 so it can be merged independent from #11526 just in case any issues arise there. which is a 2.5 candidate.

@ronso0 ronso0 force-pushed the fullscreen-rework_auto-hide branch from 0c65b7f to 6c68ff4 Compare June 11, 2023 22:13
@ronso0 ronso0 changed the base branch from main to 2.4 June 11, 2023 22:14
@ronso0 ronso0 added this to the 2.4.0 milestone Jun 11, 2023
@@ -169,6 +172,7 @@ void MixxxMainWindow::initializeQOpenGL() {
#endif

void MixxxMainWindow::initialize() {
qWarning() << " $ initialize";
Copy link
Member

Choose a reason for hiding this comment

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

Debug leftover? There are more warning that should be removed or become debug messages if it adds any value to the mixxx.log.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the last commit with debug output which should help debugging in case something doesn't work as expected.
I'll remove that in case the other commits are considered okay.

Comment on lines 383 to 384
// If we went fullscreen earlier and the desktop features a global menu
// this will signal the menu to move into the window and hide itself
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand that comment can you rephrase it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I could but I notice it's not required and doesn't belong here. It was required in #11526 at some point for hiding the menubar after starting in fullscreen mode.

Will remove it.

@ronso0 ronso0 force-pushed the fullscreen-rework_auto-hide branch from 6c68ff4 to 2858858 Compare July 2, 2023 22:54
@ronso0 ronso0 changed the base branch from 2.4 to main August 24, 2023 12:03
@ronso0 ronso0 removed this from the 2.4.0 milestone Aug 24, 2023
@ronso0 ronso0 force-pushed the fullscreen-rework_auto-hide branch from 2858858 to 32412a2 Compare August 24, 2023 12:04
@github-actions github-actions bot removed the ui label Aug 24, 2023
@ronso0
Copy link
Member Author

ronso0 commented Aug 24, 2023

I rebased onto main to clean up the 2.4 milestone.
This needs broad testing, so let's target 2.5

Now, slotViewFullScreen() just toggles fullscreen and eventFilter() catches ALL fullscreen
changes, no matter the origin.

Previously, slotViewFullScreen() was also reponsible for recreating and reconnecting the
menu bar and for refreshing the Fullscreen checkbox. This is okay if only Mixxx could
change the fullscreen state.
However, it can happen that the (Linux) window manager consumes the fullscreen hotkey,
makes Mixxx go fullscreen and slotViewFullScreen() is never called.
No-op QWindowStateChangeEvent can occur if another window (preferences, About, ...) is shown above
MixxxMainWindow. The event's oldState() is QWindowNoState even though the window state didn't change.

Ignore these event to not recreate and reconnect the menu bar unnecessarily.
That also avoids flickering, at least on Linux desktops that feature a global menu (when going fullscreen
the menu is moved to the main window and hidden immediately = rapid resizing of window content)
Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

LGTM

What is the status here? Can we just merge it to 2.5 to be part of the upcoming 2.5 beta?

@ronso0
Copy link
Member Author

ronso0 commented Dec 27, 2023

Sure, this is for main/2.5. I'd even argue it's an improvement (fixing a bug in window manager(s)) and could go to 2.4, but I won't fight for that.

We need testing on Windows and macOS @mixxxdj/developers

Testing

  • clicking View > Fullscreen as well as any of the hotkeys printed next to it should toggle fullscreen without any unusual flickering
  • Mixxx should start in fullscreen mode when using the fullscreen command line arguments --f or --full-screen, regardless the preferences option "Start in fullscreen"
  • fullscreen main window should not flicker if any dialog is openened (About dialog, key wheel window, preferences window etc.)

@daschuer daschuer merged commit 59ca3a2 into mixxxdj:main Jan 24, 2024
12 checks passed
@daschuer
Copy link
Member

OK Then lets merge. Thank you.

@ronso0 ronso0 deleted the fullscreen-rework_auto-hide branch January 25, 2024 02:19
@ronso0
Copy link
Member Author

ronso0 commented Jan 25, 2024

Cool.
Might also fix one part of #12640

One issue ...; When I press F11 recently nothing happens.

@ronso0
Copy link
Member Author

ronso0 commented Jan 25, 2024

The DEBUG messages are still in, though I'd say it's a good thing this time : )
Like, if someone runs into issues relevant info is already logged.

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.

2 participants