Review Request 109832: New tabbox layout with scaling thumbnails

Wed Oct 29 14:20:29 UTC 2014

> On Oct. 29, 2014, 11:57 a.m., Kai Uwe Broulik wrote:
> > tabbox/qml/clients/scaling/contents/ui/main.qml, lines 31-32
> > <https://git.reviewboard.kde.org/r/109832/diff/3/?file=321190#file321190line31>
> >
> >     Is this needed? Adds a bit of overhead and you're not using it consistently everywhere anyway

Replaced through direct usage. Was an artifact taken over from thumbnails switcher.

> On Oct. 29, 2014, 11:57 a.m., Kai Uwe Broulik wrote:
> > tabbox/qml/clients/scaling/contents/ui/main.qml, lines 75-76
> > <https://git.reviewboard.kde.org/r/109832/diff/3/?file=321190#file321190line75>
> >
> >     Those don't seem t be used

Removed

> On Oct. 29, 2014, 11:57 a.m., Kai Uwe Broulik wrote:
> > tabbox/qml/clients/scaling/contents/ui/main.qml, line 79
> > <https://git.reviewboard.kde.org/r/109832/diff/3/?file=321190#file321190line79>
> >
> >     Be careful with clipping, it's quite expensive

Not sure why there is clipping here. I took this over from thumbnails layout and thought it would be there for a reason.
Removed now.

> On Oct. 29, 2014, 11:57 a.m., Kai Uwe Broulik wrote:
> > tabbox/qml/clients/scaling/contents/ui/main.qml, line 88
> > <https://git.reviewboard.kde.org/r/109832/diff/3/?file=321190#file321190line88>
> >
> >     You don't need ; in qml definitions

Removed.

> On Oct. 29, 2014, 11:57 a.m., Kai Uwe Broulik wrote:
> > tabbox/qml/clients/scaling/contents/ui/main.qml, line 132
> > <https://git.reviewboard.kde.org/r/109832/diff/3/?file=321190#file321190line132>
> >
> >     Then use FrameSvg not FrameSvgItem

Not sure what you mean here. Should I just take the margins from FrameSvg as the margins of the hover effect?
I understood this code to create a hover item to get the margins of that item. (Taken over from Thumbnails)

> On Oct. 29, 2014, 11:57 a.m., Kai Uwe Broulik wrote:
> > tabbox/qml/clients/scaling/contents/ui/main.qml, line 147
> > <https://git.reviewboard.kde.org/r/109832/diff/3/?file=321190#file321190line147>
> >
> >     Is the lagging behind highlight really needed? :/

I find 250 looks smooth enough. And it's the same as all other window tabbox plugins use so I would also prefer to use the same value here.
Longer and it gets sluggish. Shorter and it looks "jumpy".

> On Oct. 29, 2014, 11:57 a.m., Kai Uwe Broulik wrote:
> > tabbox/qml/clients/scaling/contents/ui/main.qml, lines 153-155
> > <https://git.reviewboard.kde.org/r/109832/diff/3/?file=321190#file321190line153>
> >
> >     Margins only apply to corners where it's anchored to

Removed those margins.

> On Oct. 29, 2014, 11:57 a.m., Kai Uwe Broulik wrote:
> > tabbox/qml/clients/scaling/contents/ui/main.qml, line 162
> > <https://git.reviewboard.kde.org/r/109832/diff/3/?file=321190#file321190line162>
> >
> >     Put id at the beginning of the item

Done

> On Oct. 29, 2014, 11:57 a.m., Kai Uwe Broulik wrote:
> > tabbox/qml/clients/scaling/contents/ui/main.qml, line 163
> > <https://git.reviewboard.kde.org/r/109832/diff/3/?file=321190#file321190line163>
> >
> >     Doesn't have a height

The height is assigned in the states.
Is there an advantage to set one initially?

> On Oct. 29, 2014, 11:57 a.m., Kai Uwe Broulik wrote:
> > tabbox/qml/clients/scaling/contents/ui/main.qml, lines 167-168
> > <https://git.reviewboard.kde.org/r/109832/diff/3/?file=321190#file321190line167>
> >
> >     property variant is obsolete, use property var instead

changed

> On Oct. 29, 2014, 11:57 a.m., Kai Uwe Broulik wrote:
> > tabbox/qml/clients/scaling/contents/ui/main.qml, line 170
> > <https://git.reviewboard.kde.org/r/109832/diff/3/?file=321190#file321190line170>
> >
> >     spaces, foo * (1.0 / bar)

changed

