<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/103004/">http://git.reviewboard.kde.org/r/103004/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 1st, 2011, 2:05 p.m., <b>Aaron J. Seigo</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/103004/diff/1/?file=39919#file39919line135" style="color: black; font-weight: bold; text-decoration: underline;">libs/taskmanager/taskactions.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">bool ToolTipMenu::event(QEvent* e)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">135</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span><span class="p">(</span><span class="n">act</span><span class="o">-></span><span class="n">property</span><span class="p">(</span><span class="s">"show-tooltip"</span><span class="p">).</span><span class="n">toBool</span><span class="p">())</span> <span class="p">{</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">why a property for this? why not simply:
if (!act->toolTip().isEmpty()) {</pre>
</blockquote>
<p>On November 1st, 2011, 2:42 p.m., <b>Craig Drummond</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Using "if (!act->toolTip().isEmpty())" results in the menuitem's text being shown as the tooltip - which is pretty silly.
I've also tried "if(!act->toolTip().isEmpty() && act->toolTip()!=act->text())" but this again fails if the menuitem has an underscore shortcut.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">well, this is ugly ugly but it works and doesn't require any special settings in the QActions that get fed to ToolTipMenu.. it's borrowed from qaction.cpp in Qt:
static QString qt_strippedText(QString s)
{
s.remove( QString::fromLatin1("...") );
int i = 0;
while (i < s.size()) {
++i;
if (s.at(i-1) != QLatin1Char('&')) {
continue;
}
if (i < s.size() && s.at(i) == QLatin1Char('&')) {
++i;
}
s.remove(i-1,1);
}
return s.trimmed();
}
with that function in hand: qt_strippedText(action->text()) == action->toolTip();
it's not pretty, but it works and renders it an internal ugliness instead of something exposed in every QAction that is passed to libtaskmanager.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 1st, 2011, 2:05 p.m., <b>Aaron J. Seigo</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/103004/diff/1/?file=39919#file39919line384" style="color: black; font-weight: bold; text-decoration: underline;">libs/taskmanager/taskactions.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">ToggleLauncherActionImpl::ToggleLauncherActionImpl(QObject *parent, AbstractGroupableItem *item, GroupManager *strategy)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">352</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">setText</span><span class="p">(</span><span class="n">i18n</span><span class="p">(</span><span class="s">"&Show A Launcher <span class="hl">For %1 </span>When<span class="hl"> It Is</span> Not Running"</span><span class="p"><span class="hl">,</span></span><span class="hl"> </span><span class="n"><span class="hl">m_name</span></span><span class="p">));</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">381</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">setText</span><span class="p">(</span><span class="n">i18n</span><span class="p">(</span><span class="s">"&Show A Launcher When Not Running"</span><span class="p">));</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">we have a name, it should be used. it hardly matters what the eventual Launcher is called. shortening it with the removal of "It Is" is probably alright ("computer application english" :)</pre>
</blockquote>
<p>On November 1st, 2011, 2:42 p.m., <b>Craig Drummond</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">"Show A Launcher for Systemsettings When It is Not Running" is a *very* long menu item. What advantage does adding the name here give? The user already right-clicked on the taskbar entry - so they know what the item is. Placing its name here just adds extra noise. In fact I'd actually prefer just "Pin To Taskbar" and "Unpin From Taskbar"</pre>
</blockquote>
<p>On November 1st, 2011, 5:02 p.m., <b>Aaron J. Seigo</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">a) it tells the user what is (likely) to happen with the launcher; as it doesn't actually relate directly to the window itself, this is useful information .. particularly as many times you can't even see the name of the application in the task widget button, so have nothing to really reference visually.
b) "pin" has got to be one of the worst bits of new jargon that has come with docks. in my testing with run-of-the-mill users not used to docks they tend to fail with complete reliability at explaining what it will do
it could be shortened to just "Show a launcher for <name>" though? </pre>
</blockquote>
<p>On November 2nd, 2011, 7:53 a.m., <b>Craig Drummond</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I still disagree with adding the name. We dont have "Close <name>", "Minimise <name>". So why is "Show A Launcher" any different?</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">because close, minimize, etc. all operate on the window, while the launcher operates on something that isn't necessarily the window.
but since we're going back and forth on this point, i'll compromise on this point: let's try it your way without a name :)</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 1st, 2011, 2:05 p.m., <b>Aaron J. Seigo</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/103004/diff/1/?file=39919#file39919line508" style="color: black; font-weight: bold; text-decoration: underline;">libs/taskmanager/taskactions.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">EditGroupActionImpl::EditGroupActionImpl(QObject *parent, TaskGroup *group, GroupManager *groupManager)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">505</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span><span class="p">(</span><span class="o">!</span><span class="n">simple</span><span class="p">)</span> <span class="p">{</span></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">440</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">addMenu</span><span class="p">(</span><span class="k">new</span> <span class="n">AdvancedMenu</span><span class="p">(</span><span class="k">this</span><span class="p">,</span> <span class="n">item</span><span class="p">,</span> <span class="n">strategy</span><span class="p">));</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">506</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">addMenu</span><span class="p">(</span><span class="k">new</span> <span class="n">AdvancedMenu</span><span class="p">(</span><span class="k">this</span><span class="p">,</span> <span class="n">item</span><span class="p">,</span> <span class="n">strategy</span><span class="p">));</span></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">507</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="p">}</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">there's a line between "simplifiying" and "making it useless in too many cases" that this crosses imho. this would have me reaching for the window titlebar in too many cases.
i would be in favour of renaming "Advanced" (a word we are trying to get away from) with something like "More actions"</pre>
</blockquote>
<p>On November 1st, 2011, 2:42 p.m., <b>Craig Drummond</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">hehe! knew this would get some reactions. Its only an option anyway. Perhaps it would be better if the user could supply a mask of items they did *not* want in the basic menu? Then it's up to the taskbar applet to decide what to show?</pre>
</blockquote>
<p>On November 1st, 2011, 5:02 p.m., <b>Aaron J. Seigo</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">sounds like a great recipe for inconsistency, poor choices and the creation of new user-facing options. i'd really like to avoid it. coming to a good compromise situation that works well enough for everyone should be possible.</pre>
</blockquote>
<p>On November 2nd, 2011, 7:53 a.m., <b>Craig Drummond</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">In the next diff I've removed the 'simple' option. It needs more thought. Plus, it might be better to just re-think the context menu so that there is no need for a 'simple' mode.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">i agree completely about re-thinking the context menu. let's get this patch in, and then after that we can work on making the menu Not Suck(tm)</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 1st, 2011, 2:05 p.m., <b>Aaron J. Seigo</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/103004/diff/1/?file=39919#file39919line600" style="color: black; font-weight: bold; text-decoration: underline;">libs/taskmanager/taskactions.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void BasicMenu::addAppActions(QList <QAction*> &appActions)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">595</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QString</span> <span class="n">title</span><span class="o">=</span><span class="n">action</span><span class="o">-></span><span class="n">property</span><span class="p">(</span><span class="s">"action-group-name"</span><span class="p">).</span><span class="n">toString</span><span class="p">();</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">this is a misuse of QAction. you can add QActions to a QAction and that parent QAction becomes the submenu. no need for such custom property setting/reading and menu creation.</pre>
</blockquote>
<p>On November 1st, 2011, 2:42 p.m., <b>Craig Drummond</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This was for dock-manager plugin support - just to make things easier on my side - then there's a 1:1 mapping between the dockmanager item and the QAction. No biggie, I can always remove this.</pre>
</blockquote>
<p>On November 1st, 2011, 5:02 p.m., <b>Aaron J. Seigo</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">hm... can't you add all the dockmanager items to a common QAction? should be no harder than calling setProperty on them? or are these properties already set by the dockmanager API? (in which case i'd suggest the dock manager API could use improving :)</pre>
</blockquote>
<p>On November 1st, 2011, 7:45 p.m., <b>Craig Drummond</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Hmm... How do I add a QAction to a QAction? I cant see this anywhere in the documentation. I've tried QActionGroup, creating QActions with the QAction as a parent. All failed :-(</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">QMenu *menu = new QMenu(parent);
QAction *action = menu->menuAction();
you can then add actions to the menu, and return the menuAction() as the action for the menu. it's not the most intuitive thing (almost backwards imho :) but it works nicely. we use it in several places in plasma to achieve exactly this effect :)
</pre>
<br />
<p>- Aaron J.</p>
<br />
<p>On October 31st, 2011, 8:40 p.m., Craig Drummond wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Plasma.</div>
<div>By Craig Drummond.</div>
<p style="color: grey;"><i>Updated Oct. 31, 2011, 8:40 p.m.</i></p>
<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;">1. Add NewInstance action to launch a new instance of an application.
2. Move toggle launcher action out of advanced menu.
3. Don't show application name in 'Show A Launcher' action, as might not know the real name at this time (e.g. no desktop file read).
4. Add application actions, to be shown at the top of the right-click menu. IconTasks uses this to show recent docuents, dock manager menu items, and unity items.
5. A ToolTipMenu class is created, so that tooltips may be give for menu items. This is so that IconTasks can display the full path of a documents in the recent documents menu.
6. Add option to have only basic window controls in menu.
</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>libs/taskmanager/taskactions.h <span style="color: grey">(2b5a641)</span></li>
<li>libs/taskmanager/taskactions.cpp <span style="color: grey">(0e6ba8e)</span></li>
<li>libs/taskmanager/taskactions_p.h <span style="color: grey">(913966c)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/103004/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>