[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-panel-devel
Subject: Re: Review Request 112375: Correctly determine frame width when called from QtQuickControls
From: "Hugo Pereira Da Costa" <hugo () oxygen-icons ! org>
Date: 2013-08-31 12:14:38
Message-ID: 20130831121438.20576.29949 () vidsolbach ! de
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112375/#review39005
-----------------------------------------------------------
kstyles/oxygen/oxygenstyle.cpp
<http://git.reviewboard.kde.org/r/112375/#comment28774>
Rather than looking up the property multiple times, alongside the q_object_cast, \
I would rather add the test just before the last 'if' (when everything else has \
failed), and possibly caching the property in a QString.
Something like
else if( !widget && option && option->styleObject ) {
const QString elementType = option->styleObject->property( elementType );
... your code (all ifs, duplicated from the widget case)
} else {
return 1;
}
This way, you don't lookup the property if the widget is defined,
and You don't lookup the property multiple times.
This comes at the price of some code duplication, but it is a small block of \
code, so I'd deem that acceptable.
Oppinion ?
- Hugo Pereira Da Costa
On Aug. 30, 2013, 3:56 p.m., David Edmundson wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112375/
> -----------------------------------------------------------
>
> (Updated Aug. 30, 2013, 3:56 p.m.)
>
>
> Review request for Plasma and Hugo Pereira Da Costa.
>
>
> Description
> -------
>
> Correctly determine frame width when called from QtQuickControls
>
> When oxygen is used in QtQuickControls the widget parameter is empty
> as such we need to determine the widget type (lineedit etc. ) in a
> different way
>
>
> Diffs
> -----
>
> kstyles/oxygen/oxygenstyle.cpp 86b5cdf3054f5d362d90f0f76c30bfb4f2646911
>
> Diff: http://git.reviewboard.kde.org/r/112375/diff/
>
>
> Testing
> -------
>
>
> File Attachments
> ----------------
>
> QML After
> http://git.reviewboard.kde.org/media/uploaded/files/2013/08/30/spell1.png
> QML_Before
> http://git.reviewboard.kde.org/media/uploaded/files/2013/08/30/spell1_1.png
> Normal oxygen demo
> http://git.reviewboard.kde.org/media/uploaded/files/2013/08/30/oxygen1.png
>
>
> Thanks,
>
> David Edmundson
>
>
[Attachment #5 (text/html)]
<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 />
<div>
<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="http://git.reviewboard.kde.org/r/112375/diff/4/?file=185744#file185744line612" \
style="color: black; font-weight: bold; text-decoration: \
underline;">kstyles/oxygen/oxygenstyle.cpp</a> <span style="font-weight: normal;">
(Diff revision 4)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; \
">namespace Oxygen</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2">612</font></th> <td bgcolor="#fdfebc" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td> <th \
bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">612</font></th> <td bgcolor="#fdfebc" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> \
if( qobject_cast<const QLineEdit*>( widget ) ||</pre></td> </tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Rather than \
looking up the property multiple times, alongside the q_object_cast, I would rather \
add the test just before the last 'if' (when everything else has failed), and \
possibly caching the property in a QString.
Something like
else if( !widget && option && option->styleObject ) {
const QString elementType = option->styleObject->property( elementType );
... your code (all ifs, duplicated from the widget case)
} else {
return 1;
}
This way, you don't lookup the property if the widget is defined,
and You don't lookup the property multiple times.
This comes at the price of some code duplication, but it is a small block of code, so \
I'd deem that acceptable.
Oppinion ?</pre>
</div>
<br />
<p>- Hugo</p>
<br />
<p>On August 30th, 2013, 3:56 p.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, 3:56 p.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;">Correctly determine frame width when called from QtQuickControls
When oxygen is used in QtQuickControls the widget parameter is empty
as such we need to determine the widget type (lineedit etc. ) in a
different way</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/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>
_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic