----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/109832/#review69422 -----------------------------------------------------------
tabbox/qml/clients/scaling/contents/ui/main.qml <https://git.reviewboard.kde.org/r/109832/#comment48537> Is this needed? Adds a bit of overhead and you're not using it consistently everywhere anyway tabbox/qml/clients/scaling/contents/ui/main.qml <https://git.reviewboard.kde.org/r/109832/#comment48538> Don't hardcode sizes, use units.gridUnit et al You also might want to make those properties readonly tabbox/qml/clients/scaling/contents/ui/main.qml <https://git.reviewboard.kde.org/r/109832/#comment48539> Those don't seem t be used tabbox/qml/clients/scaling/contents/ui/main.qml <https://git.reviewboard.kde.org/r/109832/#comment48540> Be careful with clipping, it's quite expensive tabbox/qml/clients/scaling/contents/ui/main.qml <https://git.reviewboard.kde.org/r/109832/#comment48541> You don't need ; in qml definitions tabbox/qml/clients/scaling/contents/ui/main.qml <https://git.reviewboard.kde.org/r/109832/#comment48542> Then use FrameSvg not FrameSvgItem tabbox/qml/clients/scaling/contents/ui/main.qml <https://git.reviewboard.kde.org/r/109832/#comment48544> Is the lagging behind highlight really needed? :/ tabbox/qml/clients/scaling/contents/ui/main.qml <https://git.reviewboard.kde.org/r/109832/#comment48543> Margins only apply to corners where it's anchored to tabbox/qml/clients/scaling/contents/ui/main.qml <https://git.reviewboard.kde.org/r/109832/#comment48545> Put id at the beginning of the item tabbox/qml/clients/scaling/contents/ui/main.qml <https://git.reviewboard.kde.org/r/109832/#comment48546> Doesn't have a height tabbox/qml/clients/scaling/contents/ui/main.qml <https://git.reviewboard.kde.org/r/109832/#comment48547> property variant is obsolete, use property var instead tabbox/qml/clients/scaling/contents/ui/main.qml <https://git.reviewboard.kde.org/r/109832/#comment48548> spaces, foo * (1.0 / bar) tabbox/qml/clients/scaling/contents/ui/main.qml <https://git.reviewboard.kde.org/r/109832/#comment48549> You don't need the {} when it's just one line tabbox/qml/clients/scaling/contents/ui/main.qml <https://git.reviewboard.kde.org/r/109832/#comment48558> visible: false (space) tabbox/qml/clients/scaling/contents/ui/main.qml <https://git.reviewboard.kde.org/r/109832/#comment48557> It's not anchored to the bottom, making the margin have no effect tabbox/qml/clients/scaling/contents/ui/main.qml <https://git.reviewboard.kde.org/r/109832/#comment48550> Use units.iconSizes.xxx tabbox/qml/clients/scaling/contents/ui/main.qml <https://git.reviewboard.kde.org/r/109832/#comment48556> Please check the order, it's all higgledy-piggledy tabbox/qml/clients/scaling/contents/ui/main.qml <https://git.reviewboard.kde.org/r/109832/#comment48555> font.bold: true tabbox/qml/clients/scaling/contents/ui/main.qml <https://git.reviewboard.kde.org/r/109832/#comment48551> Could this be done in a declarative, rather than imperative, way? tabbox/qml/clients/scaling/contents/ui/main.qml <https://git.reviewboard.kde.org/r/109832/#comment48552> /** tabbox/qml/clients/scaling/contents/ui/main.qml <https://git.reviewboard.kde.org/r/109832/#comment48554> This could be inlined in the label above text: { the code } tabbox/qml/clients/scaling/contents/ui/main.qml <https://git.reviewboard.kde.org/r/109832/#comment48553> i18n this? - Kai Uwe Broulik On Okt. 29, 2014, 11:38 vorm., Andre Heinecke wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/109832/ > ----------------------------------------------------------- > > (Updated Okt. 29, 2014, 11:38 vorm.) > > > 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
