<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 Януари 4th, 2016, 6:17 преди обяд EET, <b>Martin Klapetek</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;">So I tested your patch and I will not accept removing the
animation altogether. If you have 2 or more notifications
at once, they just unexpectedly "jump", which causes quite
a trouble for visual tracking. Besides, all other popups
are animated in Plasma.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'm happy for the initial animation to go away tho and only
animate when the popup is already on the screen and it is
changing its position (replacing other popup).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The stuff you've moved back to qml is already addressed in
https://git.reviewboard.kde.org/r/126408/ and this will take
precedence; I don't want to move that code back to qml.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The fix for the config problem with the checkbox is good.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Finally, your patch actually puts the popup over my panel.</p>
<hr style="text-rendering: inherit;margin: 0;padding: 0;white-space: normal;border: 1px solid #ddd;line-height: inherit;" />
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">tl;dr - r126408 needs to go in, then we can break your patch
down to smaller patches and apply what is still relevant.</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;">Ok, i can make it to animate only when visibility is changed, but it will can cause flying, i'm not pretty sure, it must test. If you want can modify my code, i'm busy now, like this.
in .h -> repositionPopups(bool animate)
in .cpp -> NotificationsHelper::setPopupLocation -> repositionPopups(true)
NotificationsHelper::addNotificationPopup(QObject *win) -> connect(popup, &QWindow::heightChanged, [this] { repositionPopups(false); },Qt::UniqueConnection); (true only on Window::visibleChanged)
in repositionPopups if(animate && visible) setProperty("y"... blah-blah (I think only in this way will not see flyings and i'm not sure :)</p></pre>
<br />
<p>- Anthony</p>
<br />
<p>On Януари 2nd, 2016, 3:26 след обяд EET, 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 Ян. 2, 2016, 3:26 след обяд</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>