Review Request 120149: [OS X] improved menubar experience: protected Preferences menu and cleaner "system tray"
Ian Wadham
iandw.au at gmail.com
Thu Sep 18 12:57:37 BST 2014
> On Sept. 15, 2014, 12:19 p.m., Thomas Lübking wrote:
> > kdeui/actions/kaction.cpp, line 149
> > <https://git.reviewboard.kde.org/r/120149/diff/2/?file=312029#file312029line149>
> >
> > what if
> > KAction *foo = new KAction(this);
> > foo->setText("Foo");
> >
> > -> you rather want to monitor the "changed()" signal?
>
> René J.V. Bertin wrote:
> Yes, that would probably be more elegant. I'm not exactly sure how one would monitor that signal, but it seems that it might be a bit complicated in the context of something that's bound to disappear?
>
> Thomas Lübking wrote:
> You'd add a private slot to KAction and "connect(this, SIGNAL(changed()), SLOT(fixMenuRole()))"
> Before altering the text in ::fixMenuRole(), don't forget to block (and later unblock) signals to not enter a recursion.
>
> Since the changed() signal is not only called for altering text (but also icons, the role and some other stuff) you also might want to keep a string member to check whether the text actually changed before updating the role.
>
> René J.V. Bertin wrote:
> The NULL alternative would be to modify the default KAction ctor to do `setMenuRole(NoRole)`, which is the effective default on other platforms (where `TextHeuristicRole` is unused).
>
> Ian Wadham wrote:
> I think this may be the best way to go with KDE 4 on OS X. Yesterday evening (Aust. time) I trawled through all the relevant source code in both kdelibs and Qt and I came up with the same idea, but decided to sleep on it. Maybe I sent it to you in my dreams... :-) Here's why I think it may be the neatest solution.
>
> 1. KDE 4 apps usually use QAction, KAction or KStandardAction, or derivative classes (e.g. KStandardGameAction?), to set up actions.
> 2. KStandardAction sets the correct MenuRole for all standard KDE actions, including NoRole as a default.
> 3. The problem lies with actions that are not standard KDE actions but have a similar wording (e.g. Configure Editor).
> 4. These can be picked up as false positives by the Qt-Mac "heuristic" and will then corrupt the Preferences entry in Apple's Application menu.
> 5. KStandardAction's create() first sets up data-values for the required action, then it creates the KAction object (or derived action object).
> 6. Finally, it sets the MenuRole to NoRole or one of the specialised roles.
> 7. Therefore, setting the MenuRole to NoRole in the KAction constructor would not invalidate what KStandardAction does.
> 8. Also, no KAction objects will then be affected by Qt's heuristic, even if they are not KStandardActions (e.g. Configure Editor is KAction?).
> 9. That will leave QActions in some KDE apps exposed, but I doubt if they will be a problem. If they are, they could be fixed manually.
>
> In KF5, KAction is deprecated in favour of QAction. I think the only solution there is to get into Qt5, as you and Thomas have discussed, and change the default MenuRole to be NoRole, for KDE apps, rather than the heuristic role.
>
> BTW, it occurs to me that the heuristic may be for the benefit of Qt's paying customers --- businesses and organisations who may have large in-house suites of apps written using Qt and may have a mix of hardware platforms and O/S's on people's desks.
>
> If we patched Qt5-Mac in MacPorts to change the default MenuRole, I doubt if any apps ported to Apple OS X would be adversely affected --- but you never know. I suppose it is easy enough to ask MacPorts' "port" command if any apps, other than KDE or Qt apps, depend on Qt-Mac.
>
> René J.V. Bertin wrote:
> Thanks Ian for writing this down in a more concise way. It's exactly the conclusion I've came to, except maybe for the reason why the heuristics are there. There'd be a large and influential enough customerbase who have Qt code running on Macs AND 1) insist on it showing the menus in the OS-X-dictated place and 2) can't be bothered to insert 1 or 2 `setMenuRole` calls for that to happen (guided by paid support feedback? And nothing similar would ever be required on one of the more wide-spread platforms Qt supports? Personally I'd have guessed this were the oeuvre of maybe even a single person who's in love with OS X and wants to do things right without having really thought of all the potential consequences (a bit like how I killed the "icons in systray" bug at first).
>
> Qt5's case is slightly more complicated because it introduces MenuRole for cut, copy, paste and select-all. That apparently is to modify the role of those "standard actions" (quote from the relevant commit message) when a QFileDialog is open. As long as they're not used to move menu items around it's probably OK, though. Marko is helping me look into that, and dfaure provided me with the name of the heuristics' author.
>
> Ian: what kind of app that's not a KDE or Qt app could possibly be affected by this?
On Apple OS X and MacPorts I use "Qt app" to mean an app provided by Qt, such as Assistant or Designer. Those can look after their own menu roles, I presume, as KDE apps do. There might also be (in MacPorts) some non-KDE apps based on Qt, e.g. some of the FOSS science applications. Their authors would not necessarily be aware of menu-role issues.
In the world at large, I am imagining application systems with hundreds of screen-forms, background processes and printed outputs, programmed using C++ and Qt, that are required to run, say, a baggage-handling system at a major airport. I do not know whether that is the kind of complexity Qt customers have to handle in their businesses, but certainly such complexity does exist in many businesses. I am sure that the programmers working with it would welcome anything from Qt that saved them having to worry about cosmetics on the occasional Mac user's screen and left them free to concentrate on keeping the conveyors rolling and routing the baggage correctly... I use that example because there has been at least one such system that failed to run correctly and actually delayed the opening of a new airport. I have myself worked on some quite complex systems and would have welcomed C++ and Qt back in the day (60s thru 90s).
- Ian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120149/#review66546
-----------------------------------------------------------
On Sept. 17, 2014, 5:48 p.m., René J.V. Bertin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120149/
> -----------------------------------------------------------
>
> (Updated Sept. 17, 2014, 5:48 p.m.)
>
>
> Review request for KDE Software on Mac OS X, kdelibs, KDEPIM, Marco Martin, and Olivier Goffart.
>
>
> Repository: kdelibs
>
>
> Description
> -------
>
> This review is for 2 sets of changes; an initial one to the way "system tray" are rendered, and a newer set that protects the Preferences menu from getting linked to any action with an appropriate title.
>
> -- the system tray:
> Until now, "system tray" menus had some rendering issues on Mac OS X:
>
> - The menu title, the 1st menu item that on Linux shows the application name, remained empty
> - Menu items that can (in principle, potentially) show an icon always showed the icon
>
> Point 1 was resolved by emulating the Linux addTitle/setTitle action in `KStatusNotifierItemPrivate::init()` : the menu title is implemented as a deactive standard menuitem followed by a separator. This makes the item stand out on a GUI that doesn't support the kind of formatting in menus as used in the Linux implementation.
>
> Point 2 was identified as a Qt issue: `isIconVisibleInMenu` is ignored for systray menus. It was resolved by adding `KMenu::addAction` methods that overload the ones from QMenu that were hitherto inherited unchanged by KMenu. The only different method is `addAction(QAction*)` which removes the icon from the `QAction` if `isIconVisibleInMenu()` is false. The other `addAction` methods are "overloaded with themselves" with `using QMenu::addAction;` in the header file.
>
> -- the Preferences menu item
> This is a menu item living in the Application menu, a menu that sits in the menubar between the Apple (?) menu and the File menu. This menu also contains the Quit command.
> KDE and Qt applications typically do not set up their menus in this fashion, so Qt provides an automatic way to put relevant menu items (actions) in the Application menu, using Apple's naming. The algorithm is described under QMenuBar in the Qt documentation: for the Preferences action, it will consider any action that has a text containing `config`, `options`, `settings` or `preferences`, and put it under the Preferences label if its menu role is set to `heuristic` (which appears to be the default).
> In practice, many applications provide a series of menu actions with names that trigger this method, and they do not always create their own preferences/settings/configuration menu first. Yet it is the first menu action that matches that will be installed under the Preferences menu, with the Command-, shortcut. A good example is KDevelop: it will have a Preferences menu that activates the `Configure Selection` action - which does not open a settings dialog but launches the configure or cmake procedure for the selected project ...
>
> My proposed solution overrides this Qt behaviour. On OS X, the `KAction(const QString &text, QObject *parent)` constructor calls a modified (static) function `setTextWithCorrectMenuRole` which checks the text against the patterns Qt will consider for `PreferencesRole`. If it finds a match, it will force the role to `NoRole`, unless it is a perfect match with the standard KDE configuration action for the application (`"&Configuration appName..."`) in which case it sets the role to `PreferencesRole`. This latter consideration allows kdelibs to "catch" the configuration menu for applications like KMail, which appear not to be created using KStandardActions.
> This approach can be extended to other menu actions that end up incorrectly in the OS X Application menu.
>
> Applications that create menu actions using QAction or a different KAction constructor will see no change (and should use `setMenuRole` selectively on OS X).
>
>
> Diffs
> -----
>
> kdeui/actions/kaction.cpp 9e8f7fb
> kdeui/notifications/kstatusnotifieritem.cpp 1b15d40
>
> Diff: https://git.reviewboard.kde.org/r/120149/diff/
>
>
> Testing
> -------
>
> Testing was done with kdelibs git/master and KDE/MacPorts on OS X 10.6.8 . The modified code is in compile-time conditional blocks used only on OS X, so no regressions are to be expected on other platforms.
>
> KF5 is not production ready on OS X, so I am not currently able to port these modifications beyond KDE4. However, I did see that Qt5 has a new approach to adding titles to menus, which can be described as a "labelled separator". Backporting that function from the Qt5 source to kdelibs gave menu items that had the separator but not the text (title) label. It is thus likely that some kind of emulation will also be required with KF5, on OS X.
>
> I considered doing the addTitle/setTitle emulation in kmenu.cpp, but decided against that for now. Menu titles are rendered as under Linux in menus that are not attached to the OS X toplevel menubar (say in context menus). Without knowing how to distinguish the kind of menu in KMenu methods the emulation will have to remain in the client code.
>
> The Preferences menu protection should carry over easily to KF5, supposing Qt5 uses the same heuristics to place relevant menu actions under the OS X application menu, and supposing `KAction` has made the transition to KF5.
>
>
> Thanks,
>
> René J.V. Bertin
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20140918/2b2dfda8/attachment.htm>
More information about the kde-core-devel
mailing list