> 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 ;-)
>
> Martin Tobias Holmedahl Sandsmark wrote:
> 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?
>
> David Edmundson wrote:
> >It has the same race condition problem that you say this patch has, only
> worse.
>
> That's not the case.
>
> Currently with the race condition case the app will get a KSNI object.
> That will go into legacy mode, fail the env check and do nothing. Plasma will
> load, then it will call checkForRegisteredHosts, setLegacyMode(false) and
> create a 100% working SNI.
>
> With ths modification the user will end up with either:
> - a half-working icon via xembedsniproxy
> - a floating window with an icon in
> - absolutley nothing.
>
> From our position, that's obviously an unnaceptable step backwards.
>
> That assumes a working Plasma setup and for apps to correct have the env
> vars set by startkde.
> (obviously, that's not the case on *your* system - though I still don't
> know what, which is what we've tried asking multiple times.)
>
> > Or a proxy SNI "host" that allows applications to connect even if the
> full plasma session isn't up and running yet?
>
> This sort of exists, there is the SNI watcher which is a KDED module
> which is what clients register to.
> A host (Plasma) talks to the watcher, which tells it where clients are.
> From a pure SNI API point of view, clients can register to the watcher
> before a host exists.
>
> Or:
> We could just add a setAllowLegacyFallBack(bool) to kstatusnotifier item
> which our QPT can then use.
> Or even a qApp->setProperty("") in the QPT constructor and check in the
> KSNI code.
> Or we can copy/paste the DBus code from KSNI into here. It's not too much
> code.
>
> Martin Tobias Holmedahl Sandsmark wrote:
> > Currently with the race condition case the app will get a KSNI object.
> That will go into legacy mode, fail the env check and do nothing. Plasma will
> load, then it will call checkForRegisteredHosts, setLegacyMode(false) and
> create a 100% working SNI.
>
> My bad, that's actually a good solution.
>
> > With ths modification the user will end up with either: [...] From our
> position, that's obviously an unnaceptable step backwards.
>
> I agree.
>
>
> > That assumes a working Plasma setup and for apps to correct have the
> env vars set by startkde. (obviously, that's not the case on your system -
> though I still don't know what, which is what we've tried asking multiple
> times.)
>
> Several applications and/or libraries look for the same variables to
> enable certain functionality. E. g. the owncloud client via qt-keychain use
> them to detect whether it should use kwallet5:
> https://github.com/frankosterfeld/qtkeychain/blob/master/keychain_unix.cpp#L33-L73.
> I added a fix/workaround it with this patch, but it's not merged yet:
> https://github.com/frankosterfeld/qtkeychain/pull/75.
>
> I know plasma-integration is not supported outside a full Plasma session,
> but I thought I'd at at least give it a shot to fix it running without an SNI
> host available.
>
>
> > Or we can copy/paste the DBus code from KSNI into here. It's not too
> much code.
>
> You mean detecting when the SNI host shows up and then creating a KSNI? I
> guess that could work.
FWIW: I have a proposal that gets rid of our QPT creating an KSNI utilising
Qt's DBus system tray as it has significantly better menu handling.
https://codereview.qt-project.org/#/c/168458/
The race condition handling as above would have to be done inside
QGenericUnixTheme, but that's then do-able.
Assuming it goes in it would be Plasma 5.11 material.
- David
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128473/#review97506
-----------------------------------------------------------
On Aug. 13, 2016, 7:31 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 Aug. 13, 2016, 7:31 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
>
>
