<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/119781/">https://git.reviewboard.kde.org/r/119781/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 17th, 2014, 1:38 p.m. UTC, <b>David Edmundson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/119781/diff/2/?file=305297#file305297line30" style="color: black; font-weight: bold; text-decoration: underline;">src/declarativeimports/plasmacomponents/qml/styles/TextAreaStyle.qml</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">30</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="nx">ScrollViewStyle</span> <span class="p">{</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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. </p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">What might work is:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">QtQuickControls.TextAreaStyle<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
{<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
ScrollViewStyle {<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
id: svs<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
}</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%">frame<span style="color: #666666">:</span> svs<span style="color: #666666">.</span><span style="color: #7D9029">frame</span>
scrollBarBackground<span style="color: #666666">:</span> svs<span style="color: #666666">.</span><span style="color: #7D9029">scrollBarBackground</span>
handle<span style="color: #666666">:</span> svs<span style="color: #666666">.</span><span style="color: #7D9029">handle</span>
<span style="color: #666666">(</span>and so on<span style="color: #666666">)</span>
textColor<span style="color: #666666">:</span> theme<span style="color: #666666">...</span>whatever
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">}<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Then we don't need to redeclare the properties and have the cursorHandle, selectionHandle etc.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p></pre>
</blockquote>
<p>On August 17th, 2014, 2:59 p.m. UTC, <b>Marco Martin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">hmm, not convinced.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
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.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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</p></pre>
</blockquote>
<p>On August 17th, 2014, 5:09 p.m. UTC, <b>David Edmundson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Having one near empty QObject vs potentially having a shell that doesn't load seems worth it.</p></pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">ok, convinced ;)</p></pre>
<br />
<p>- Marco</p>
<br />
<p>On August 14th, 2014, 11:10 a.m. UTC, Marco Martin wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDE Frameworks and Plasma.</div>
<div>By Marco Martin.</div>
<p style="color: grey;"><i>Updated Aug. 14, 2014, 11:10 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
plasma-framework
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This ports TextArea to Qtcontrols, all old properties/functions work (except for errorHighlight that was a stub already)<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
moves also the scrollview style to make everything in the same place. (needs to be more complete still before becoming a proper import)</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>src/declarativeimports/plasmacomponents/qml/TextArea.qml <span style="color: grey">(3f68934)</span></li>
<li>src/declarativeimports/plasmacomponents/qml/styles/ScrollViewStyle.qml <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/declarativeimports/plasmacomponents/qml/styles/TextAreaStyle.qml <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/declarativeimports/plasmaextracomponents/qml/ScrollArea.qml <span style="color: grey">(3818142)</span></li>
<li>src/declarativeimports/plasmaextracomponents/qml/styles/ScrollViewStyle.qml <span style="color: grey">(cb0b190)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/119781/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>