[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-pim
Subject: Re: Review Request 120149: [OS X] improved menubar experience: protected Preferences menu and cleane
From: René J.V. Bertin <rjvbertin () gmail ! com>
Date: 2017-02-08 12:47:45
Message-ID: 20170208124745.23155.11814 () mimi ! kde ! org
[Download RAW message or body]
--===============1786018441545820868==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120149/
-----------------------------------------------------------
(Updated Feb. 8, 2017, 12:47 p.m.)
Status
------
This change has been discarded.
Review request for KDE Software on Mac OS X, kdelibs, KDEPIM, Qt KDE, 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
--===============1786018441545820868==
MIME-Version: 1.0
Content-Type: text/html; charset="utf-8"
Content-Transfer-Encoding: 8bit
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 \
solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;"> \
<tr> <td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/120149/">https://git.reviewboard.kde.org/r/120149/</a>
</td>
</tr>
</table>
<br />
<table bgcolor="#e0e0e0" width="100%" cellpadding="12" style="border: 1px gray solid; \
border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;"> <tr>
<td>
<h1 style="margin: 0; padding: 0; font-size: 10pt;">This change has been \
discarded.</h1> </td>
</tr>
</table>
<br />
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: \
1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; \
-webkit-border-radius: 6px;"> <tr>
<td>
<div>Review request for KDE Software on Mac OS X, kdelibs, KDEPIM, Qt KDE, Marco \
Martin, and Olivier Goffart.</div> <div>By René J.V. Bertin.</div>
<p style="color: grey;"><i>Updated Feb. 8, 2017, 12:47 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdelibs
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" \
style="border: 1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">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.</p> <p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">-- the system tray: Until now, "system tray" menus had some rendering \
issues on Mac OS X:</p> <ul style="padding: 0;text-rendering: inherit;margin: 0 0 0 \
1em;line-height: inherit;white-space: normal;"> <li style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: normal;">The menu title, the 1st \
menu item that on Linux shows the application name, remained empty</li> <li \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
normal;">Menu items that can (in principle, potentially) show an icon always showed \
the icon</li> </ul>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">Point 1 was resolved by emulating the Linux \
addTitle/setTitle action in <code style="text-rendering: inherit;color: \
#4444cc;padding: 0;white-space: normal;margin: 0;line-height: \
inherit;">KStatusNotifierItemPrivate::init()</code> : 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.</p> <p style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: inherit;">Point 2 was identified as a Qt issue: \
<code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: \
normal;margin: 0;line-height: inherit;">isIconVisibleInMenu</code> is ignored for \
systray menus. It was resolved by adding <code style="text-rendering: inherit;color: \
#4444cc;padding: 0;white-space: normal;margin: 0;line-height: \
inherit;">KMenu::addAction</code> methods that overload the ones from QMenu that were \
hitherto inherited unchanged by KMenu. The only different method is <code \
style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: \
0;line-height: inherit;">addAction(QAction*)</code> which removes the icon from the \
<code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: \
normal;margin: 0;line-height: inherit;">QAction</code> if <code \
style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: \
0; line-height: inherit;">isIconVisibleInMenu()</code> is false. The other <code \
style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: \
0;line-height: inherit;">addAction</code> methods are "overloaded with themselves" \
with <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: \
normal;margin: 0;line-height: inherit;">using QMenu::addAction;</code> in the header \
file.</p> <p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">-- 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 \
<code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: \
normal;margin: 0;line-height: inherit;">config</code>, <code style="text-rendering: \
inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: \
inherit;">options</code>, <code style="text-rendering: inherit;color: \
#4444cc;padding: 0;white-space: normal;margin: 0;line-height: \
inherit;">settings</code> or <code style="text-rendering: inherit;color: \
#4444cc;padding: 0;white-space: normal;margin: 0;line-height: \
inherit;">preferences</code>, and put it under the Preferences label if its menu role \
is set to <code style="text-rendering: inherit;color: #4444cc;pad ding: \
0;white-space: normal;margin: 0;line-height: inherit;">heuristic</code> (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 <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: \
normal;margin: 0;line-height: inherit;">Configure Selection</code> action - which \
does not open a settings dialog but launches the configure or cmake procedure for the \
selected project ...</p> <p style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: inherit;">My proposed solution overrides this Qt \
behaviour. On OS X, the <code style="text-rendering: inherit;color: #4444cc;padding: \
0;white-space: normal;margin: 0;line-height: inherit;">KAction(const QString \
&text, QObject *parent)</code> constructor calls a modified (static) function \
<code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: \
normal;margin: 0;line-height: inherit;">setTextWithCorrectMenuRole</code> which \
checks the text against the patterns Qt will consider for <code \
style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: \
0;line-height: inherit;">PreferencesRole</code>. If it finds a match, it will force \
the role to <code style="text-rendering: inherit;color: #4444cc;padding: \
0;white-space: normal;margin: 0;line-height: inherit;">NoRole</code>, unless it is a \
perfect match with the standard KDE configuration action for the appl ication (<code \
style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: \
0;line-height: inherit;">"&Configuration appName..."</code>) in which case it \
sets the role to <code style="text-rendering: inherit;color: #4444cc;padding: \
0;white-space: normal;margin: 0;line-height: inherit;">PreferencesRole</code>. 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.</p> <p style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: inherit;">Applications that create menu actions \
using QAction or a different KAction constructor will see no change (and should use \
<code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: \
normal;margin: 0;line-height: inherit;">setMenuRole</code> selectively on OS \
X).</p></pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">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.</p> <p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">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.</p> <p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p> <p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">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 <code style="text-rendering: inherit;color: \
#4444cc;padding: 0;white-space: normal;margin: 0;line-height: \
inherit;">KAction</code> has made the transition to KF5.</p></pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>kdeui/actions/kaction.cpp <span style="color: grey">(9e8f7fb)</span></li>
<li>kdeui/notifications/kstatusnotifieritem.cpp <span style="color: \
grey">(1b15d40)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/120149/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
--===============1786018441545820868==--
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic