Review Request 106112: Pager QML port

Tue Feb 12 12:58:26 UTC 2013

> On Feb. 9, 2013, 6:49 p.m., Mark Gaiser wrote:
> > 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.
> 
> Luís Gabriel Lima wrote:
>     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.
> 
> Shaun Reich wrote:
>     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.
> 
> Mark Gaiser wrote:
>     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.

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.

- Aaron J.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106112/#review27093
-----------------------------------------------------------

On Aug. 30, 2012, 2:30 a.m., Luís Gabriel Lima wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106112/
> -----------------------------------------------------------
> 
> (Updated Aug. 30, 2012, 2:30 a.m.)
> 
> 
> Review request for Plasma and Marco Martin.
> 
> 
> Description
> -------
> 
> 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.
> 
> 
> Diffs
> -----
> 
>   plasma/desktop/applets/pager/CMakeLists.txt 5d80514 
>   plasma/desktop/applets/pager/model.h PRE-CREATION 
>   plasma/desktop/applets/pager/model.cpp PRE-CREATION 
>   plasma/desktop/applets/pager/package/contents/ui/main.qml PRE-CREATION 
>   plasma/desktop/applets/pager/package/contents/ui/utils.js PRE-CREATION 
>   plasma/desktop/applets/pager/package/metadata.desktop PRE-CREATION 
>   plasma/desktop/applets/pager/pager.h 6c7c045 
>   plasma/desktop/applets/pager/pager.cpp 74dc529 
> 
> Diff: http://git.reviewboard.kde.org/r/106112/diff/
> 
> 
> Testing
> -------
> 
> - Tested inside panels and floating on desktop, sizing works as expected
> - Mouse interactions (move windows around, change desktop, etc)
> 
> 
> Screenshots
> -----------
> 
> 
>   http://git.reviewboard.kde.org/r/106112/s/691/
> 
>   http://git.reviewboard.kde.org/r/106112/s/692/
> 
> 
> Thanks,
> 
> Luís Gabriel Lima
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20130212/043fe82d/attachment-0001.html>