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