<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/128473/">https://git.reviewboard.kde.org/r/128473/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 19th, 2016, 4:02 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;">KStatusNotifierItemPrivate::setLegacySystemTrayEnabled(bool enabled)
already has a recursion check added in b45544f3d4dd9cb1873b92a609f45a68f5f6e471 (in knotifications) - which basically checks if we're using the KDE platform theme (albeit in a slightly weird way)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Not saying yours is "worse" but we don't want two fixes in two places.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Could you check you have that patch? and why it doesn't work?</p></pre>
</blockquote>
<p>On July 19th, 2016, 1:43 p.m. UTC, <b>Martin Tobias Holmedahl Sandsmark</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;">firstly, as you said, it checks in a weird way, which doesn't work, that's why I thought it made more sense to fix it in the platform theme itself which already knows that it is loaded and whether an SNI host is available.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">(fwiw, qApp->platformName() is not correct either, that's what I thought was the "proper" way to do it)</p></pre>
</blockquote>
<p>On July 19th, 2016, 1:49 p.m. UTC, <b>Martin Tobias Holmedahl Sandsmark</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;">patching around that also leads to no legacy tray icon being created at all, which is obviously wrong.</p></pre>
</blockquote>
<p>On July 19th, 2016, 3:01 p.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;">Is it wrong?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If you're logged in to a plasma session with the plasma integration running it's a valid assumption that people will run Plasmashell. </p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Without plasmashell you won't get the legacy tray icons appearing either. </p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">And if you're running a different shell..you shouldn't be using the plasma integration in the first place.</p></pre>
</blockquote>
<p>On July 21st, 2016, 2:58 p.m. UTC, <b>Martin Tobias Holmedahl Sandsmark</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;">The legacy tray icons will work in cases where SNI won't, so breaking that fallback is wrong in my opinion, but I agree it is better than the alternative.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Anyhow, this is a mess and the interdependencies on behaviour is bad. IMHO it should be "fixed" in both places, the plasma-integration plugin shouldn't rely on some shaky string magic logic in KSNI not to hang applications.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So if the only objection to this patch is that some applications a) under X11 get a legacy tray icon instead of a SNI one if started too early instead of hanging, b) under Wayland might not get a tray icon at all instead of just hanging if started too early, I think this should go in.</p></pre>
</blockquote>
<p>On July 21st, 2016, 3:31 p.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;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The legacy tray icons will work in cases where SNI won't</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">How? If Plasmashell is running you get the SNIs. If plasmashell is not running you don't get the legacy icons anyway.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It's not going in until you can explain:
1) why this is needed
2) why the other patch doesn't work.</p></pre>
</blockquote>
<p>On July 21st, 2016, 4:12 p.m. UTC, <b>Martin Tobias Holmedahl Sandsmark</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;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">How? If Plasmashell is running you get the SNIs. If plasmashell is not running you don't get the legacy icons anyway.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Because they're very different technologies with very different kinds of bugs? There's a reason the fallback is there already.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">and that's just the unintended ways it can fail, then you have users that for some reasons intentionally don't run plasma-shell with the default settings, use another tray solution, etc.</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">2) why the other patch doesn't work.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The other patch won't work because it doesn't know if the plasma integration plugin is loaded. it's also the wrong place to put those kinds of checks, even if you would find a 100% bulletproof bug free way to detect that. hardcoding in fixes for bugs in platform plugins (e. g. if another platform plugin decides to do the same) is wrong.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">and as already demonstrated, it doesn't work well in practice.</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">1) why this is needed</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">to avoid applications hanging/crashing, approach the zen of failing gracefully, and in general make this a bit more robust.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">imho, this shouldn't even be using the same KSNI classes that applications are supposed to use, it's a design error that leads to these kinds of problems, but this makes it a bit more sane.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">but this discussion was absurd several posts ago, and it's not getting better...</p></pre>
</blockquote>
<p>On July 22nd, 2016, 5:45 a.m. UTC, <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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So could you please explain why the existing solution doesn't work and why this is needed in addition? We just try to understand and whether there are things we need to change in KNotifications.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Btw. the fact that this will break SNIs during startup in a racy way on Wayland is a reason for me to not make it go in. We need to start thinking about the "can break in Wayland" cases more, after all there are people using it productivly ;-)</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;">Sorry for the late reply, dayjob started up again. :-)</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So could you please explain why the existing solution doesn't work [...]</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'll try to summarize:</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;">It has the same race condition problem that you say this patch has, only worse, as far as I can tell. If a SNI is unavailable on startup of the application, the application won't even get a legacy tray icon on X11, and nothing on Wayland.</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">A fundemental problem is that KNotifications does not have a (robust) way of knowing whether plasma-integration is used. The current hack to try to do this seems to break for various users. I tried with a patch to use «qgetenv("QT_QPA_PLATFORMTHEME") == "kde"» in addition to the existing checks, but it didn't seem to be enough.</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">An "ideological" reason is that putting hacks in a framework like KNotifications for plasma-integration is wrong.</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">People might run a plasma session without SNI. I'm not sure how realistic this is, though, but I don't think applications should crash and burn even in this case.</li>
</ul>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">[...] and why this is needed in addition?</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The hack in KNotifications should be removed eventually, IMHO, this is a replacement not an addition to the existing fix/solution.</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">We need to start thinking about the "can break in Wayland" cases more, after all there are people using it productivly ;-)</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I agree, I'd like to move to wayland soon myself. :-)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">But this patch won't break anything on Wayland that isn't broken already.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I don't really have any good ideas for how to fix it properly. Maybe using dbus activation somehow? Or a proxy SNI "host" that allows applications to connect even if the full plasma session isn't up and running yet?</p></pre>
<br />
<p>- Martin Tobias Holmedahl</p>
<br />
<p>On July 17th, 2016, 8:14 p.m. UTC, Martin Tobias Holmedahl Sandsmark 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.</div>
<div>By Martin Tobias Holmedahl Sandsmark.</div>
<p style="color: grey;"><i>Updated July 17, 2016, 8:14 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
plasma-integration
</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;">If the status notifier item host is not available, KSNI tries to create a normal QSystemTrayIcon.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The plasma platform plugin uses KSNI when it is called to create a QPlatformSystemTrayIcon.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So if the status notifier item host for any reason was unavailable, this would recursively run forever (assuming a turing machine with infinite memory).</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;">Now it is possible to run applications that have tray icons with the plasma platform plugin even when the status notifier item host is down or unavailable.</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>src/platformtheme/kdeplatformsystemtrayicon.h <span style="color: grey">(6825b4d)</span></li>
<li>src/platformtheme/kdeplatformsystemtrayicon.cpp <span style="color: grey">(0e82385)</span></li>
<li>src/platformtheme/kdeplatformtheme.cpp <span style="color: grey">(5f0407c)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/128473/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>