Review Request 124785: [Panelconfig] Reset the panel offset on alignment change

[David Kahles]

> On Aug. 22, 2015, 11:42 p.m., David Edmundson wrote:
> > desktoppackage/contents/configuration/panelconfiguration/Ruler.qml, line 48
> > <https://git.reviewboard.kde.org/r/124785/diff/1/?file=395466#file395466line48>
> >
> >     PanelConfiguration.qml has this
> >     
> >     Connections {
> >             target: panel
> >             onOffsetChanged: ruler.offset = panel.offset
> >             onMinimumLengthChanged: ruler.minimumLength = panel.minimumLength
> >             onMaximumLengthChanged: ruler.maximumLength = panel.maximumLength
> >         }
> >     
> >     
> >     my thoughts are:
> >     
> >     1) with your change we're changing the ruler offset but not the panel offset and they'll get out of sync. Are you sure it's the right offset to change?
> >     
> >     2) All of that original code is really weird and should be done as bindings...then we can kill the Component.onCompleted in this class.
> >     (and yeah, I know it's not yours)

1) Yes I am.
Ruler.qml, line 30 has this:
    property alias offset: offsetHandle.value

and line 75 (in the new Ruler.qml) has this:
    SliderHandle {
        id: offsetHandle
        graphicElementName: "offsetslider"
        onValueChanged: panel.offset = value
        property int position: (dialogRoot.vertical) ? y : x
    }
    
So when I change `offset`, this means that panel.offset is changed too.

2) Yes thats right, but I think we should merge this now and do the refactoring later, because it works for now. Or do you consider this as a merge blocker?


- David


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


On Aug. 17, 2015, 11:31 a.m., David Kahles wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124785/
> -----------------------------------------------------------
> 
> (Updated Aug. 17, 2015, 11:31 a.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-desktop
> 
> 
> Description
> -------
> 
> As offset and length have a different meaning in all alignments, the
> panel shifts on alignment change. This could result in wrong panel
> positions (e.g. panel shifted over a monitor border). The better approach
> would be the recalculation of all values, so that the panel stays at it's
> current position, but this would be error prone and complicated. As the
> panel alignment is rarely changed, it's not worth it. The more easy
> approach is just setting the panel offset to zero. This makes sure the
> panel has a valid position and size.
> 
> 
> Diffs
> -----
> 
>   desktoppackage/contents/configuration/panelconfiguration/Ruler.qml a31feb40598ba24a107f41ff3b3f823afaa89da6 
> 
> Diff: https://git.reviewboard.kde.org/r/124785/diff/
> 
> 
> Testing
> -------
> 
> When changing the alignment, the offset gets 0, so there are no invalid panel positions on alignment change
> 
> 
> Thanks,
> 
> David Kahles
> 
>

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