<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/106112/">http://git.reviewboard.kde.org/r/106112/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On February 9th, 2013, 6:49 p.m. UTC, <b>Mark Gaiser</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;">Just wanted to add that it has a regression that slipped in the 4.10.0 release. https://bugs.kde.org/show_bug.cgi?id=312684
If you have only one virtual desktop the pager just shows an empty area. That is obviously wrong. It is actually very clearly visible in this - long - review: http://www.muktware.com/5194/kde-410-review-time-switch-kde. Just look at the first image, the gap between activity icon and dolphin is the pager.</pre>
</blockquote>
<p>On February 11th, 2013, 9:02 p.m. UTC, <b>Luís Gabriel Lima</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;">Actually it's not a regression. As you can see in the history of this review, it was requested to hide the pager when there is only one virtual desktop. Although I can see a bug in the current implementation, the pager should be considerably small (practically hidden). Right now it's just making the virtual desktop rect invisible and keeping the same size.</pre>
</blockquote>
<p>On February 11th, 2013, 9:39 p.m. UTC, <b>Shaun Reich</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;">Oh it most certainly is on at least some level. Yes, if the pager is going to be hidden when one desktop is there, it shouldn't ghostily take up any room. But I don't think hiding the widget just because there is one desktop is a solution. In fact, I have seen users get confused around this, because their system is set to 1 VD and they add a pager widget, and what happens? Nothing. So they add it a few more times. Then they try adding it to a different panel, it still isn't working.
Turns out I had to tell them that they had to change the VD count to > 1, and surprise surprise! there's 20 pager widgets all over the desktop.
This is another case of someone trying to make something look clean and minimalistic, but not thinking about the consequences when it comes to user interaction.
If you don't like how the pager widget looks with 1 desktop, then don't ship it by default with 1 desktop, or don't ship the pager widget at all on a default desktop (which honestly I don't think many users(aka average) would use it in a default desktop).
Turning the pager widget into a ghost widget is definitely not the answer and we're going to get a lot of issues because of that, as I noted above, because I witnessed them first hand... Making it invisible really just pushes the confusion upward a bit more.</pre>
</blockquote>
<p>On February 12th, 2013, 10:03 a.m. UTC, <b>Mark Gaiser</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 understand your reasoning, but i don't think you should do this. In my opinion a plasmoid - and certainly one that's on the panel - should never be invisible but still take up room. If you do it you'd better show a big warning notifying the user of that. Or you should just prohibit the plasmoid if the user has just one virtual desktop.
Another usecase that apparently hasn't been considered is my usecase (obviously ;)). I have 3 "displays" attached. 2 monitors and one beamer. The beamer obviously isn't on by default yet it is possible that windows open in the beamer area. Thus i don't see those windows. If the pager would just show itself i would instantly notice that some windows where opened on a display that isn't turned on which would very easily allow me to drag those windows back to one of the enabled displays.
So yes, i think this is a regression.
Also a point of critic about the code. http://quickgit.kde.org/?p=kde-workspace.git&a=blob&h=228af8ec44f217553dec7ee7e10e1a0ffeda66dd&hb=1538ddeabae2488163f76a12080e6b152872f3a8&f=plasma%2Fdesktop%2Fapplets%2Fpager%2Fpager.cpp
m_dummy = new Plasma::FrameSvg(this);
m_dummy->setImagePath("widgets/pager");
Should be done through plasma QML.
void Pager::updatePagerStyle()
Should all be done through QML.
In short, all the painting - and how it should be painted - should be done through QML. So if you want to set a different opacity then you simply supply that value in the model and use it in QML. If you want to set the Z level then you also provide that through the model and set it that way. etcetera.. etcetera...
If you need help with some QML stuff, you can ask me.</pre>
</blockquote>
<p>On February 12th, 2013, 12:58 p.m. UTC, <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;">design discussion belongs on plasma-devel in a proper thread, not in the comments section of this review.
@Mark: the FrameSvg is not used for painting. the style information is made available to the QML via a property, where it used by the QML to paint and handle display. looking at the code, it seems that at least some of the values would require extensions to the color scheme support we currently offer to QML in Plasma Components. keep in mind that this is also a port from C++ to C++/QML; the effort to get rid of all the C++ would hardly be justified (in terms of effort/benefit) given that it would not provide any benefits that i can see. certainly, if this was to be written from scratch today we'd probably do it differently. we try to avoid wholesale rewrites, however, when not necessary.</pre>
</blockquote>
<p>On February 12th, 2013, 2:34 p.m. UTC, <b>Luís Gabriel Lima</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;">@Mark: Can you create a thread on plasma-devel to discuss the hidden problem?
About you critics, Aaron already have answered them properly. The FrameSvg is a dummy one (as its name suggests), and it's used to get the margins of the framesvg. We need the margins to calculate the correct size and positioning of the virtual desktop rects.</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;">Aaron and Luis, Oke, will do. It will be roughly the same as i said above minus the code critics.</pre>
<br />
<p>- Mark</p>
<br />
<p>On August 30th, 2012, 2:30 a.m. UTC, Luís Gabriel Lima wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Plasma and Marco Martin.</div>
<div>By Luís Gabriel Lima.</div>
<p style="color: grey;"><i>Updated Aug. 30, 2012, 2:30 a.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;">This patch contains the QML port of the Pager plasmoid done during the GSoC 2012.
In this port basically I used QML to paint the Pager UI and deal with the user interaction.
The geometry calculation of the desktop and window rectangles was kept in C++ as well as other routines that needs to interact with classes like KWindowSystem, QDbusConnection and so on.
This patch also introduces the PagerModel, a QAIM subclass that holds the desktop/window geometries and is used by the QML part to fill the UI.</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;">- Tested inside panels and floating on desktop, sizing works as expected
- Mouse interactions (move windows around, change desktop, etc)</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/desktop/applets/pager/CMakeLists.txt <span style="color: grey">(5d80514)</span></li>
<li>plasma/desktop/applets/pager/model.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>plasma/desktop/applets/pager/model.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>plasma/desktop/applets/pager/package/contents/ui/main.qml <span style="color: grey">(PRE-CREATION)</span></li>
<li>plasma/desktop/applets/pager/package/contents/ui/utils.js <span style="color: grey">(PRE-CREATION)</span></li>
<li>plasma/desktop/applets/pager/package/metadata.desktop <span style="color: grey">(PRE-CREATION)</span></li>
<li>plasma/desktop/applets/pager/pager.h <span style="color: grey">(6c7c045)</span></li>
<li>plasma/desktop/applets/pager/pager.cpp <span style="color: grey">(74dc529)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/106112/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Screenshots </h1>
<div>
<a href="http://git.reviewboard.kde.org/r/106112/s/691/"><img src="http://git.reviewboard.kde.org/media/uploaded/images/2012/08/21/pager_400x100.png" style="border: 1px black solid;" alt="" /></a>
<a href="http://git.reviewboard.kde.org/r/106112/s/692/"><img src="http://git.reviewboard.kde.org/media/uploaded/images/2012/08/21/pager-panel_400x100.png" style="border: 1px black solid;" alt="" /></a>
</div>
</td>
</tr>
</table>
</div>
</body>
</html>