<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="https://git.reviewboard.kde.org/r/115859/">https://git.reviewboard.kde.org/r/115859/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On February 18th, 2014, 2:15 p.m. UTC, <b>Thomas Lübking</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<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="https://git.reviewboard.kde.org/r/115859/diff/3/?file=244774#file244774line72" style="color: black; font-weight: bold; text-decoration: underline;">kwin/effects/slide/slide.cpp</a>
<span style="font-weight: normal;">
(Diff revision 3)
</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; ">void SlideEffect::prePaintWindow(EffectWindow* w, WindowPrePaintData& data, int time)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">72</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">w</span><span class="o">-></span><span class="n">setData</span><span class="p">(</span><span class="n">WindowForceBackgroundContrastRole</span><span class="p">,</span> <span class="n">QVariant</span><span class="p">(</span><span class="nb">true</span><span class="p">));</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">what about setting and removing this with setting the "slide" flag and catch windowAdded() while slide is true for that matter?</pre>
</blockquote>
<p>On February 18th, 2014, 3:44 p.m. UTC, <b>Sebastian Kügler</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;">This means duplicating the logic which window to transform from ::prePaintWindow. We can do that, but it doesn't make the code less complex and more prone to errors in the future. I suppose setting a flag isn't that expensive?
(I can do it, but wanted to make sure it's worth it, first.)</pre>
</blockquote>
<p>On February 18th, 2014, 4:02 p.m. UTC, <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;">the logics could be moved into "shouldSlide(EffectWindow*)"
it's not a flag - setData() operates on a QHash where non-trivial writing has some overhead (tree balancing)
also (and at least) ::postPaintWindow should shortcut on "!slide"</pre>
</blockquote>
<p>On February 18th, 2014, 4:07 p.m. UTC, <b>Sebastian Kügler</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;">Okay. :)
Changing the patch to do that and your other comments. (Thanks for reviewing.)</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Isn't this more like https://git.reviewboard.kde.org/r/115857/ ?
As mentioned there, i do not believe that aligning effect runtimes is a good way to prevent artifacts/flicker etc. - esp. not by a "magic" value.
Also i'm not sure why alignment would be required here.
The only clash would be if a popup hides while the desktop is switched.
Ie. the slide effect sets the WindowForceBackgroundContrastRole and withdraws it, while the slidingpopup effect would prefer to keep it a little longer, right?
In that case the role could be interpreted as integer, rather than bool ("still required? still? still? no more? 'key.") ie. keep bg contrast processing as long as the data().toInt() > 0</pre>
<br />
<p>- Thomas</p>
<br />
<p>On February 18th, 2014, 3:58 p.m. UTC, Sebastian Kügler wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://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 kwin and Plasma.</div>
<div>By Sebastian Kügler.</div>
<p style="color: grey;"><i>Updated Feb. 18, 2014, 3:58 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kde-workspace
</div>
<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;">Force-allow background contrast while sliding
When the sliding effect is active, the background blur of the panel
would be disabled unless forced with WindowForceBackgroundContrastRole,
which is actually what we want: during sliding, the backgroundcontrast
effect would otherwise be disabled, leading to the panel and popups
flickering between contrast enabled and disabled.
With this patch, the panel keeps its coloring during desktop changes.</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;">Switched desktops, panel keeps color, fullscreen video player doesn't seem to be affected negatively, everything still works as expected, except that the flickering in Plasma Dialogs is gone.</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/effects/slide/slide.cpp <span style="color: grey">(8ecb2a6)</span></li>
<li>kwin/effects/slide/slide.h <span style="color: grey">(c8e0a84)</span></li>
<li>kwin/effects/slidingpopups/slidingpopups.cpp <span style="color: grey">(fd697f0)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/115859/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>