> On July 19, 2016, 4:02 a.m., David Edmundson wrote:
> > 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)
> >
> > Not saying yours is "worse" but we don't want two fixes in two places.
> >
> > Could you check you have that patch? and why it doesn't work?
>
> Martin Tobias Holmedahl Sandsmark wrote:
> 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.
>
> (fwiw, qApp->platformName() is not correct either, that's what I thought
> was the "proper" way to do it)
>
> Martin Tobias Holmedahl Sandsmark wrote:
> patching around that also leads to no legacy tray icon being created at
> all, which is obviously wrong.
>
> David Edmundson wrote:
> Is it wrong?
>
> If you're logged in to a plasma session with the plasma integration
> running it's a valid assumption that people will run Plasmashell.
>
> Without plasmashell you won't get the legacy tray icons appearing either.
>
> And if you're running a different shell..you shouldn't be using the
> plasma integration in the first place.
>
> Martin Tobias Holmedahl Sandsmark wrote:
> 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.
>
> 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.
>
> 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.
>
> David Edmundson wrote:
> >The legacy tray icons will work in cases where SNI won't
>
> How? If Plasmashell is running you get the SNIs. If plasmashell is not
> running you don't get the legacy icons anyway.
>
> It's not going in until you can explain:
> 1) why this is needed
> 2) why the other patch doesn't work.
>
> Martin Tobias Holmedahl Sandsmark wrote:
> > How? If Plasmashell is running you get the SNIs. If plasmashell is not
> running you don't get the legacy icons anyway.
>
> Because they're very different technologies with very different kinds of
> bugs? There's a reason the fallback is there already.
>
> 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.
>
>
> > 2) why the other patch doesn't work.
>
> 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.
>
> and as already demonstrated, it doesn't work well in practice.
>
>
> > 1) why this is needed
>
> to avoid applications hanging/crashing, approach the zen of failing
> gracefully, and in general make this a bit more robust.
>
> 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.
>
>
> but this discussion was absurd several posts ago, and it's not getting
> better...
>
> Martin Gräßlin wrote:
> 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.
>
> 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 ;-)
Sorry for the late reply, dayjob started up again. :-)
> So could you please explain why the existing solution doesn't work [...]
I'll try to summarize:
* 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.
* 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.
* An "ideological" reason is that putting hacks in a framework like
KNotifications for plasma-integration is wrong.
* 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.
> [...] and why this is needed in addition?
The hack in KNotifications should be removed eventually, IMHO, this is a
replacement not an addition to the existing fix/solution.
> We need to start thinking about the "can break in Wayland" cases more, after
> all there are people using it productivly ;-)
I agree, I'd like to move to wayland soon myself. :-)
But this patch won't break anything on Wayland that isn't broken already.
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?
- Martin Tobias Holmedahl
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128473/#review97506
-----------------------------------------------------------
On July 17, 2016, 8:14 p.m., Martin Tobias Holmedahl Sandsmark wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128473/
> -----------------------------------------------------------
>
> (Updated July 17, 2016, 8:14 p.m.)
>
>
> Review request for Plasma.
>
>
> Repository: plasma-integration
>
>
> Description
> -------
>
> If the status notifier item host is not available, KSNI tries to create a
> normal QSystemTrayIcon.
>
> The plasma platform plugin uses KSNI when it is called to create a
> QPlatformSystemTrayIcon.
>
> So if the status notifier item host for any reason was unavailable, this
> would recursively run forever (assuming a turing machine with infinite
> memory).
>
>
> Diffs
> -----
>
> src/platformtheme/kdeplatformsystemtrayicon.h 6825b4d
> src/platformtheme/kdeplatformsystemtrayicon.cpp 0e82385
> src/platformtheme/kdeplatformtheme.cpp 5f0407c
>
> Diff: https://git.reviewboard.kde.org/r/128473/diff/
>
>
> Testing
> -------
>
> 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.
>
>
> Thanks,
>
> Martin Tobias Holmedahl Sandsmark
>
>
