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

Support hiding menubar and add menubarbutton #3189

Closed
wants to merge 22 commits into from

Conversation

poelzi
Copy link
Contributor

@poelzi poelzi commented Oct 19, 2020

Add a new widget MainMenuBarButton that will show the same menu
as the mainmenu bar. Show the button in the skin if the main
menubar is hidden.

DONE: Next step is to add a checkbox in view "[ ] show menubar". The MainMenuBar button is only shown if the main menu is off.

I think this allows the best of 2 worlds, visual impaired or old school friends can stay with the menubar.
Those who want to save the space can have access to the menu with a single button.

Screenshot_20201214_001340

Screenshot_20201214_001406

@poelzi poelzi marked this pull request as draft October 19, 2020 10:06
Copy link
Contributor

@uklotzde uklotzde left a comment

Choose a reason for hiding this comment

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

Thanks for adopting this task! Just some technical comments, I haven't tested it yet.

Please enable all warnings for local development to avoid commits that are rejected by the CI builds.

@@ -60,6 +60,11 @@ WMainMenuBar::WMainMenuBar(QWidget* pParent, UserSettingsPointer pConfig,
}

void WMainMenuBar::initialize() {
WMainMenuBar* target = this;
createMenu([target](QMenu* x) { target->addMenu(x); });
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer a one-line:

createMenu([this](QMenu* pMenu) { addMenu(pMenu); });

src/widget/wmainmenubar.cpp Outdated Show resolved Hide resolved
src/widget/wmainmenubar.h Outdated Show resolved Hide resolved
src/widget/wmainmenubarbutton.h Outdated Show resolved Hide resolved
src/widget/wmainmenubarbutton.cpp Outdated Show resolved Hide resolved
void WMainMenuBarButton::initialize(WMainMenuBar* pMainMenu) {
m_pMenu = new QMenu(this);
setMenu(m_pMenu);
pMainMenu->createMenu([=](QMenu* x) { this->m_pMenu->addMenu(x); });
Copy link
Contributor

Choose a reason for hiding this comment

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

In C++ I would prefer to explicitly capture the context:

pMainMenu->createMenu([this](QMenu* x) { m_pMenu->addMenu(x) });

src/widget/wmainmenubarbutton.h Outdated Show resolved Hide resolved
@poelzi
Copy link
Contributor Author

poelzi commented Oct 19, 2020

@uklotzde Thanks for the technical feedback, but I was more interested if this approach is feasible for everyone, not about the code yet. I know it's unpolished, I just wanted to test if I can get it working as I thought.

@ronso0
Copy link
Member

ronso0 commented Oct 19, 2020

At first start, the visibility controls in the regular View menu didn't work.

@ronso0
Copy link
Member

ronso0 commented Oct 19, 2020

I prefer to continue the discussion in Zulip: Getting rid of the menu bar so we have it in one place.
IME it happens too often that with different PRs touching the same issue information and opinions are scattered and we don't actually move forward as fast as we could.

@poelzi
Copy link
Contributor Author

poelzi commented Oct 20, 2020

I added Deer and LateNight support so far, LateNight could use some more styling.
So far, there is only one bug left I found: fullscreen messes the menus up.

createMenuBar();

Does anyone anything about this ugly workaround ?
Does unsetting and setting the menubar to the mainwindow also fix the problem or does it have to be a newly constructed menubar ?
The reported bug is from mixxx 1.x, I can't imaging ubuntu desktop being broken for so long.

@daschuer
Copy link
Member

I like the solution here, because calling the menu dies not resize the skin.

@ronso0
Copy link
Member

ronso0 commented Oct 20, 2020

Yes, this here is more complete but also more complex. The shortcut Ctrl+7 is a nice addition, as well.

I like the solution here, because calling the menu dies not resize the skin.

@daschuer Both here and in #3184 the skin is resized when toggling the menu bar, or what do you mean?

In #3184 the menus are accessible even when they're not visible, for example Ctrl+F for the File menu, and arrow keys can select other menus.
This PR does not yet have a way to open menus with the keyboard.

Depending on how long the review and testing takes, we could also use #3184 until thi sis finished.

@mixxxdj/developers
Btw, I think both solutions could make it into 2.3 as a nice UI improvement (again, depending on how quick we can test on all OS): finally a clean full screen view!

@ronso0
Copy link
Member

ronso0 commented Oct 20, 2020

Does anyone anything about this ugly workaround ?
Does unsetting and setting the menubar to the mainwindow also fix the problem or does it have to be a newly constructed menubar ?
The reported bug is from mixxx 1.x, I can't imaging ubuntu desktop being broken for so long.

At least on xUbunut 20.04 this hack is not required.
@daschuer Cna you test on your Ubuntu 18.04 machine?

@ronso0
Copy link
Member

ronso0 commented Oct 20, 2020

@poelzi What do you mena by 'messing up the menus'? Works nicely here.

@daschuer

This comment has been minimized.

@daschuer

This comment has been minimized.

@daschuer daschuer changed the base branch from master to main October 21, 2020 11:17
@Holzhaus

This comment has been minimized.

@ronso0
Copy link
Member

ronso0 commented Oct 21, 2020

(I just hid the pre-commit hook posts, please discuss this in #3195 only)

@daschuer
Copy link
Member

All becomes messed up when you switch to full screen and back on Ubuntu Bionic

@daschuer
Copy link
Member

@daschuer Cna you test on your Ubuntu 18.04 machine?

It is not required for Gnome desktops, which have no global menu bar, but it is still required for Unity which features one.

@daschuer
Copy link
Member

With https://extensions.gnome.org/extension/1250/gnome-global-application-menu/ installed on Ubuntu Gnome Mixxx has no menu at all without the hack and a looooong start delay.

Warning [Main]: Failed to unregister window menu, reason: org.freedesktop.DBus.Error.NoReply ("Did not receive a reply. Possible causes include: the remote application did not send a reply, the message bus security policy blocked the reply, the reply timeout expired, or the network connection was broken.")
Debug [Main]: Loading resources from  "/home/daniel/workspace/advanced_autodj_2/

@poelzi
Copy link
Contributor Author

poelzi commented Oct 26, 2020

I did some testing on unity and it turned out, hide() and show() on the menubar is enough, it does not have to be rebuild.

@poelzi
Copy link
Contributor Author

poelzi commented Oct 26, 2020

I will test gnome soon

@poelzi poelzi requested a review from uklotzde November 4, 2020 23:23
@poelzi poelzi marked this pull request as ready for review November 4, 2020 23:40
@poelzi
Copy link
Contributor Author

poelzi commented Nov 4, 2020

@daschuer I tested the new workaround on 18.04 and 19.04 and the menubar now works in fullscreen and switching.
I'm now happy with the functionality.

@poelzi poelzi changed the title [WIP] Support hiding menubar and add menubarbutton Support hiding menubar and add menubarbutton Nov 5, 2020
Run in fullscreen, restart mixxxx. Now mixxx starts in fullscreen
but the menu shows fullscreen not set.
This happens because restore geomentry also restores the fullscreen
flag. Emitting a proper signal in the boot phase fixes the desync.
Loading geometry is not a very safe operation, better skip this
in safe-mode
WMainMenu is now only the holder of the QActions,
Visibility connections and menu logic.
It creates new QMenuBar instances on demand requested by the main window.

Simplify workaround code.
Shade does not support vinyl, so we need a feature flag as well
restore styling from menubar.
@daschuer
Copy link
Member

In full screen mode the menu pops up it the wrong place:

grafik

@daschuer
Copy link
Member

The unity menu integration is now always broken the menu. Is always attached on top of the window.

@daschuer
Copy link
Member

clazy is complaining old style connects.

@daschuer
Copy link
Member

The integration into global menu, at least on linux does not work reliable and I just can't figure out how to do it that, but I guess since most will not use the menubar anyways, I think it makes sense to disable native menubar and make the hamburger menu the default.

This feels like a regression. This is relevant on small scenes, especially because the burger menu is not available with Shade.

The menu integration issue starts once one is trying to switch between menu bar and burger menu. Is there really a use case for this? I guess not so it should be OK to require Mixxx to restart, after the menu bar is hidden.

@github-actions
Copy link

This PR is marked as stale because it has been open 90 days with no activity.

@ronso0
Copy link
Member

ronso0 commented May 23, 2024

I'm closing this for now, many conflicts have emerged.
If anyone wants to pick it up / rebase / overhaul, the branch is still available.

@ronso0 ronso0 closed this May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build skins stale Stale issues that haven't been updated for a long time. ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants