> On Sept. 17, 2014, 6:13 nachm., Pino Toscano wrote: > > kdeui/actions/kaction.cpp, lines 150-179 > > <https://git.reviewboard.kde.org/r/120149/diff/3/?file=312869#file312869line150> > > > > The whole setTextWithCorrectMenuRole is totally broken from an i18n > > point of view: > > - we don't use Qt's tr() in kdelibs > > - mess up an already translated string is totally no-no, you don't know > > what it will contain > > - you cannot translate single bits of messages as standalone messages, > > they can have a different declension > > > > Honestly, I don't see a way to make this marking as "preferences role" > > automatic, based on the action text. Unless there's something I'm missing, > > the only solution left is to mark such application-specific preferences > > actions as such. > > René J.V. Bertin wrote: > Please see my reply to Ian's comment all at the top. It contains a copy > of Qt's "heuristic" code. I copied that code in the you tripped over in order > to undo its (and only its) effect (i.e. to do a very targeted > `setMenuRole(NoRole)`. There is an additional step, an attempt to be able to > do `setMenuRole(PreferencesRole)` when appropriate. That's an attempt I made, > and I'll gladly accept that it's guaranteed not to work in all possible > languages. > > @pino: do your remarks apply to my bit of code only or also to the code > copied from Qt? I've opened a ticket about this whole "feature" but would > love to be able to make a stronger case than possible with my educated > guessing (pun not really intended). I'd also be fine with initialising the > menu role to `NoRole` (unconditionally or on OS X only), but that as long as > Qt doesn't incorporate this, that will only introduce a larger gap between > KDE4 and KF5 (where KAction is no longer there to override unwanted QAction > features). > > Note also that Qt5 actually *extends* the guesstimating to the Cut, Copy, > Paste and SelectAll menu roles ... > > Pino Toscano wrote: > Yes, I think that heuristic is plain wrong, whichever place it is. I > don't see "Qt does it" as reason to introduce *broken* code in kdelibs as > well. > > From what I see, KStandardAction::create sets proper roles for some kind > of actions, so either: > a) such "standard" action types get created using > KStandardAction::create, and them customizing the resulting action > b) actions have the proper role set manually > > René J.V. Bertin wrote: > We agree on that (indeed, items created through KStandardAction get the > appropriate role). I'm testing a version that simply does > `setMenuRole(NoRole)` in `setTextWithCorrectMenuRole()` to see what that > gives combined with my patched Qt version that doesn't guess AboutRole and > PreferencesRole. > > If that gives acceptable and expected behaviour, we'll have to sit back > and wait to see what Qt will do with my bug report, and then take proceed > from there. I presume there must be an appropriate place somewhere during the > KDE initialisation (KDE4 or KF5) where one can configure Qt, for instance? > > Thomas Lübking wrote: > > I presume there must be an appropriate place somewhere during the KDE > initialisation > > You'd have some Qt::AA_MacNoMurkyActionRoleHeuristics and set it on eg. > the KFrameworkIntegrationPlugin constructor - though maybe it should rather > only be done when calling KStandardActions::create(), to avoid hitting > applications which make no use of the standard actions. > > René J.V. Bertin wrote: > I'm guessing KFrameworkIntegrationPlugin is the KF5 name, there probably > is a KDE4 equivalent? > > I think it has to be called at the very beginning. Not just because > otherwise Qt may have already picked a PreferencesRole action before > KStandardActions items start getting created, but also because I fail to see > why one would let applications which make no use of standard actions be hit > with such actions by Qt ... After all, they either deal out MenuRoles > themselves, or they have a good reason not doing so.
kde-workspace/qguiplatformplugin_kde/qguiplatformplugin_kde.cpp - Thomas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120149/#review66764 ----------------------------------------------------------- On Sept. 17, 2014, 5:48 nachm., 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 nachm.) > > > 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 > >
