| ngraham |
| Frameworks | |
| KDE Applications |
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.
| Lint Skipped |
| Unit Tests Skipped |
| src/ktoggleshowmenubaraction.cpp | ||
|---|---|---|
| 27 | What's this for? | |
| src/ktoggleshowmenubaraction.h | ||
| 50 | 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? | |
| 79 | There's an extra empty line | |
| src/ktoggleshowmenubaraction.h | ||
|---|---|---|
| 29 | Unfortunately, me too, ... any suggestion is appreciated | |
| src/ktoggleshowmenubaraction.cpp | ||
|---|---|---|
| 78 | 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? | |
| src/ktoggleshowmenubaraction.cpp | ||
|---|---|---|
| 78 | Do you suggest to add a warning in case of no shortcut? | |
Even though a lot of inline comments have been marked as resolved, I don't actually see that the requested changes have been made.
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.
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?
| src/ktoggleshowmenubaraction.cpp | ||
|---|---|---|
| 78 | 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. | |
Usually the message should be always shown, but I think there there are some exceptions.
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 :-)