<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/126597/">https://git.reviewboard.kde.org/r/126597/</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 2nd, 2016, 9:27 a.m. UTC, <b>David Edmundson</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Snarky comments don't really help acheive anything.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">What version of Qt are you running. This was all added to work round a change in a late Qt XCB 5.5?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Also could you expand on the bugs you're seeing. You've linked to 355069 which is marked as fixed with no additional comments since the commit, which doesn't tell me much.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This looks pretty much like it's just a revert of 4a6f7db0018a2a7366ac7f2ad61fc33f31566c03
Plus a change from setX/setY to setPosition to skip the the "Behavior on y {" animation. Is that right?</p></pre>
</blockquote>
<p>On January 2nd, 2016, 11:24 a.m. UTC, <b>Anthony Fieroni</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Qt 5.5.1, libxcb 1.11.1 it's the lastest stable release on Arch. I'm not reverting the patch, the patch is pretty <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">ugly</em> and don't work at all. The bugs are many, flying on notifications on screen, configuration not work when "Use custom position for the notification popup" is unchecked. Just throw some notifications (4 to 5) by "kdialog --passivepopup test" and you will see position changing (bottom left instead of bottom right), flying top to bottom. Configuration is compleatly broken, just uncheck more than one "Use custom position for the notification popup" and you will see that signal configurationChanged is not triggered, add position changed (on monitor pic) makes things not working at all.
And yes, "Behavior on y {" animation is bypassed setPosition, not only that configNotifications.qml is changed to work correctly, to trigger signal every time <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">correctly</em>.</p></pre>
</blockquote>
<p>On January 2nd, 2016, 11:43 a.m. UTC, <b>Anthony Fieroni</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">setPosition(x,y) prevent from flyings, other thigs are to make configuration to work correctly. Try my patch, if has some thing that not work i want to know.</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">As far as we can see, we have 3 changes:</p>
<ul style="padding: 0;text-rendering: inherit;margin: 0 0 0 1em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">moving some stuff (back) from C++ to QML
>From what I can tell, this is exactly the same, and does nothing.</p>
</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">a config fix
+1 (except for the minor comment below)</p>
</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">removing the animation</p>
</li>
</ul>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It's not really a fix as much as just avoiding the problem. However, if it fixes things, I'm not against it.
I'll wait for Martin to comment on that. </p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If we do merge that, I want the Behavior on y code removed as it'd be just dead code.</p></pre>
<br />
<p>- David</p>
<br />
<p>On January 2nd, 2016, 6:55 a.m. UTC, Anthony Fieroni wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for Plasma, David Edmundson and Martin Klapetek.</div>
<div>By Anthony Fieroni.</div>
<p style="color: grey;"><i>Updated Jan. 2, 2016, 6:55 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://bugs.kde.org/show_bug.cgi?id=355069">355069</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
plasma-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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Complete not flying any more, please compete rewrite or do not touch this code any more !</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">First of all, setProperty animate position, but with this kid of logic, empty notification window is trying to be shown more than <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">4</em> times, cause to be drawn in differet positions (flying through screen). Now position is set at once <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">setPosition(x,y)</em> and looks and feel pretty good. Removed pretty <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">ugly</em> checks and unneeded functions.
Second of all, configuration was <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">totally</em> broken, not only flying windows, but in default configuration they (<em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">notifications</em>) apears in compleately wrong positions, mostly on bottom left mixed with bottom right.
Third of all, check on "Use custom position for the notification popup" then choose position , hit apply. Then uncheck it, the check it again, cause not trigger signal configuration changed.
At last, all test works as expected, no regressions even more, all on configurations <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">works</em> as expected no matter how many time chech for positions is checked/unchecked, every time configuration changed is trigger and position is applied. No wrong positions at all, no flying at all.</p></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/notifications/lib/notificationsapplet.h <span style="color: grey">(5b262f1)</span></li>
<li>applets/notifications/lib/notificationsapplet.cpp <span style="color: grey">(891cdb0)</span></li>
<li>applets/notifications/package/contents/ui/Notifications.qml <span style="color: grey">(f479a65)</span></li>
<li>applets/notifications/package/contents/ui/ScreenPositionSelector.qml <span style="color: grey">(efff648)</span></li>
<li>applets/notifications/package/contents/ui/configNotifications.qml <span style="color: grey">(95a8e59)</span></li>
<li>applets/notifications/plugin/notificationshelper.h <span style="color: grey">(860a2da)</span></li>
<li>applets/notifications/plugin/notificationshelper.cpp <span style="color: grey">(15b4479)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/126597/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>