<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>
<p>On January 9th, 2013, 12:36 p.m., <b>Xuetian Weng</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;">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>
</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;">agreed; short term solution for 4.10 is to duplicate this one class. this is inevitable since icontasks already copied (forked, really) some code from libplasma. (and this is a good reason why not to do that :)
long term, icontasks needs to use either Plasma::ToolTip or Plasma::Dialog. preferably ToolTip. in both cases, it will probably mean some modifications to ToolTip or Dialog which should be done in libplasma2 (or, well, in the QML components that replace those bits)</pre>
<br />
<p>- Aaron J.</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>