> On Okt. 29, 2014, 11:57 vorm., Kai Uwe Broulik wrote: > > > > Martin Gräßlin wrote: > @Kai Uwe: may I ask you to do such a review for all other existing window > switchers? Or just have a look at them and improve as you seem fit.
Okay, once they've all landed in kdeplasma-addons I will take a look. > On Okt. 29, 2014, 11:57 vorm., 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? > > Andre Heinecke wrote: > I use both functions more then once. > If i did it declartively I had to duplicate the code, right? I was more thinking of something like width: Math.min(textItem.contentWidth, scalingTabBox.width - captionFrame.anchors.leftMargin - captionFrame.anchors.rightMargin - captionIconItem.width * 2 - captionFrame.anchors.rightMargin) instead of manually invoking that method whenever the width or text changes and let QML figure that out for us automagically. > On Okt. 29, 2014, 11:57 vorm., 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? > > Andre Heinecke wrote: > 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. You cannot assume that a language uses exactly this kind of braces in that order: i18nc("braces just denote the window is minimized", "(%1)", text) > On Okt. 29, 2014, 11:57 vorm., 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 > > Andre Heinecke wrote: > The height is assigned in the states. > Is there an advantage to set one initially? Sorry, I missed that. Fine then. > On Okt. 29, 2014, 11:57 vorm., 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? :/ > > Andre Heinecke wrote: > 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". Right, we need to ask VDG/HIG about that later then. > On Okt. 29, 2014, 11:57 vorm., 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 > > Andre Heinecke wrote: > 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) I just forwarded that comment from David who complained about that in a different RR :) > On Okt. 29, 2014, 11:57 vorm., 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 > > Andre Heinecke wrote: > 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. Fair enough. Needs investigation/fixing in the others as well, Martin G said it might be needed by KWin internally. > On Okt. 29, 2014, 11:57 vorm., 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 > > Andre Heinecke wrote: > 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. 5 could be units.smallSpacing or so I guess - Kai Uwe ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/109832/#review69422 ----------------------------------------------------------- On Okt. 29, 2014, 3:22 nachm., Andre Heinecke wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/109832/ > ----------------------------------------------------------- > > (Updated Okt. 29, 2014, 3:22 nachm.) > > > 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
