Review Request 120149: [OS X] improved menubar experience: protected Preferences menu and cleaner "system tray"
Thomas Lübking
thomas.luebking at gmail.com
Tue Sep 16 07:43:49 BST 2014
> On Sept. 15, 2014, 12:19 nachm., Thomas Lübking wrote:
> > kdeui/actions/kaction.cpp, line 188
> > <https://git.reviewboard.kde.org/r/120149/diff/2/?file=312029#file312029line188>
> >
> > given KAction, KMenu and KMenuBar are all deprecated in KF5, is this actually of any upstream relevance?
>
> René J.V. Bertin wrote:
> I wouldn't know. It's been mentioned often enough that KF5 is still quite a while away from being release-ready on OS X, and in the meantime the best way to maintain momentum in getting it that far is to improve the user experience of what *is* available on the platform. Meaning KDE4 ...
>
> As to upstream relevance ... it clearly is unfortunate that KAction, KMenu and KMenuBar have been deprecated, because Qt's baked-in heuristics haven't changed in Qt5. From what I see that makes it neigh impossible to address the issues I'm trying to address in KF5 .
>
> Thomas Lübking wrote:
> Hint: KStandardAction is *not* deprecated so if setting the explicit role in KStandardAction::preferences() is sufficient, you won.
>
> René J.V. Bertin wrote:
> To rephrase what I was getting at in the issue above: `KStandardAction::preferences()` could work, but only if no other menu action is created first that has a label that starts with "Config" ... and of course just about any slightly elaborate KDE app has several Configure actions in its Settings menu ... with the one that *should* go under Preferences last (and thus added last).
>
> The more I think about it, the more I feel that Qt goes too far out of its way to bend applications to OS X conventions. Those apps that do want to adhere to those *guidelines* can always make sure that they create the Preference (and About) action(s) first on OS X. But a functioning UI that doesn't hide a few surprises seems more important overall.
>
> I'll try to see what dfaure thinks of this - is he already getting updates from this RR through one of the reviewer groups?
> is he already getting updates
Yes - but that was rather a joke ("if nobody else has an answer, david likely has" ;-)
You'll have to file a bug or patch against Qt, notably if this only has relevance on OSX (and since then esp. the Qt devs focussing on OSX will have to review it)
> On Sept. 15, 2014, 12:19 nachm., Thomas Lübking wrote:
> > kdeui/actions/kstandardaction.cpp, line 173
> > <https://git.reviewboard.kde.org/r/120149/diff/2/?file=312030#file312030line173>
> >
> > Will it?
> > I don't see such code in QAction, setText is not virtual and why setTextWithCorrectRole itfp then?
>
> René J.V. Bertin wrote:
> True.
>
> Does C++ offer any mechanism by which one could overload a class's member function (or extend a class) without creating a child class? That might offer a solution on KF5.
>
> If no, the setText call can go back to where it came from (personally I do find it more logical to set a role after having set the text, though).
>
> Thomas Lübking wrote:
> you can simply override a protected or public function by just adding the exact same signature to the derived class, BUT:
>
> KAction *ka = new KAction(this);
> QAction *qa = ka;
> ka->setText("foo"); // calls your reimplementation
> qa->setText("foo"); // calls the QAction function, despite qa == ka, ie. is actually KAction
>
> to have the above example work, the function needed to be virtual (and that means virtual in the base class, ie. QAction)
> Q_SLOTS are usually not virtual, but QMetaObject::invokeMethod("slotFunction") will resolve the best matching ::slotFunction() for the object (though by runtime resolution)
>
> René J.V. Bertin wrote:
> (Q_SLOTS are usually static member functions, no?)
>
> I know you can override a function through a derived class, and still invoke the overriden version, but that's irrelevant in a situation where there is no derived (child) class. As when KAction is removed, leaving KF5 apps with just QAction. That's why I specifically asked a about overriding a function without deriving the class...
> Q_SLOTS are usually static member functions, no?
No.
> That's why I specifically asked
Misunderstanding.
No, you can't. You can trick the linker in resolving a function from another shared object (QAction replacement in KDE library) and ignoring the other variant, but that's obviously not an option.
If this doesn't improve in Qt for you, you'd end up w/ a watchdog object to track QAction text changes and fix the "broken" text based Qt role assignment.
- Thomas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120149/#review66546
-----------------------------------------------------------
On Sept. 15, 2014, 11:03 vorm., René J.V. Bertin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120149/
> -----------------------------------------------------------
>
> (Updated Sept. 15, 2014, 11:03 vorm.)
>
>
> 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/actions/kstandardaction.cpp 7de0c6f
> kdeui/notifications/kstatusnotifieritem.cpp 1b15d40
> kdeui/widgets/kmenu.h f96e263
> kdeui/widgets/kmenu.cpp 7dab149
>
> 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/20140916/96b29c6a/attachment.htm>
More information about the kde-core-devel
mailing list