> On Sept. 15, 2014, 2:19 p.m., Thomas Lübking wrote: > > kdeui/widgets/kmenu.cpp, line 174 > > <https://git.reviewboard.kde.org/r/120149/diff/2/?file=312033#file312033line174> > > > > this is *utterly* wrong - you're manipulating a QAction reference just > > because it (at this very time!) hints it won't show it's icon in menus? > > > > The only sane approach is to fix the menu to honor this value and if > > that's not possible, clone the action (w/o icon), watch the original one > > for changed() and destroyed() for aligning updates and forward the clones > > signals to the original one. > > René J.V. Bertin wrote: > I don't understand at all what you're trying to say. > > Do you mean that the user could change `isIconVisibleInMenu()` while the > app is running, and that the change should be apparent in the menu without > relaunching the application, or recreating the menu? > If so, is that even possible on OS X? (I could check but I have no idea > what kind of application might do such a thing...) > And if that's indeed what you mean, what do you think is worse - Qt > ignoring `isIconVisibleInMenu()` altogether, or me not supporting potential > changes to the flag and/or icon? > > I do agree that my code changes the action object the user passes in. The > thing is that it's probably not possible simply to clone the action and add > the clone, because then everything the user does to his/her copy after adding > it will have no effect ...
I've played around a bit with Qt's systray example. Here are my findings: - In Qt4, `isIconVisibleInMenu` is ignored completely for system tray menus, be it when adding a QAction or when changing the property later. - Icon changes are respected. - In Qt 5.3, `isIconVisibleInMenu` is respected when adding a QAction to the system tray menu, but subsequent changes to it have no effect. - Icon changes are respected, unless `isIconVisibleinMenu` has been turned off, in which case the previously show icon remains visible. Based on these findings, I think someone could file an upstream bug report for Qt5, but for KDE/Qt4 it seems that we can stick with my proposal. With the current state of things, the user will have to restart the application if s/he wants to change icon visibility in the systray menu, whether my patch is applied or not (actually, without my patch visibility will never change...) - René J.V. ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120149/#review66546 ----------------------------------------------------------- On Sept. 15, 2014, 1:03 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. 15, 2014, 1:03 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/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 > >
