- Group Reviewers
- R106:3a3220d41ac4: Provide option to hide menu bar for Ksysguard
Unit Tests Skipped
Thank you for the patch! I see that this is your first KDE contribution, how exciting! This needs some work, so please don't get discouraged, and I'll help you where I can. Here are the problems that need to be solved in order for the patch to be landable:
- Please see https://community.kde.org/Infrastructure/Phabricator#Formatting_your_patch for information regarding how to format your patch's title and summary sections.
- The patch doesn't apply and I don't see how it could work; all is does is remove a comment. The patch you attached to https://bugs.kde.org/show_bug.cgi?id=395349 is substantially different; can you edit this revision to have that diff instead? To do that, you can click on the Edit Revision link in the top-right corner of the page (it may be under the Action button if your window is narrow.
Thank you, for all the feedback.
Yes, I have to admit the Diff 42047, was the proper patch while Diff 42048 was just an attempt to clean it a little bit.
Sorry, I did a bit of a mess with the diffs and all the tools that are new me.
Here is patch that should work
Thanks! Please also remove - Bug 395349 from the title and replace it with BUG: 395349 in the Summary section.
Now the patch looks better, applies cleanly, and works in my testing. There are just a few whitespace issues for you to address. However--and this is a complaint with other KDE software as well--if you remove the menubar, there is no GUI method to get it back. You have to have already known about the Ctrl+M shortcut, because once the menubar is hidden, there's no way to learn it from ksysguard itself. Different KDE apps handle this in different ways:
- Dolphin puts most of the menubar's functionality under a Control button that appears in the toolbar when the menubar is hidden (not bad)
- Kate displays a dialog warning you and including a reminder about the Ctrl+M shortcut (better than nothing, but nobody will read it or remember the lesson):
- Gwenview does nothing, leading to bug reports: https://bugs.kde.org/show_bug.cgi?id=210620
My worry is that if we implement this patch as-is, with no warning or safety valve or obvious way to restore the menubar or access the lost functionality, we will get bug reports like https://bugs.kde.org/show_bug.cgi?id=210620.
Thoughts on how we can resolve this?
Unrelated whitespace change
|78 ↗||(On Diff #42101)|
Accidental trailing whitespace
Hi Nathaniel, thanks for all the support.
I can understand and agree with your concerns.
Not sure but maybe an option could be to add the hide menu bar entry in the setting menu via kxmlguiwindow and another entry to unhide it maybe can be putted in the context menu on the window title bar.
Just an idea, that requires wider discussions and knowledges.
Ah but won't that menu become invisible once the menubar is disabled?
and another entry to unhide it maybe can be putted in the context menu on the window title bar.
This would require work in the KWin window manager, and I'm not sure if it would be technically possible or desirable.
For now, maybe let's just display a warning like the one Kate shows so we don't get bug reports. It will be slightly annoying, but people who know better can turn it off. You can see how they do it here: https://cgit.kde.org/kate.git/tree/kate/katemainwindow.cpp#n596
(bonus points if you then submit another patch for Gwenview to fix https://bugs.kde.org/show_bug.cgi?id=210620)
Long-term, I would like to see Dolphin's approach with the Control button (or some variant of it) become more common.
Wonderful. I think the warning is good enough for now until we can come up with a better way of handling this. Code looks good! Can you provide your full name and email address so I can land this patch with proper authorship information?
Now that you're an expert on creating warning messages when the menu bar is hidden, would you like to try your hand at doing the same thing for Gwenview? https://bugs.kde.org/show_bug.cgi?id=210620 ;-)
Actually, reading over this again, is it really necessary to add a showMessage parameter to toggleShowMenuBar? In general bool-only arguments are frowned upon because they're not very readable; enums are preferred in their place. But do we even need that parameter in the first place? I don't see that it's ever even set to false anywhere.
@broulik just pointed out that KStandardAction has gained support for the more modern slot syntax.
So, ideally this line should be changed to
mShowMenuBarAction = KStandardAction::showMenubar(this, &TopLevel::toggleShowMenuBar, actionCollection());
Which has the advantage of letting the compiler assert slot compatibility, whereas the old SLOT() syntax turns it into a runtime problem which is easy to miss should it break in the future.
Not technically a blocking issue though.
Awesome job, @lsartorelli. For your next trick, would you like to create KStandardAction::showMenubarWithWarning (or something like that)? This would essentially duplicate the code you've written here, but it would be in a new central location so we could ensure consistency between apps, and make changes in only one place rather than n places. Then we could start to port KSysGuard, Kate, Gwenview to use it--and also adopt it for some of the remaining apps that don't currently show a warning, like Konsole.
The relevant code is in https://cgit.kde.org/kconfigwidgets.git/tree/src/kstandardaction.cpp#n619. Wanna have a go at it?