D17013: Show menu bar, how to re-enable, common shortcut dialog
loh.tar added a comment. Hi, I accidentally went by. How about to add an icon to some (main) toolbar, if any is present? That would the need to show such a message reduce to cases where no tool bar is available. If that icon was added, and not already there, could that pop up a small tool tip to get some attention. A quick search here in my browser give no hit for "isVisible". There should be no need to show such message when the action is somewhere seen. Sorry for the noise, if I should completely wrong :-) REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D17013 To: lsartorelli, ngraham, #frameworks, #kde_applications Cc: loh.tar, aacid, cfeck, broulik, kde-frameworks-devel, michaelh, ngraham, bruns
D17013: Show menu bar, how to re-enable, common shortcut dialog
lsartorelli updated this revision to Diff 46321. lsartorelli added a comment. Added @since and @author in the header documentation REPOSITORY R236 KWidgetsAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17013?vs=46248=46321 REVISION DETAIL https://phabricator.kde.org/D17013 AFFECTED FILES src/CMakeLists.txt src/ktoggleshowmenubaraction.cpp src/ktoggleshowmenubaraction.h To: lsartorelli, ngraham, #frameworks, #kde_applications Cc: aacid, cfeck, broulik, kde-frameworks-devel, michaelh, ngraham, bruns
D17013: Show menu bar, how to re-enable, common shortcut dialog
aacid added a comment. the documentation is mising a @since 5.SOMETHING REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D17013 To: lsartorelli, ngraham, #frameworks, #kde_applications Cc: aacid, cfeck, broulik, kde-frameworks-devel, michaelh, ngraham, bruns
D17013: Show menu bar, how to re-enable, common shortcut dialog
lsartorelli added a comment. Usually the message should be always shown, but I think there there are some exceptions. - for Kate the message is supposed to be off till its code base is upgraded with the removal of ts own popup. - for dolphin the message should be shown only if the menu bar and the tool bar are hided at the same time. (in this case a better message could be shown) REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D17013 To: lsartorelli, ngraham, #frameworks, #kde_applications Cc: aacid, cfeck, broulik, kde-frameworks-devel, michaelh, ngraham, bruns
D17013: Show menu bar, how to re-enable, common shortcut dialog
ngraham added a comment. Looking better now. Do we really want to only optionally show the warning with a bool argument? In general bool arguments to functions are discouraged because they're not very readable. And if the whole point of this function is to always show a consistent message, don't we want it to always be shown when an app uses this function? INLINE COMMENTS > aacid wrote in ktoggleshowmenubaraction.cpp:77 > No strong opinion really, anyone else has an idea? I would recommend telling the user how they can add one. Since a shortcut is set by default, only advanced users who change this would trigger that boundary condition anyway, and they'd read the warning and understand what's going on. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D17013 To: lsartorelli, ngraham, #frameworks, #kde_applications Cc: aacid, cfeck, broulik, kde-frameworks-devel, michaelh, ngraham, bruns
D17013: Show menu bar, how to re-enable, common shortcut dialog
lsartorelli updated this revision to Diff 46248. lsartorelli added a comment. Hi, you are right, I did not realize a bit of mess in between my revisions. Now all the requests should have been fulfilled properly. Thanks for this massive support and please, just let me know if I need to fix something else. REPOSITORY R236 KWidgetsAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17013?vs=45934=46248 REVISION DETAIL https://phabricator.kde.org/D17013 AFFECTED FILES src/CMakeLists.txt src/ktoggleshowmenubaraction.cpp src/ktoggleshowmenubaraction.h To: lsartorelli, ngraham, #frameworks, #kde_applications Cc: aacid, cfeck, broulik, kde-frameworks-devel, michaelh, ngraham, bruns
D17013: Show menu bar, how to re-enable, common shortcut dialog
ngraham added a comment. Even though a lot of inline comments have been marked as resolved, I don't actually see that the requested changes have been made. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D17013 To: lsartorelli, ngraham, #frameworks, #kde_applications Cc: aacid, cfeck, broulik, kde-frameworks-devel, michaelh, ngraham, bruns
D17013: Show menu bar, how to re-enable, common shortcut dialog
lsartorelli marked an inline comment as done. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D17013 To: lsartorelli, ngraham, #frameworks, #kde_applications Cc: aacid, cfeck, broulik, kde-frameworks-devel, michaelh, ngraham, bruns
D17013: Show menu bar, how to re-enable, common shortcut dialog
lsartorelli updated this revision to Diff 45934. lsartorelli marked an inline comment as done. REPOSITORY R236 KWidgetsAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17013?vs=45874=45934 REVISION DETAIL https://phabricator.kde.org/D17013 AFFECTED FILES src/CMakeLists.txt src/ktoggleshowmenubaraction.cpp src/ktoggleshowmenubaraction.h To: lsartorelli, ngraham, #frameworks, #kde_applications Cc: aacid, cfeck, broulik, kde-frameworks-devel, michaelh, ngraham, bruns
D17013: Show menu bar, how to re-enable, common shortcut dialog
aacid added inline comments. INLINE COMMENTS > lsartorelli wrote in ktoggleshowmenubaraction.cpp:77 > Do you suggest to add a warning in case of no shortcut? > The only options I am thinking of are > to tell the user add the missing shortcut via system settings, > to tell the user to modify the config file for current app or > I don't know No strong opinion really, anyone else has an idea? > cfeck wrote in ktoggleshowmenubaraction.cpp:21 > Do we still follow the rule "include own headers first"? This is marked as done, but it is not really done, is it? I still see KMessageBox as first include. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D17013 To: lsartorelli, ngraham, #frameworks, #kde_applications Cc: aacid, cfeck, broulik, kde-frameworks-devel, michaelh, ngraham, bruns
D17013: Show menu bar, how to re-enable, common shortcut dialog
lsartorelli marked an inline comment as done. lsartorelli added inline comments. INLINE COMMENTS > aacid wrote in ktoggleshowmenubaraction.cpp:77 > If shortcut() returns a QKeySequence (i think it does), you want > toString(NativeText) > > Not a native speaker but i don't think "typing" is what you want, i'd say > "pressing". > > Also what if there's no shortcut? Do you suggest to add a warning in case of no shortcut? The only options I am thinking of are to tell the user add the missing shortcut via system settings, to tell the user to modify the config file for current app or I don't know REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D17013 To: lsartorelli, ngraham, #frameworks, #kde_applications Cc: aacid, cfeck, broulik, kde-frameworks-devel, michaelh, ngraham, bruns
D17013: Show menu bar, how to re-enable, common shortcut dialog
lsartorelli updated this revision to Diff 45874. REPOSITORY R236 KWidgetsAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17013?vs=45821=45874 REVISION DETAIL https://phabricator.kde.org/D17013 AFFECTED FILES src/CMakeLists.txt src/ktoggleshowmenubaraction.cpp src/ktoggleshowmenubaraction.h To: lsartorelli, ngraham, #frameworks, #kde_applications Cc: aacid, cfeck, broulik, kde-frameworks-devel, michaelh, ngraham, bruns
D17013: Show menu bar, how to re-enable, common shortcut dialog
aacid added inline comments.
INLINE COMMENTS
> ktoggleshowmenubaraction.cpp:77
> +KMessageBox::information(d->window, tr("This will hide the menu bar
> completely."
> +" You can show it again by
> typing %1.").arg(d->q->shortcut().toString()),
> +tr("Hide menu bar"),
If shortcut() returns a QKeySequence (i think it does), you want
toString(NativeText)
Not a native speaker but i don't think "typing" is what you want, i'd say
"pressing".
Also what if there's no shortcut?
REPOSITORY
R236 KWidgetsAddons
REVISION DETAIL
https://phabricator.kde.org/D17013
To: lsartorelli, ngraham, #frameworks, #kde_applications
Cc: aacid, cfeck, broulik, kde-frameworks-devel, michaelh, ngraham, bruns
D17013: Show menu bar, how to re-enable, common shortcut dialog
lsartorelli updated this revision to Diff 45821. REPOSITORY R236 KWidgetsAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17013?vs=45812=45821 REVISION DETAIL https://phabricator.kde.org/D17013 AFFECTED FILES src/CMakeLists.txt src/ktoggleshowmenubaraction.cpp src/ktoggleshowmenubaraction.h To: lsartorelli, ngraham, #frameworks, #kde_applications Cc: cfeck, broulik, kde-frameworks-devel, michaelh, ngraham, bruns
D17013: Show menu bar, how to re-enable, common shortcut dialog
lsartorelli marked 3 inline comments as done. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D17013 To: lsartorelli, ngraham, #frameworks, #kde_applications Cc: cfeck, broulik, kde-frameworks-devel, michaelh, ngraham, bruns
D17013: Show menu bar, how to re-enable, common shortcut dialog
ngraham added inline comments. INLINE COMMENTS > ktoggleshowmenubaraction.h:57 > +/** > + * Sets the parent window for message box with the user remainder. > + * @param window the window that will be parent for the message box remainder -> reminder > ktoggleshowmenubaraction.h:64 > + * If set to true, this action will remind the user through a message box > + * on how to show back the menu bar using the keyborad shortcut. > + * Default: false show back the menu bar -> show the menu bar again keyborad -> keyboard REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D17013 To: lsartorelli, ngraham, #frameworks, #kde_applications Cc: cfeck, broulik, kde-frameworks-devel, michaelh, ngraham, bruns
D17013: Show menu bar, how to re-enable, common shortcut dialog
ngraham added reviewers: Frameworks, KDE Applications. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D17013 To: lsartorelli, ngraham, #frameworks, #kde_applications Cc: cfeck, broulik, kde-frameworks-devel, michaelh, ngraham, bruns
D17013: Show menu bar, how to re-enable, common shortcut dialog
lsartorelli added inline comments. INLINE COMMENTS > cfeck wrote in ktoggleshowmenubaraction.h:28 > reminds > > Additionally, "show back" does not sound like proper english, but I am not a > native english speaker. Unfortunately, me too, ... any suggestion is appreciated REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D17013 To: lsartorelli, ngraham Cc: cfeck, broulik, kde-frameworks-devel, michaelh, ngraham, bruns
D17013: Show menu bar, how to re-enable, common shortcut dialog
lsartorelli updated this revision to Diff 45812. lsartorelli marked 2 inline comments as done. REPOSITORY R236 KWidgetsAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17013?vs=45811=45812 REVISION DETAIL https://phabricator.kde.org/D17013 AFFECTED FILES src/CMakeLists.txt src/ktoggleshowmenubaraction.cpp src/ktoggleshowmenubaraction.h To: lsartorelli, ngraham Cc: cfeck, broulik, kde-frameworks-devel, michaelh, ngraham, bruns
D17013: Show menu bar, how to re-enable, common shortcut dialog
lsartorelli updated this revision to Diff 45811. REPOSITORY R236 KWidgetsAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17013?vs=45807=45811 REVISION DETAIL https://phabricator.kde.org/D17013 AFFECTED FILES src/CMakeLists.txt src/ktoggleshowmenubaraction.cpp src/ktoggleshowmenubaraction.h To: lsartorelli, ngraham Cc: cfeck, broulik, kde-frameworks-devel, michaelh, ngraham, bruns
D17013: Show menu bar, how to re-enable, common shortcut dialog
cfeck added inline comments. INLINE COMMENTS > ktoggleshowmenubaraction.cpp:21 > + > +#include "ktoggleshowmenubaraction.h" > + Do we still follow the rule "include own headers first"? > ktoggleshowmenubaraction.h:28 > + * An action to hide or show the menubar of a window. > + * The action by default remind the user how to show back > + * the menu bar using the keyboard shortcut. reminds Additionally, "show back" does not sound like proper english, but I am not a native english speaker. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D17013 To: lsartorelli, ngraham Cc: cfeck, broulik, kde-frameworks-devel, michaelh, ngraham, bruns
D17013: Show menu bar, how to re-enable, common shortcut dialog
lsartorelli added a dependent revision: D17016: Show menu bar, how to re-enable, common shortcut dialog. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D17013 To: lsartorelli, ngraham Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns
D17013: Show menu bar, how to re-enable, common shortcut dialog
lsartorelli marked 2 inline comments as done. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D17013 To: lsartorelli, ngraham Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns
D17013: Show menu bar, how to re-enable, common shortcut dialog
lsartorelli updated this revision to Diff 45807. REPOSITORY R236 KWidgetsAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17013?vs=45798=45807 REVISION DETAIL https://phabricator.kde.org/D17013 AFFECTED FILES src/CMakeLists.txt src/ktoggleshowmenubaraction.cpp src/ktoggleshowmenubaraction.h To: lsartorelli, ngraham Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns
D17013: Show menu bar, how to re-enable, common shortcut dialog
broulik added inline comments. INLINE COMMENTS > ktoggleshowmenubaraction.cpp:26 > + > +#define i18n QString::fromLatin1 > + What's this for? > ktoggleshowmenubaraction.h:49 > + */ > +KToggleShowMenuBarAction(QWidget *window, QObject *parent); > + I know we typically use `window` here for legacy reasons, but given there's `QWindow` in Qt 5, might be better to use `widget` for all of this now? Just asking, you don't have to change this now, unless someone else comments :) > ktoggleshowmenubaraction.h:78 > + > + > +private: There's an extra empty line REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D17013 To: lsartorelli, ngraham Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns
D17013: Show menu bar, how to re-enable, common shortcut dialog
lsartorelli added a dependent revision: D17014: Show menu bar, how to re-enable, common shortcut dialog. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D17013 To: lsartorelli, ngraham Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D17013: Show menu bar, how to re-enable, common shortcut dialog
lsartorelli created this revision. lsartorelli added a reviewer: ngraham. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. lsartorelli requested review of this revision. REVISION SUMMARY Provide a common message ,encapsulated into the ShowMenuBarAction, with a reminder about how to re-enable the menu bar with a keyboard shortcut sequence. The message dialog is optional by default and can be enabled if needed. The message dialog pops up only after the menu bar become hided. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D17013 AFFECTED FILES src/CMakeLists.txt src/ktoggleshowmenubaraction.cpp src/ktoggleshowmenubaraction.h To: lsartorelli, ngraham Cc: kde-frameworks-devel, michaelh, ngraham, bruns