> On Oct. 29, 2014, 11:57 a.m., Kai Uwe Broulik wrote:
> > tabbox/qml/clients/scaling/contents/ui/main.qml, line 36
> > <https://git.reviewboard.kde.org/r/109832/diff/3/?file=321190#file321190line36>
> >
> >     Don't hardcode sizes, use units.gridUnit et al
> >     
> >     You also might want to make those properties readonly

I've changed all hardcoded values (there were some hardcoded 5's where i meant icon spacing) to use properties and the basic size properties are now based on units.

> On Oct. 29, 2014, 11:57 a.m., Kai Uwe Broulik wrote:
> > tabbox/qml/clients/scaling/contents/ui/main.qml, lines 375-376
> > <https://git.reviewboard.kde.org/r/109832/diff/3/?file=321190#file321190line375>
> >
> >     Could this be done in a declarative, rather than imperative, way?

I use both functions more then once.
If i did it declartively I had to duplicate the code, right?

> On Oct. 29, 2014, 11:57 a.m., Kai Uwe Broulik wrote:
> > tabbox/qml/clients/scaling/contents/ui/main.qml, line 402
> > <https://git.reviewboard.kde.org/r/109832/diff/3/?file=321190#file321190line402>
> >
> >     This could be inlined in the label above
> >     text: {
> >       the code
> >     }

Right. simplified this to: minimized ? "(" + caption + ")" : caption

> On Oct. 29, 2014, 11:57 a.m., Kai Uwe Broulik wrote:
> > tabbox/qml/clients/scaling/contents/ui/main.qml, line 405
> > <https://git.reviewboard.kde.org/r/109832/diff/3/?file=321190#file321190line405>
> >
> >     i18n this?

I don't think this is neccessary. The braces are just used to indicate that a window is minimized. They have no lingustic meaning in this context.

- Andre

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/109832/#review69422
-----------------------------------------------------------

On Oct. 29, 2014, 11:38 a.m., Andre Heinecke wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/109832/
> -----------------------------------------------------------
> 
> (Updated Oct. 29, 2014, 11:38 a.m.)
> 
> 
> Review request for kwin, Plasma and Martin Gräßlin.
> 
> 
> Bugs: 292566
>     http://bugs.kde.org/show_bug.cgi?id=292566
> 
> 
> Repository: kde-workspace
> 
> 
> Description
> -------
> 
> I'm reopening this review request as I have updated this Window Switcher for Plasma 5.1 and would like to get another review to check if there are any suggestions or issues regarding the port to the new API.
> 
> Secondly I would like to ask again to have this Window Switcher Layout included in the KWin repository. I would prefer it if users could obtain this layout from their trusted distributors and did not have to rely on an unverifyable third party download to obtain this plugin. 
> 
> As suggested in the original review I've put this up on kde-look and recieved some positive feedback. But I really feel that it is rotting away there and that KDE-Look is not a good place to distribute executable plugins.
> 
> IMHO the approach of this Window Switcher is different enough from the others included in KWin to be a useful addition to the fold. Especially as this behavior is already familiar to KDE users from some versions < 4.6
> 
> It should also be close enough to the other layouts like thumbnails to keep maintenance very similar (I've mostly looked at the changes made to thumbnails to adapt this for Plasma 5)
> 
> 
> Original description:
> 
> This Layout tries to mimic some of the old KDE 4.6 tabbox behavior and layout, it scales the thumbnails shown in the tabbox to avoid scrolling.
> There are also three different states in this layout depending on the size of the scaled thumbnails to provide appropriate information even when there are many opened windows.
> 
> States:
> 1. Thumbnails are larger then 200px: Show the Title and the Icon of the Window directly below the thumbnail.
> 2. Thumbnails are between 200px and 64px: Thumbnails are shown together with the icon but only the title of the currently selected window is shown.
> 3. Thumbnails would be smaller then 64px: Only the Icons of the windows are shown and the title of the currently selected window (like big icons)
> If the Thumbnails would be smaller then 32px the tabbox starts to scroll in the Icon Only state.
> 
> Size of the thumbnails depends on the screen size and the number of opened windows.
> 
> 
> Diffs
> -----
> 
>   tabbox/qml/CMakeLists.txt fc55ab9 
>   tabbox/qml/clients/scaling/contents/ui/main.qml PRE-CREATION 
>   tabbox/qml/clients/scaling/metadata.desktop PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/109832/diff/
> 
> 
> Testing
> -------
> 
> Tested with plasma 5.3.1 from Kubuntu next / unstable repositories.
> 
> 
> Thanks,
> 
> Andre Heinecke
> 
>

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