> 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 > >
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
