Review Request 121885: Properly check for systray being available

[David Faure]

> On Jan. 7, 2015, 8:10 a.m., David Faure wrote:
> > src/platformtheme/kdeplatformsystemtrayicon.cpp, line 330
> > <https://git.reviewboard.kde.org/r/121885/diff/1/?file=338653#file338653line330>
> >
> >     I don't see a deadlock here, unless StatusNotifierWatcher calls this method itself :-)
> >     
> >     Just one thing though: this creation of a QDBusInterface will auto-start the service (i.e. autoload the kded module), if it wasn't loaded already. Is this desired?
> >     
> >     (so yeah, if the constructor calls this method we have a problem ;)
> 
> Martin Klapetek wrote:
>     I don't know the implementation in detail but I think that the watcher is watching for new SNHost and tells SNIs that new host is available, so that they could register with it. So I'd say it is desired that this service always runs.
>     
>     Can I ask how would this be autostarted? It doesn't install dbus service file (also the .desktop has autoload=true so it should be always present)

Ah OK, what I said doesn't apply then. I was assuming it had a .service file installed (and/or I was confusing with a call to org.kde.kded5 modules/foo, which would autoload it). But if it's loaded from the start then ok, it's either present (when kded5 is running) or not (no kded5, or no SNW module installed).


- David


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121885/#review73331
-----------------------------------------------------------


On Jan. 6, 2015, 6:16 p.m., Martin Klapetek wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121885/
> -----------------------------------------------------------
> 
> (Updated Jan. 6, 2015, 6:16 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Bugs: 339707
>     https://bugs.kde.org/show_bug.cgi?id=339707
> 
> 
> Repository: frameworkintegration
> 
> 
> Description
> -------
> 
> The "org.kde.StatusNotifierWatcher" is just a watcher/helper, not the actual systray object, that's "org.kde.StatusNotifierHost-$PID". Because Plasma appends the PID, we cannot check directly for it being present on the bus, so we check the org.kde.StatusNotifierWatcher.IsStatusNotifierHostRegistered property that's meant to be used for this.
> 
> Plus QSystemTrayIcon::isSystemTrayAvailable() is actually returning always true, because the Watcher is in kded and is /always/ present.
> 
> This also fixes many apps with KSNI crashing on plasma exit, bug 339707 (though I believe this is not the direct cause for that bug)
> 
> 
> Diffs
> -----
> 
>   src/platformtheme/kdeplatformsystemtrayicon.cpp b5e207c 
> 
> Diff: https://git.reviewboard.kde.org/r/121885/diff/
> 
> 
> Testing
> -------
> 
> Things do not crash anymore and the ::isSystemTrayAvailable() now returns correct value.
> 
> 
> Thanks,
> 
> Martin Klapetek
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20150107/fd2aec5f/attachment.html>