<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/107160/">http://git.reviewboard.kde.org/r/107160/</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 6th, 2012, 1:39 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;">one small issue with where hiding status is kept.
it may be useful to keep in mind the difference between the data and the visualization -> anything that the use can see (e.g. whether an icon is hidden or not) is visualization and must not be in the data classes; but any data that is used to populate the visualization (e.g. the current attention status of an item) should be handled by the data classes. this data/visualization split is common in plasma, and once one gets used to it things become easier :)
anyways.. this one small change of moving the hiding status from Task to WidgetItem and this can be pushed into master :)
thanks ...</pre>
</blockquote>
<p>On November 7th, 2012, 6:53 a.m., <b>Dmitry Ashkadov</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;">Moving hiding property from Task to WidgetItem won't work, because it is used in QML to determine the location/area where the task will be placed (popup dialog or panel).</pre>
</blockquote>
<p>On November 7th, 2012, 6:55 a.m., <b>Dmitry Ashkadov</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;">And this is a reason to create UiTask for visualization: Task - data, UiTask - visualization</pre>
</blockquote>
<p>On November 7th, 2012, 12:06 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;">WidgetItem is that same thing, however.
right now in the code there is the getLocationForTask function. it does this:
var loc = getDefaultLocationForTask(task)
if (loc === JS.LOCATION_TRAY && task.typeId == JS.TASK_NOTIFICATIONS_TYPEID)
return JS.LOCATION_NOTIFICATION // redefine location for notifications applet
return loc
getDefaultLocationForTask is:
/// Returns location depending on status and hide state of task
function getDefaultLocationForTask(task) {
if (task.status === TaskStatusAttention || task.hideState === TaskHideStateShown) return JS.LOCATION_TRAY
if (task.hideState === TaskHideStateHidden || (task.status !== TaskStatusActive && task.status !== TaskStatusUnknown)) {
return JS.LOCATION_POPUP
}
return JS.LOCATION_TRAY
}
in all cases where this is used, the code already has a WidgetItem or a StatusNotifierItem. so getDefaultLocationForTask could just as easily take a WidgetItem or a StatusNotifierItem and these (C++) classes could provide the same function as Task::visibilityPreference().
the onVisibilityPreference signal is also used only in WidgetItem and StatusNotifierItem .. so they could just as well do that internally in the C++. i don't see anywhere that needs this information that does not already have a WidgetItem or a StatusNotifierItem?</pre>
</blockquote>
<p>On November 7th, 2012, 12:31 p.m., <b>Dmitry Ashkadov</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;">QML is intended to provide a simple way to describe GUI and GUI logic. No more.
C++ code provides user's preference about visibility of task: he can want to see item always or not. But the real responsibility for visibility of task belongs to GUI logic (QML code). QML code handles changing of status of task, takes a decision on location (popup/panel) of task. GUI part/QML code is a representation of tray tasks and it may not take into account user's preference at all. So, I don't want split GUI logic, I don't want duplicate the same code in StatusNotifierItem and WidgetItem. Moreover StatusNotifierItem is implemented in QML.
I think C++ should provide user's preference and signal if he changes his preference.</pre>
</blockquote>
<p>On November 7th, 2012, 3:27 p.m., <b>Dmitry Ashkadov</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 think Plasmoid class may be merged to Applet. If formFactor and location properties were implemented in Plasma::Applet, I would remove them from Plasmoid class during merging.</pre>
</blockquote>
<p>On November 7th, 2012, 4:54 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;">"I don't want duplicate the same code in StatusNotifierItem and WidgetItem."
they could share the code by creating a common super-class that they both subclass. alternatively, you could create a small C++ class that only tracks this one visibility property and create one as a property in each StatusNotifierItem or WidgetItem
"Moreover StatusNotifierItem is implemented in QML."
the visbility property class would resolve that; or it can be done in C++ as WidgetItem was. probably the visbility property object is easiest and the overhead is pretty low.
"I don't want split GUI logic"
sorry, but this is not optional. putting this GUI property in Task breaks the system tray as you can no longer have more than one plasmoid using Manager with correct behaviour.
"If formFactor and location properties were implemented in Plasma::Applet"
these will be available in libplasma2 as properties; but for now you can add those properties to Applet in applet.h and they will use the Plasma::Applet methods automatically.
</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">What do you think about current solution? Should I ship it?</pre>
<br />
<p>- Dmitry</p>
<br />
<p>On November 7th, 2012, 3:20 p.m., Dmitry Ashkadov 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, Aaron J. Seigo and Marco Martin.</div>
<div>By Dmitry Ashkadov.</div>
<p style="color: grey;"><i>Updated Nov. 7, 2012, 3:20 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;">Aaron has noticed in his mail to plasma-devel that the system tray requires some refactoring. This is a first step of refactoring. If it is approved I'll continue work on system tray.</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>plasma/generic/applets/systemtray/CMakeLists.txt <span style="color: grey">(d3478a8)</span></li>
<li>plasma/generic/applets/systemtray/core/task.h <span style="color: grey">(66bf6e1)</span></li>
<li>plasma/generic/applets/systemtray/core/task.cpp <span style="color: grey">(8754785)</span></li>
<li>plasma/generic/applets/systemtray/package/contents/code/main.js <span style="color: grey">(10518cd)</span></li>
<li>plasma/generic/applets/systemtray/package/contents/ui/IconsList.qml <span style="color: grey">(f251cc5)</span></li>
<li>plasma/generic/applets/systemtray/package/contents/ui/StatusNotifierItem.qml <span style="color: grey">(27d476a)</span></li>
<li>plasma/generic/applets/systemtray/package/contents/ui/main.qml <span style="color: grey">(f80abc7)</span></li>
<li>plasma/generic/applets/systemtray/protocols/dbussystemtray/dbussystemtraytask.h <span style="color: grey">(34aae74)</span></li>
<li>plasma/generic/applets/systemtray/protocols/fdo/fdotask.h <span style="color: grey">(85a9ec5)</span></li>
<li>plasma/generic/applets/systemtray/protocols/plasmoid/plasmoidtask.h <span style="color: grey">(1913986)</span></li>
<li>plasma/generic/applets/systemtray/ui/applet.h <span style="color: grey">(bab8d9c)</span></li>
<li>plasma/generic/applets/systemtray/ui/applet.cpp <span style="color: grey">(41efb10)</span></li>
<li>plasma/generic/applets/systemtray/ui/mouseredirectarea.h <span style="color: grey">(91c40c0)</span></li>
<li>plasma/generic/applets/systemtray/ui/mouseredirectarea.cpp <span style="color: grey">(aac2a53)</span></li>
<li>plasma/generic/applets/systemtray/ui/plasmoid.h <span style="color: grey">(01a7c5b)</span></li>
<li>plasma/generic/applets/systemtray/ui/plasmoid.cpp <span style="color: grey">(d3e1937)</span></li>
<li>plasma/generic/applets/systemtray/ui/taskspool.h <span style="color: grey">(4b5bcd4)</span></li>
<li>plasma/generic/applets/systemtray/ui/taskspool.cpp <span style="color: grey">(df39e3d)</span></li>
<li>plasma/generic/applets/systemtray/ui/uitask.h <span style="color: grey">(7b20bde)</span></li>
<li>plasma/generic/applets/systemtray/ui/uitask.cpp <span style="color: grey">(2a15800)</span></li>
<li>plasma/generic/applets/systemtray/ui/widgetitem.h <span style="color: grey">(40aa92d)</span></li>
<li>plasma/generic/applets/systemtray/ui/widgetitem.cpp <span style="color: grey">(9d2c622)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/107160/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>