----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128950/#review99284 -----------------------------------------------------------
The problem with this patch (which highlights a mix we have in plasma in general) is that it's mixing whether the screen is managed by the view or managed by the containment. With this you're overriding the screen() method from containment, and manipulating things directly via the view. The issue this has is that it almost certainly won't work on any pre-startup plasma scripts, where we don't have a view yet. This current patch would cause a crash if used. That's why all other usages of panel() are guarded. - David Edmundson On Sept. 19, 2016, 9:28 p.m., Kamil Sołtysik wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/128950/ > ----------------------------------------------------------- > > (Updated Sept. 19, 2016, 9:28 p.m.) > > > Review request for Plasma. > > > Bugs: https://bugs.kde.org/show_bug.cgi?id=363592 > > https://bugs.kde.org/show_bug.cgi?id=https://bugs.kde.org/show_bug.cgi?id=363592 > > > Repository: plasma-workspace > > > Description > ------- > > As stated in doc here: > https://userbase.kde.org/KDE_System_Administration/PlasmaDesktopScripting#Panels > Panels should allow to be placed on selected screen by setting value of > Panel::screen property. While looking at code I found out that this property > is read-only in code. Patch changes that. > > > Diffs > ----- > > shell/scripting/panel.h 0d70784 > shell/scripting/panel.cpp b9383df > > Diff: https://git.reviewboard.kde.org/r/128950/diff/ > > > Testing > ------- > > Panel's position can now be changed by setting the property. Setting invalid > value causes no response. > > > Thanks, > > Kamil Sołtysik > >
