<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/112375/">http://git.reviewboard.kde.org/r/112375/</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 30th, 2013, 12:43 p.m. UTC, <b>Hugo Pereira Da Costa</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;">I respectfully disagree with the change.
If I understand the descripion right, the only thing that matters for QtQuick is to change the default returned value (if all else fail) from 1 to FRAME_FRAMEWIDTH. Correct ?
The other enumerations, (LineEdit_FrameWidth, ComboBox_FrameWidth, etc.) must stay: even if they have the same values at the moment, this might change in the future (in the near future in fact, since nuno and I want to revisit the metrics, make them more dpi independent, etc.).
That the cast wont work for QtQuick is not a good reason to remove it, with all (non quick) applications around, and for which such casts work.
Now as for the change needed for QtQuick, this will (must) break (that is: change) all applications that render a frame and don't fall in the cathegories above (think custom widgets). I too have no example of this (but I'm sure there are), and since these are "custom" things, it is normal that they don't show up in oxygen-demo.
Please update the change to the "minimal" (namely -> change the default value returned for PM_DefaultFrameWidth), and then, well, we'll need to test ...
</pre>
</blockquote>
<p>On August 30th, 2013, 1:21 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;">I expected this to be a review which started a discussion :)
It's one that sets precedent for a few other things.
The thing that matters is for QtQuick to get the same width as QLineEdits get. This happens to be FRAME_FRAMEWIDTH now.
The reason I removed the LineEdit_FrameWidth is that if someone later does decide to change the LineEdit_FrameWidth the QtQuickControls will become broken again not matching the desktop counterpart. I'm deliberately taking away that option.
The only way we can support QtQuickControls is to support the lowest common denominator of QStyle that they both follow even if it makes the desktop worse (or at least different).
This basically means supporting only values in QStyle, and not hacking in our own extra granularity determined by casting widgets.
I appreciate this is a very sucky situation to be in, but if we don't we will we see a very clear discrepancy in the UI depending on which technology happens to be used. I don't think we want that.
By only other options are:
--
I could try to add PM_LineEdit and PM_ComboBox into Qt5.2? It would solve this particular issue.
The base implementation of QStyle can then just call PM_DefaultFrameWidth for backwards compatibility. I'm not sure it would get in as it relies on all styles calling the base implementation for enum values they don't support.
--
We make a QML Oxygen theme from 'scratch' a bit like how we have an Oxygen GTK theme.. different tech that happens to look the same. </pre>
</blockquote>
<p>On August 30th, 2013, 1:23 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;">s/By/My</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Why do QtQuickControls need pixelMetric() for line edits? To find out, how much margin there is around a styled line edit, it should simply use sizeFromContents() and subElementRect().</pre>
<br />
<p>- Christoph</p>
<br />
<p>On August 30th, 2013, 11:11 a.m. UTC, David Edmundson wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Plasma and Hugo Pereira Da Costa.</div>
<div>By David Edmundson.</div>
<p style="color: grey;"><i>Updated Aug. 30, 2013, 11:11 a.m.</i></p>
<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;">Use a single frame width for all PM_DefaultFrameWidth
The current code sets a width of 3 for all line edits, combo boxes
and frames, otherwise it returns a width of 1.
The QtQuickControls engine cannot qobject_cast() the widget so always
return a frame width of 1 for qtquickcontrol line edits and combo boxes.
This simplifies the code and solves that issue.
I can think of no other way to resolve this without editing Qt, and even then it would be difficult to extend the PixelMetric enum without breaking compatibility.
Note this is potentially a visual change in oxygen, however I have yet to see anything actually different.</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>kstyles/oxygen/oxygenmetrics.h <span style="color: grey">(0643ae5b20d0c9efa328a87e08707cebaabf9f5e)</span></li>
<li>kstyles/oxygen/oxygenstyle.cpp <span style="color: grey">(86b5cdf3054f5d362d90f0f76c30bfb4f2646911)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/112375/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments </h1>
<ul>
<li><a href="http://git.reviewboard.kde.org/media/uploaded/files/2013/08/30/spell1.png">QML After</a></li>
<li><a href="http://git.reviewboard.kde.org/media/uploaded/files/2013/08/30/spell1_1.png">QML_Before</a></li>
<li><a href="http://git.reviewboard.kde.org/media/uploaded/files/2013/08/30/oxygen1.png">Normal oxygen demo</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>