Re: Review Request 128473: Avoid recursive calls to QPlatformTheme::createPlatformSystemTrayIcon()
> 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
Re: Review Request 128473: Avoid recursive calls to QPlatformTheme::createPlatformSystemTrayIcon()
--- 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.) Status -- This change has been discarded. 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
Re: Review Request 128473: Avoid recursive calls to QPlatformTheme::createPlatformSystemTrayIcon()
> 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
Re: Review Request 128473: Avoid recursive calls to QPlatformTheme::createPlatformSystemTrayIcon()
> 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
Re: Review Request 128473: Avoid recursive calls to QPlatformTheme::createPlatformSystemTrayIcon()
> 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
Re: Review Request 128473: Avoid recursive calls to QPlatformTheme::createPlatformSystemTrayIcon()
> On July 19, 2016, 6: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... 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 --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128473/#review97506 --- On July 17, 2016, 10:14 p.m., Martin Tobias Holmedahl Sandsmark wrote: > > --- > This is an automatically generated e-mail. To reply, visit:
Re: Review Request 128473: Avoid recursive calls to QPlatformTheme::createPlatformSystemTrayIcon()
> 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. > 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 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). > > >
Re: Review Request 128473: Avoid recursive calls to QPlatformTheme::createPlatformSystemTrayIcon()
> 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. >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. - David --- 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 > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 128473: Avoid recursive calls to QPlatformTheme::createPlatformSystemTrayIcon()
> 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. 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. - 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 > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 128473: Avoid recursive calls to QPlatformTheme::createPlatformSystemTrayIcon()
> On July 18, 2016, 5:49 a.m., Martin Gräßlin wrote: > > I acknowledge the problem in general, but I think the solution is wrong as > > this creates now a race condition on startup where apps don't show up in > > the systray at all. That is if an application tries to create a systray > > icon before Plasma is started. > > Martin Tobias Holmedahl Sandsmark wrote: > it still creates a systray icon, it just creates an "old style" tray icon. > > Martin Gräßlin wrote: > > it still creates a systray icon, it just creates an "old style" tray > icon. > > Which won't work on Wayland. And yes that's a valid point as we need to > think further than 3 months ;-) > > Martin Gräßlin wrote: > actually it won't even work on X11 as if Plasma is not up yet, neither > will be the xembed proxy. Which means no systray and Qt won't create it. > > Martin Tobias Holmedahl Sandsmark wrote: > won't it jump into the xembed proxy when it appears? I seem to recall > that happening, but I might be wrong. > > Martin Tobias Holmedahl Sandsmark wrote: > and regarding wayland, what does the "normal" platform plugin for wayland > do with QPlatformSystemTrayIcon? > > Martin Tobias Holmedahl Sandsmark wrote: > well, I looked in qtbase, and apparently Qt has a SNI implementation, so > the fallback here should work under wayland? > > David Edmundson wrote: > The Qt SNI won't work. > > That's in QGenericUnixTheme - however because we load our platform theme > (which subclasses QPlatformTheme) we don't load it - and it's private API so > can't (without changing Qt) > > Martin Tobias Holmedahl Sandsmark wrote: > Anyhow, you don't agree that it is better for the application to not get > a tray icon than recursing like this and not showing up at all and spinning > like crazy? I do agree. But it's also important to base patches based on the right assumptions. Which is why in the other comment thread I simply want to understand why the current solution doesn't work and how you end up in this situation where this is a problem before merging. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128473/#review97514 --- 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 > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 128473: Avoid recursive calls to QPlatformTheme::createPlatformSystemTrayIcon()
> 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. 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. - David --- 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 > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 128473: Avoid recursive calls to QPlatformTheme::createPlatformSystemTrayIcon()
> On July 18, 2016, 5:49 a.m., Martin Gräßlin wrote: > > I acknowledge the problem in general, but I think the solution is wrong as > > this creates now a race condition on startup where apps don't show up in > > the systray at all. That is if an application tries to create a systray > > icon before Plasma is started. > > Martin Tobias Holmedahl Sandsmark wrote: > it still creates a systray icon, it just creates an "old style" tray icon. > > Martin Gräßlin wrote: > > it still creates a systray icon, it just creates an "old style" tray > icon. > > Which won't work on Wayland. And yes that's a valid point as we need to > think further than 3 months ;-) > > Martin Gräßlin wrote: > actually it won't even work on X11 as if Plasma is not up yet, neither > will be the xembed proxy. Which means no systray and Qt won't create it. > > Martin Tobias Holmedahl Sandsmark wrote: > won't it jump into the xembed proxy when it appears? I seem to recall > that happening, but I might be wrong. > > Martin Tobias Holmedahl Sandsmark wrote: > and regarding wayland, what does the "normal" platform plugin for wayland > do with QPlatformSystemTrayIcon? > > Martin Tobias Holmedahl Sandsmark wrote: > well, I looked in qtbase, and apparently Qt has a SNI implementation, so > the fallback here should work under wayland? > > David Edmundson wrote: > The Qt SNI won't work. > > That's in QGenericUnixTheme - however because we load our platform theme > (which subclasses QPlatformTheme) we don't load it - and it's private API so > can't (without changing Qt) Anyhow, you don't agree that it is better for the application to not get a tray icon than recursing like this and not showing up at all and spinning like crazy? - Martin Tobias Holmedahl --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128473/#review97514 --- 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 > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 128473: Avoid recursive calls to QPlatformTheme::createPlatformSystemTrayIcon()
> 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) patching around that also leads to no legacy tray icon being created at all, which is obviously wrong. - 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 > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 128473: Avoid recursive calls to QPlatformTheme::createPlatformSystemTrayIcon()
> 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? 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 --- 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 > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 128473: Avoid recursive calls to QPlatformTheme::createPlatformSystemTrayIcon()
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128473/#review97506 --- 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? - David Edmundson 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 > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 128473: Avoid recursive calls to QPlatformTheme::createPlatformSystemTrayIcon()
> On July 18, 2016, 5:49 a.m., Martin Gräßlin wrote: > > I acknowledge the problem in general, but I think the solution is wrong as > > this creates now a race condition on startup where apps don't show up in > > the systray at all. That is if an application tries to create a systray > > icon before Plasma is started. > > Martin Tobias Holmedahl Sandsmark wrote: > it still creates a systray icon, it just creates an "old style" tray icon. > > Martin Gräßlin wrote: > > it still creates a systray icon, it just creates an "old style" tray > icon. > > Which won't work on Wayland. And yes that's a valid point as we need to > think further than 3 months ;-) > > Martin Gräßlin wrote: > actually it won't even work on X11 as if Plasma is not up yet, neither > will be the xembed proxy. Which means no systray and Qt won't create it. > > Martin Tobias Holmedahl Sandsmark wrote: > won't it jump into the xembed proxy when it appears? I seem to recall > that happening, but I might be wrong. > > Martin Tobias Holmedahl Sandsmark wrote: > and regarding wayland, what does the "normal" platform plugin for wayland > do with QPlatformSystemTrayIcon? > > Martin Tobias Holmedahl Sandsmark wrote: > well, I looked in qtbase, and apparently Qt has a SNI implementation, so > the fallback here should work under wayland? The Qt SNI won't work. That's in QGenericUnixTheme - however because we load our platform theme (which subclasses QPlatformTheme) we don't load it - and it's private API so can't (without changing Qt) - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128473/#review97514 --- 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 > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 128473: Avoid recursive calls to QPlatformTheme::createPlatformSystemTrayIcon()
> On July 18, 2016, 5:49 a.m., Martin Gräßlin wrote: > > I acknowledge the problem in general, but I think the solution is wrong as > > this creates now a race condition on startup where apps don't show up in > > the systray at all. That is if an application tries to create a systray > > icon before Plasma is started. > > Martin Tobias Holmedahl Sandsmark wrote: > it still creates a systray icon, it just creates an "old style" tray icon. > > Martin Gräßlin wrote: > > it still creates a systray icon, it just creates an "old style" tray > icon. > > Which won't work on Wayland. And yes that's a valid point as we need to > think further than 3 months ;-) > > Martin Gräßlin wrote: > actually it won't even work on X11 as if Plasma is not up yet, neither > will be the xembed proxy. Which means no systray and Qt won't create it. > > Martin Tobias Holmedahl Sandsmark wrote: > won't it jump into the xembed proxy when it appears? I seem to recall > that happening, but I might be wrong. > > Martin Tobias Holmedahl Sandsmark wrote: > and regarding wayland, what does the "normal" platform plugin for wayland > do with QPlatformSystemTrayIcon? well, I looked in qtbase, and apparently Qt has a SNI implementation, so the fallback here should work under wayland? - Martin Tobias Holmedahl --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128473/#review97514 --- 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 > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 128473: Avoid recursive calls to QPlatformTheme::createPlatformSystemTrayIcon()
> On July 18, 2016, 5:49 a.m., Martin Gräßlin wrote: > > I acknowledge the problem in general, but I think the solution is wrong as > > this creates now a race condition on startup where apps don't show up in > > the systray at all. That is if an application tries to create a systray > > icon before Plasma is started. > > Martin Tobias Holmedahl Sandsmark wrote: > it still creates a systray icon, it just creates an "old style" tray icon. > > Martin Gräßlin wrote: > > it still creates a systray icon, it just creates an "old style" tray > icon. > > Which won't work on Wayland. And yes that's a valid point as we need to > think further than 3 months ;-) > > Martin Gräßlin wrote: > actually it won't even work on X11 as if Plasma is not up yet, neither > will be the xembed proxy. Which means no systray and Qt won't create it. > > Martin Tobias Holmedahl Sandsmark wrote: > won't it jump into the xembed proxy when it appears? I seem to recall > that happening, but I might be wrong. and regarding wayland, what does the "normal" platform plugin for wayland do with QPlatformSystemTrayIcon? - Martin Tobias Holmedahl --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128473/#review97514 --- 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 > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 128473: Avoid recursive calls to QPlatformTheme::createPlatformSystemTrayIcon()
> On July 18, 2016, 5:49 a.m., Martin Gräßlin wrote: > > I acknowledge the problem in general, but I think the solution is wrong as > > this creates now a race condition on startup where apps don't show up in > > the systray at all. That is if an application tries to create a systray > > icon before Plasma is started. > > Martin Tobias Holmedahl Sandsmark wrote: > it still creates a systray icon, it just creates an "old style" tray icon. > > Martin Gräßlin wrote: > > it still creates a systray icon, it just creates an "old style" tray > icon. > > Which won't work on Wayland. And yes that's a valid point as we need to > think further than 3 months ;-) > > Martin Gräßlin wrote: > actually it won't even work on X11 as if Plasma is not up yet, neither > will be the xembed proxy. Which means no systray and Qt won't create it. won't it jump into the xembed proxy when it appears? I seem to recall that happening, but I might be wrong. - Martin Tobias Holmedahl --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128473/#review97514 --- 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 > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 128473: Avoid recursive calls to QPlatformTheme::createPlatformSystemTrayIcon()
> On July 18, 2016, 7:49 a.m., Martin Gräßlin wrote: > > I acknowledge the problem in general, but I think the solution is wrong as > > this creates now a race condition on startup where apps don't show up in > > the systray at all. That is if an application tries to create a systray > > icon before Plasma is started. > > Martin Tobias Holmedahl Sandsmark wrote: > it still creates a systray icon, it just creates an "old style" tray icon. > > Martin Gräßlin wrote: > > it still creates a systray icon, it just creates an "old style" tray > icon. > > Which won't work on Wayland. And yes that's a valid point as we need to > think further than 3 months ;-) actually it won't even work on X11 as if Plasma is not up yet, neither will be the xembed proxy. Which means no systray and Qt won't create it. - Martin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128473/#review97514 --- On July 17, 2016, 10: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, 10: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 > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 128473: Avoid recursive calls to QPlatformTheme::createPlatformSystemTrayIcon()
> On July 18, 2016, 7:49 a.m., Martin Gräßlin wrote: > > I acknowledge the problem in general, but I think the solution is wrong as > > this creates now a race condition on startup where apps don't show up in > > the systray at all. That is if an application tries to create a systray > > icon before Plasma is started. > > Martin Tobias Holmedahl Sandsmark wrote: > it still creates a systray icon, it just creates an "old style" tray icon. > it still creates a systray icon, it just creates an "old style" tray icon. Which won't work on Wayland. And yes that's a valid point as we need to think further than 3 months ;-) - Martin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128473/#review97514 --- On July 17, 2016, 10: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, 10: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 > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 128473: Avoid recursive calls to QPlatformTheme::createPlatformSystemTrayIcon()
> On July 18, 2016, 5:49 a.m., Martin Gräßlin wrote: > > I acknowledge the problem in general, but I think the solution is wrong as > > this creates now a race condition on startup where apps don't show up in > > the systray at all. That is if an application tries to create a systray > > icon before Plasma is started. it still creates a systray icon, it just creates an "old style" tray icon. - Martin Tobias Holmedahl --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128473/#review97514 --- 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 > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 128473: Avoid recursive calls to QPlatformTheme::createPlatformSystemTrayIcon()
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128473/#review97514 --- I acknowledge the problem in general, but I think the solution is wrong as this creates now a race condition on startup where apps don't show up in the systray at all. That is if an application tries to create a systray icon before Plasma is started. - Martin Gräßlin On July 17, 2016, 10: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, 10: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 > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
