D15644: Provide option to hide menu bar for Ksysguard

[Nathaniel Graham]

ngraham added a comment.


  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?

REPOSITORY
  R106 KSysguard

REVISION DETAIL
  https://phabricator.kde.org/D15644

To: lsartorelli, ngraham, #plasma, #frameworks, sitter
Cc: broulik, sitter, acrouthamel, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15644: Provide option to hide menu bar for Ksysguard

[Harald Sitter]

This revision was automatically updated to reflect the committed changes.
Closed by commit R106:3a3220d41ac4: Provide option to hide menu bar for 
Ksysguard (authored by lsartorelli, committed by sitter).

REPOSITORY
  R106 KSysguard

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15644?vs=42368=42414

REVISION DETAIL
  https://phabricator.kde.org/D15644

AFFECTED FILES
  gui/ksysguard.cpp
  gui/ksysguard.h

To: lsartorelli, ngraham, #plasma, #frameworks, sitter
Cc: broulik, sitter, acrouthamel, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15644: Provide option to hide menu bar for Ksysguard

[Harald Sitter]

sitter accepted this revision.
sitter added a comment.


  It's perfect now!

REVISION DETAIL
  https://phabricator.kde.org/D15644

To: lsartorelli, ngraham, #plasma, #frameworks, sitter
Cc: broulik, sitter, acrouthamel, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15644: Provide option to hide menu bar for Ksysguard

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.


  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?

REVISION DETAIL
  https://phabricator.kde.org/D15644

To: lsartorelli, ngraham, #plasma, #frameworks
Cc: broulik, sitter, acrouthamel, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15644: Provide option to hide menu bar for Ksysguard

[Luca Sartorelli]

lsartorelli updated this revision to Diff 42368.
lsartorelli added a comment.


  little clean up

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15644?vs=42350=42368

REVISION DETAIL
  https://phabricator.kde.org/D15644

AFFECTED FILES
  gui/ksysguard.cpp
  gui/ksysguard.h

To: lsartorelli, ngraham, #plasma, #frameworks
Cc: broulik, sitter, acrouthamel, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15644: Provide option to hide menu bar for Ksysguard

[Harald Sitter]

sitter added a subscriber: broulik.
sitter added inline comments.

INLINE COMMENTS

> ksysguard.cpp:148
> +  // set up 'Settings' menu
> +  mShowMenuBarAction = KStandardAction::showMenubar(this, 
> SLOT(toggleShowMenuBar()), actionCollection());
>  

@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, 
::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

REVISION DETAIL
  https://phabricator.kde.org/D15644

To: lsartorelli, ngraham, #plasma, #frameworks
Cc: broulik, sitter, acrouthamel, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15644: Provide option to hide menu bar for Ksysguard

[Harald Sitter]

sitter added a comment.


  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 

INLINE COMMENTS

> ksysguard.cpp:54
>  #include 
> +#include 
>  

That should be sorted alphabetically.

REVISION DETAIL
  https://phabricator.kde.org/D15644

To: lsartorelli, ngraham, #plasma, #frameworks
Cc: sitter, acrouthamel, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15644: Provide option to hide menu bar for Ksysguard

[Luca Sartorelli]

lsartorelli updated this revision to Diff 42350.
lsartorelli added a comment.


  Removed unused parameter in toggleShowMenuBar()

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15644?vs=42298=42350

REVISION DETAIL
  https://phabricator.kde.org/D15644

AFFECTED FILES
  gui/ksysguard.cpp
  gui/ksysguard.h

To: lsartorelli, ngraham, #plasma, #frameworks
Cc: acrouthamel, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15644: Provide option to hide menu bar for Ksysguard

[Nathaniel Graham]

ngraham added a comment.


  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.

REVISION DETAIL
  https://phabricator.kde.org/D15644

To: lsartorelli, ngraham, #plasma, #frameworks
Cc: acrouthamel, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15644: Provide option to hide menu bar for Ksysguard

[Luca Sartorelli]

lsartorelli added a comment.


  Thank you very much,
  
  I am so happy and proud for my first patch.
  
  Here my details:
  Name: Luca 
  Surname: Sartorelli 
  Mail: dj3...@gmail.com
  
  And here is the patch for gwenview: https://phabricator.kde.org/D15747
  
  Just added you as reviewer 2 secs ago

REVISION DETAIL
  https://phabricator.kde.org/D15644

To: lsartorelli, ngraham, #plasma, #frameworks
Cc: acrouthamel, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15644: Provide option to hide menu bar for Ksysguard

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.
This revision is now accepted and ready to land.


  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?

REVISION DETAIL
  https://phabricator.kde.org/D15644

To: lsartorelli, ngraham, #plasma, #frameworks
Cc: acrouthamel, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15644: Provide option to hide menu bar for Ksysguard

[Luca Sartorelli]

lsartorelli updated this revision to Diff 42298.
lsartorelli added a comment.


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

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15644?vs=42101=42298

REVISION DETAIL
  https://phabricator.kde.org/D15644

AFFECTED FILES
  gui/ksysguard.cpp
  gui/ksysguard.h

To: lsartorelli, ngraham, #plasma, #frameworks
Cc: acrouthamel, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15644: Provide option to hide menu bar for Ksysguard

[Nathaniel Graham]

ngraham added a comment.


  In D15644#330281 , @lsartorelli 
wrote:
  
  > 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.

REVISION DETAIL
  https://phabricator.kde.org/D15644

To: lsartorelli, ngraham, #plasma, #frameworks
Cc: acrouthamel, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15644: Provide option to hide menu bar for Ksysguard

[Luca Sartorelli]

lsartorelli added a comment.


  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.

REVISION DETAIL
  https://phabricator.kde.org/D15644

To: lsartorelli, ngraham, #plasma, #frameworks
Cc: acrouthamel, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15644: Provide option to hide menu bar for Ksysguard - Bug 395349

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

> ksysguard.cpp:149
> +
> +  // setup 'Settings' menu
> +  KToggleAction* showMenuBar = KStandardAction::showMenubar(nullptr, 
> nullptr, actionCollection());

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

REVISION DETAIL
  https://phabricator.kde.org/D15644

To: lsartorelli, ngraham, #plasma, #frameworks
Cc: acrouthamel, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15644: Provide option to hide menu bar for Ksysguard - Bug 395349

[Nathaniel Graham]

ngraham added a comment.


  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. 
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): F6277660: Hide.png 
  - 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?

INLINE COMMENTS

> ksysguard.cpp:73
>  
> +
>  //Comment out to stop ksysguard from forking.  Good for debugging

Unrelated whitespace change

> ksysguard.cpp:148
>connect(mConfigureSheetAction, ::triggered, this, 
> ::configureCurrentSheet);
> -
> +
> +  // setup 'Settings' menu

Extra whitespace

> ksysguard.h:78
>  void updateProcessCount();
> -void configureCurrentSheet();
> +void configureCurrentSheet();
> +void toggleShowMenuBar();

Accidental trailing whitespace

REVISION DETAIL
  https://phabricator.kde.org/D15644

To: lsartorelli, ngraham, #plasma, #frameworks
Cc: acrouthamel, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15644: Provide option to hide menu bar for Ksysguard - Bug 395349

[Luca Sartorelli]

lsartorelli updated this revision to Diff 42101.
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.
lsartorelli added a comment.


  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

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15644?vs=42048=42101

REVISION DETAIL
  https://phabricator.kde.org/D15644

AFFECTED FILES
  gui/ksysguard.cpp
  gui/ksysguard.h

To: lsartorelli, ngraham, #plasma, #frameworks
Cc: acrouthamel, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart