<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/108224/">http://git.reviewboard.kde.org/r/108224/</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 6th, 2013, 7:34 a.m., <b>Martin Gräßlin</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 don't like having a link dependency on plasmagenericshell. If that is supposed to be used, then it needs to go to the workspaces libs.
I also think that this approach just doesn't scale. We use Plasma styled dialogs for more things and I don't want to have to use this approach everywhere and not all of them are C++ based. Furthermore in case of KWin it's a little bit stupid to set an X11 property for the shadows on a KWin internal window.
I will look into the whole thing. I think especially the tabbox has some old code still around which might not be needed any more.</pre>
</blockquote>
<p>On January 6th, 2013, 8:02 a.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;">yep, I totally agree what you said. But as I exam the code I couldn't think of any easier solution that can avoid rewrite this part of code.
One of the problem is, the background is defined in qml, so set shadow here isn't correct if someone don't use a plasma background. But maybe the background should be move out, but in that case it would break existing tabbox qml (fortunately there aren't much on kde-look.org).
The right approach is to use Plasma::Dialog IMHO, but use it here is not very easy and it would still use same code (which is in kdelibs) to set X11 property for shadow.
So kwin will directly use Plasma::Svg for tabbox shadow?.. I can't see a clear way for doing this right so I just take a easiest way for this. (And to call attention for right people on this, on purpose :P)</pre>
</blockquote>
<p>On January 6th, 2013, 11:20 a.m., <b>Thomas Lübking</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;">KWin needs a (tmp proprietary) property to set the shadow from qml.
Item {
shadowLeft = <someimage>
}
Plasma (components) hopefully provides a qml way to access the pixmap(id) of a Plasma::Svg directly, otherwise one will have to create PlasmaCore.SvgItem (i hope they exist) and pass them over to the property (but i've atm. no idea how to make an image out of that)</pre>
</blockquote>
<p>On January 6th, 2013, 12:04 p.m., <b>Martin Gräßlin</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;">given that KWin includes one layout for Plasma Active which does not set a background at all and which should not get a shadow, anything which would enforce the usage of Shadows is wrong.
It needs to be done in a way as Thomas suggests. If that's not possible than we have to live with a regression.</pre>
</blockquote>
<p>On January 7th, 2013, 10:24 a.m., <b>Aaron J. Seigo</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 don't like having a link dependency on plasmagenericshell. If that is supposed to be used, then it needs to go to the workspaces libs"
libplasmagenerichsell is in kde-workspace/libs/plasmagenericshell. it obviously has no dependency on any plasma application or shell since it's in kde-workspace/libs. what more do you want?
"We use Plasma styled dialogs for more things and I don't want to have to use this approach everywhere and not all of them are C++ based."
if instead of using plasma style dialogs, the code used Plasma::Dialog then it wouldn't be an issue. if there is some reason Plasma::Dialog can not be used, we can perhaps look at why and see if that can be fixed.</pre>
</blockquote>
<p>On January 7th, 2013, 10:46 a.m., <b>Martin Gräßlin</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 don't like having a link dependency on plasmagenericshell. If that is supposed to be used, then it needs to go to the workspaces libs"
> libplasmagenerichsell is in kde-workspace/libs/plasmagenericshell. it obviously has no dependency on any plasma application or shell since it's in kde-workspace/libs. what more do you want?
When I wrote that I didn't know that it is in libs, I assumed it's not, because the include used "" instead of <> also the name implies that it is about a generic plasma desktop shell. My guessing skills are not that good ;-)
>> "We use Plasma styled dialogs for more things and I don't want to have to use this approach everywhere and not all of them are C++ based."
> if instead of using plasma style dialogs, the code used Plasma::Dialog then it wouldn't be an issue. if there is some reason Plasma::Dialog can not be used, we can perhaps look at why and see if that can be fixed.
KWin is a very special beast, it cannot manage it's own Clients. If it somehow gets a dependency to KWindowSystem it even starts to break as events gets eaten. I don't know if that has been the issue here, but it is quite likely. I remember that for the Qml based TabBox it just didn't work with using the appropriate Plasma Component. It's nice that you want to fix it, but I rather doubt that you want to include fixes for the special needs of a window manager :-)
But definetaly using Plasma::Dialog would mean that it has to be Plasma styled. That is already a showstopper for TabBox given that the window strip may not be Plasma styled. Also for Qml based scripts it would turn the you *can* use Plasma style into a you *have* to use Plasma style. I must say that I don't want to have to depend on Plasma styling as that would be considered as a showstopper for any approaches of using KWin in Razor-Qt (they are very interested) and Unity (I have hope since Compiz is dead and Sam no longer employed at Canonical).</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;">so if i am to understand this correctly .. kwin is not using a top level window for tabbox at all, and the issue for the tabbox and this change is that the shadows are no longer *part* of the background?
if that's so, then all that needs be done is to add the shadows by name in the rendering in the tabbox. shadow-top, etc. this can be done easily in the QML itself, and the problem would be solved.</pre>
<br />
<p>- Aaron J.</p>
<br />
<p>On January 6th, 2013, 6:55 a.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 kwin, Plasma, Aaron J. Seigo, and Martin Gräßlin.</div>
<div>By Xuetian Weng.</div>
<p style="color: grey;"><i>Updated Jan. 6, 2013, 6:55 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 the same technique for krunner and some plasma to brought back tabbox shadow.</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;">compiles, 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>kwin/CMakeLists.txt <span style="color: grey">(62e9964)</span></li>
<li>kwin/kcmkwin/kwintabbox/CMakeLists.txt <span style="color: grey">(72a6b72)</span></li>
<li>kwin/tabbox/declarative.h <span style="color: grey">(e36e67b)</span></li>
<li>kwin/tabbox/declarative.cpp <span style="color: grey">(3bdcfac)</span></li>
<li>powerdevil/daemon/brightnessosdwidget.h <span style="color: grey">(ef08903)</span></li>
<li>powerdevil/daemon/brightnessosdwidget.cpp <span style="color: grey">(e4cf80a)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/108224/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>