Provide option to hide menu bar for Ksysguard
ClosedPublic

Authored by lsartorelli on Sep 21 2018, 12:17 PM.

Details

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
lsartorelli created this revision.Sep 21 2018, 12:17 PM
Restricted Application added a project: Plasma. ยท View Herald TranscriptSep 21 2018, 12:17 PM
Restricted Application added a subscriber: plasma-devel. ยท View Herald Transcript
lsartorelli requested review of this revision.Sep 21 2018, 12:17 PM
ngraham requested changes to this revision.Sep 21 2018, 2:28 PM
ngraham added reviewers: Plasma, Frameworks.
ngraham added a subscriber: ngraham.

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:

  1. Please see https://community.kde.org/Infrastructure/Phabricator#Formatting_your_patch for information regarding how to format your patch's title and summary sections.
  2. 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.
This revision now requires changes to proceed.Sep 21 2018, 2:28 PM

Yeah I think that is a mistake. D42047 which precedes the current diff, makes much more sense.

lsartorelli retitled this revision from Bug 395349 to Provide option to hide menu bar for Ksysguard - Bug 395349.
lsartorelli edited the summary of this revision. (Show Details)

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

ngraham added a comment.EditedSep 21 2018, 4:44 PM

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?

gui/ksysguard.cpp
73

Unrelated whitespace change

148

Extra whitespace

gui/ksysguard.h
78

Accidental trailing whitespace

ngraham added inline comments.Sep 21 2018, 4:47 PM
gui/ksysguard.cpp
149

Minor nitpick: "setup" is a noun; it should be "set up" so that there's a verb in the sentence.

lsartorelli retitled this revision from Provide option to hide menu bar for Ksysguard - Bug 395349 to Provide option to hide menu bar for Ksysguard.Sep 22 2018, 7:29 PM
lsartorelli edited the summary of this revision. (Show Details)

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.

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

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.

Added remainder message box with keyboard shortcut, to have back the menu bar.

ngraham accepted this revision.Sep 25 2018, 3:12 PM

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 ;-)

Any concerns from Plasma or Frameworks folks before I land this?

This revision is now accepted and ready to land.Sep 25 2018, 3:12 PM

Thank you very much,

I am so happy and proud for my first patch.

Here my details:
Name: Luca
Surname: Sartorelli
Mail: dj3mb3@gmail.com

And here is the patch for gwenview: https://phabricator.kde.org/D15747

Just added you as reviewer 2 secs ago

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.

Removed unused parameter in toggleShowMenuBar()

ngraham accepted this revision.Sep 26 2018, 12:53 PM
sitter added a subscriber: sitter.Sep 26 2018, 1:42 PM

Looks almost perfect to me. Only nit-pick I have is the fact that the include is out of order. Other than that it looks awesome ๐Ÿ‘

gui/ksysguard.cpp
54

That should be sorted alphabetically.

sitter added a subscriber: broulik.Sep 26 2018, 1:50 PM
sitter added inline comments.
gui/ksysguard.cpp
148

@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.

https://wiki.qt.io/New_Signal_Slot_Syntax

little clean up

ngraham accepted this revision.Sep 26 2018, 2:36 PM

Looks perfect to me. BTW, you can mark inline comments as done by clicking in their checkboxes and then clicking Submit on the bottom of the page (I know, I know, it's a bit weird).

@sitter, does this look good to you now?

lsartorelli marked 6 inline comments as done.Sep 26 2018, 3:12 PM
sitter accepted this revision.Sep 27 2018, 8:36 AM

It's perfect now!

This revision was automatically updated to reflect the committed changes.

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?