Review Request 119781: Port TextArea to QtControls

[David Edmundson]

> On Aug. 17, 2014, 1:38 p.m., David Edmundson wrote:
> > src/declarativeimports/plasmacomponents/qml/styles/TextAreaStyle.qml, line 30
> > <https://git.reviewboard.kde.org/r/119781/diff/2/?file=305297#file305297line30>
> >
> >     I can see why you're doing this but I think this approach is a bit dangerous, it means we constantly have to match our propoerties with upstream's. 
> >     
> >     If 5.4 introduces a new property to TextAreaStyle, we won't use the default implementation but instead try to use properties that doesn't exist which could potentially explode.
> >     
> >     What might work is:
> >     
> >     QtQuickControls.TextAreaStyle
> >     {
> >         ScrollViewStyle {
> >            id: svs
> >         }
> >         
> >         frame: svs.frame
> >         scrollBarBackground: svs.scrollBarBackground
> >         handle: svs.handle
> >         (and so on)
> >         
> >         textColor: theme...whatever
> >     
> >     }
> >     Then we don't need to redeclare the properties and have the cursorHandle, selectionHandle etc.
> >     
> >     There's also an abandoned upstream review request to make Scrollbars a single component which will solve most the duplication. We could try and help get that finished.
> 
> Marco Martin wrote:
>     hmm, not convinced.
>     I see building a scrollviewstyle inside another scrollviewstyle even worse, it creates an extra instance for basically nothing just binding everything it implements. It's true that an addition in textareastyle  in base will give problems, but i'm not sure it's worth instancing a dead scrollviewstyle.
>     
>     TextAreaStyle is in itself an extension of the same style's ScrollViewStyle. base style, Desktop style and Android style all inherit from their own scrollviews

>It's true that an addition in textareastyle  in base will give problems, but i'm not sure it's worth instancing a dead scrollviewstyle.

Having one near empty QObject vs potentially having a shell that doesn't load seems worth it.


- David


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


On Aug. 14, 2014, 11:10 a.m., Marco Martin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/119781/
> -----------------------------------------------------------
> 
> (Updated Aug. 14, 2014, 11:10 a.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> -------
> 
> This ports TextArea to Qtcontrols, all old properties/functions work (except for errorHighlight that was a stub already)
> moves also the scrollview style to make everything in the same place. (needs to be more complete still before becoming a proper import)
> 
> 
> Diffs
> -----
> 
>   src/declarativeimports/plasmacomponents/qml/TextArea.qml 3f68934 
>   src/declarativeimports/plasmacomponents/qml/styles/ScrollViewStyle.qml PRE-CREATION 
>   src/declarativeimports/plasmacomponents/qml/styles/TextAreaStyle.qml PRE-CREATION 
>   src/declarativeimports/plasmaextracomponents/qml/ScrollArea.qml 3818142 
>   src/declarativeimports/plasmaextracomponents/qml/styles/ScrollViewStyle.qml cb0b190 
> 
> Diff: https://git.reviewboard.kde.org/r/119781/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20140817/34a3aae8/attachment-0001.html>