> On Sept. 15, 2014, 2:45 a.m., Ian Wadham wrote:
> > I cannot see how "static void setTextWithCorrectMenuRole()" could work in
> > all languages.
> >
> > Does not the "text" parameter come translated into the user's language, not
> > necessarily English? So how can the ".contains" checks work with all
> > languages?
> >
> > I see that the code does translate the string "Configure %1..." before
> > comparing it to "text". I am not sure that would work either if there are
> > non-standard spacings or punctuations in "text". That string is not a
> > hard-and-fast standard in all KDE apps, just a default that is provided by
> > KStandardAction or KStandardGameAction. The developer is quite at liberty
> > to change it to "<appname> Settings..." or "<appname> Game Settings...",
> > for example. Also some languages might have different orders of words or
> > numbers of words. I suppose Qt-Mac's QMenuBar class must have some way to
> > take care of that in this situation.
> >
> > Finally, if something other than a combination of "Configure" and <appname>
> > does match, should you leave it at QAction::NoRole? Maybe set
> > QAction::PreferencesRole or leave it at QAction::TextHeuristicRole and let
> > QMenuBar do its thing?
>
> René J.V. Bertin wrote:
> I've wondered about that myself, and was hoping to get constructive
> feedback here. I should have mentioned it, because I've never really done any
> "internationalised" coding...
>
> Qt's documentation for QMenuBar mentions it uses `QObject::tr` to
> translate the trigger patterns, but that only works for C strings. The text
> objects I'm scanning are already QStrings, and presumably either in a
> language-independent representation, or in English.
>
> Maybe I should do something like `text.contains( QObject::tr("config") )`
> ?
>
> Of course I'm not claiming that the present patch is a catch-all for all
> possible use cases. In fact, being a patch to the base library, it only
> pretends to catch standard use cases, and cases that correspond to those
> close enough (i.e. someone creating a "Configure <appname>..." action outside
> of KStandardAction). Qt tries to be more generic, catching the examples Ian
> gives, but it goes about it too randomly.
>
> @Ian: check my "companion" RR for KDevelop: it shows exactly what to do
> when this patch here isn't enough.
>
> And I could of course check what happens when I switch my own KDE
> environment to a different language ... I probably have the French language
> installed ...
>
> René J.V. Bertin wrote:
> Bummer, indeed, switching kmail to another language makes its "Configure
> KMail..." action pop back in the Settings menu instead of under the
> Preferences item. This is an example of an application for which the explicit
> check against "Configure <appname>..." is included.
>
> So ... how should I improve this patch to apply to all installed
> languages?
Here's what Qt does, in `qmenu_mac.mm` (that's ObjC++ ... best of both worlds,
sadly not recognised by KDevelop's parsers ...)
case QAction::TextHeuristicRole: {
QString aboutString = QMenuBar::tr("About").toLower();
if (t.startsWith(aboutString) || t.endsWith(aboutString)) {
if (t.indexOf(QRegExp(QString::fromLatin1("qt$"),
Qt::CaseInsensitive)) == -1) {
#ifndef QT_MAC_USE_COCOA
ret = kHICommandAbout;
#else
ret = [loader aboutMenuItem];
#endif
} else {
#ifndef QT_MAC_USE_COCOA
ret = kHICommandAboutQt;
#else
ret = [loader aboutQtMenuItem];
#endif
}
} else if (t.startsWith(QMenuBar::tr("Config").toLower())
|| t.startsWith(QMenuBar::tr("Preference").toLower())
|| t.startsWith(QMenuBar::tr("Options").toLower())
|| t.startsWith(QMenuBar::tr("Setting").toLower())
|| t.startsWith(QMenuBar::tr("Setup").toLower())) {
#ifndef QT_MAC_USE_COCOA
ret = kHICommandPreferences;
#else
ret = [loader preferencesMenuItem];
#endif
} else if (t.startsWith(QMenuBar::tr("Quit").toLower())
|| t.startsWith(QMenuBar::tr("Exit").toLower())) {
#ifndef QT_MAC_USE_COCOA
ret = kHICommandQuit;
#else
ret = [loader quitMenuItem];
#endif
}
- René J.V.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120149/#review66503
-----------------------------------------------------------
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
>
>
