<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>
</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;">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>
<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>