<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/108265/">http://git.reviewboard.kde.org/r/108265/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On January 9th, 2013, 9:17 a.m., <b>Mark Gaiser</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 cannot imagine that code duplication is the way to go here. Surely there is a smarter way that doesn't involve duplicating code. There where other duplicated code paths before in KDE like the tooltip stuff and the rules to blur behind a window. The tooltip mess is still active, the blur behind is finally solved but that's mainly because nearly all apps added the needed lines to enable that feature.
If you duplicate this class then it's bound to be a problematic part in the future. I strongly recommend finding a different solution that does not involve duplicating this code.</pre>
</blockquote>
<p>On January 9th, 2013, 9:23 a.m., <b>Cedric Bellegarde</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;">In fact, tooltip.cpp in icontasks is already forked code from kdelibs...
But i think my one line patch from https://git.reviewboard.kde.org/r/108241/ should be ok, no?</pre>
</blockquote>
<p>On January 9th, 2013, 9:44 a.m., <b>Mark Gaiser</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'm not going to judge, but it "seems" better then duplicating a class.</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;">Cedric Bellegarde, no, since disable overriding the shadow will only give tooltip default oxygen shadow, instead of the correct plasma svg based one. Check other theme like Slim Glow, I don't think your patch will still work.
Mark Gaiser, for long term solution, every one should use Plasma::Dialog even for Tooltip, but Plasma::Dialog still missing one api to override the imagePath to use, so it's not a good idea to open this dup class up in kdelibs. And actually, this part of code is being copied to kde-workspace once. Instead of modifying the kdelibs API, this patch is the only way to go for 4.10.</pre>
<br />
<p>- Xuetian</p>
<br />
<p>On January 8th, 2013, 2:06 p.m., Xuetian Weng wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Plasma, Cedric Bellegarde and Aaron J. Seigo.</div>
<div>By Xuetian Weng.</div>
<p style="color: grey;"><i>Updated Jan. 8, 2013, 2:06 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;">copy dialogshadows to icontasks, and use DialogShadows to handle icon tasks shadow.
This is a different solution of https://git.reviewboard.kde.org/r/108241/ , I guess this is what it should be.
(remove halo part is just to keep consistency with default tooltip)</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </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;">tested, no problem.</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>applets/icontasks/CMakeLists.txt <span style="color: grey">(1ff6042)</span></li>
<li>applets/icontasks/tooltips/dialogshadows.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>applets/icontasks/tooltips/dialogshadows_p.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>applets/icontasks/tooltips/tooltip.cpp <span style="color: grey">(43f09ed)</span></li>
<li>applets/icontasks/tooltips/tooltipmanager.cpp <span style="color: grey">(dd36a9e)</span></li>
<li>applets/icontasks/tooltips/windowpreview.cpp <span style="color: grey">(94c9eb2)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/108265/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>