D15644: Provide option to hide menu bar for Ksysguard

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 co

D15644: Provide option to hide menu bar for Ksysguard

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?v

D15644: Provide option to hide menu bar for Ksysguard

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, jens

D15644: Provide option to hide menu bar for Ksysguard

lsartorelli marked 6 inline comments as done. 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

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? REVISIO

D15644: Provide option to hide menu bar for Ksysguard

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&id=42368 REVISION DETAIL https://phabricator.kde.org/D15644 AFFECTED FILES gui/ksysguard.cpp gui/ksysguard.h To: lsarto

D15644: Provide option to hide menu bar for Ksysguard

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

D15644: Provide option to hide menu bar for Ksysguard

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://phabric

D15644: Provide option to hide menu bar for Ksysguard

ngraham accepted this revision. 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

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&id=42350 REVISION DETAIL https://phabricator.kde.org/D15644 AFFECTED FILES gui/ksysguard.c

D15644: Provide option to hide menu bar for Ksysguard

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 paramete

D15644: Provide option to hide menu bar for Ksysguard

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 se

D15644: Provide option to hide menu bar for Ksysguard

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 l

D15644: Provide option to hide menu bar for Ksysguard

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&id=42298 REVISION DETAIL https://phabricator.kde.org/D15644 A

D15644: Provide option to hide menu bar for Ksysguard

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 sett

D15644: Provide option to hide menu bar for Ksysguard

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 contex

D15644: Provide option to hide menu bar for Ksysguard

lsartorelli retitled this revision from "Provide option to hide menu bar for Ksysguard - Bug 395349" to "Provide option to hide menu bar for Ksysguard". lsartorelli edited the summary of this revision. REVISION DETAIL https://phabricator.kde.org/D15644 To: lsartorelli, ngraham, #plasma, #frame

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

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

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

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

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

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