Review Request 120243: Remove fallback QSystemTray support

[David Edmundson]

> On Sept. 17, 2014, 12:55 p.m., Aleix Pol Gonzalez wrote:
> > Maybe we want to ifdef it? We won't require Qt 5.4 in a while...
> 
> David Edmundson wrote:
>     It doesn't require Qt5.4.
> 
> Martin Gräßlin wrote:
>     +1 for removing the fallback.
> 
> Luigi Toscano wrote:
>     I guess the ifdef part was about keeping the support until Qt 5.4 is mandatory for Frameworks.
>     
>     
>     I understand you want to get rid of this "legacy" support in Plasma, but forcing it on all application is in my opinion a different story. Isn't there any other solution that removing the code for supporting it on Qt 5.4?
> 
> Martin Gräßlin wrote:
>     > Isn't there any other solution that removing the code for supporting it on Qt 5.4?
>     
>     What could be tried is to use QSystemTrayIcon::isSystemTrayAvailable and not create a QSystemTrayIcon if it returns false.
>     
>     Nevertheless: I'm in favor of removing the code. It was a fallback for the transition to SNI. Nowadays SNI is widely adopted (e.g. Plasma 1 and 5 and Unity) and at the same time support for Xembedd is getting removed more and more and cannot be taken for granted (see initial comment about Wayland).
> 
> Luigi Toscano wrote:
>     Last note on my side: I understand the case of Wayland, but in that case isn't it possible to detect if running under Wayland and skip the fallback?
> 
> Martin Gräßlin wrote:
>     > I understand the case of Wayland, but in that case isn't it possible to detect if running under Wayland and skip the fallback?
>     
>     no, not really. The application in question might be connected to an XWayland server and think it's in an X session.

> QSystemTrayIcon::isSystemTrayAvailable 

is currently always return true in your QPT.
Possibly fixable, at which point the setLegacyMode can have a if () before it tries doing anything.

I made a rushed patch as we need something soonish.


- David


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


On Sept. 17, 2014, 12:07 p.m., David Edmundson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120243/
> -----------------------------------------------------------
> 
> (Updated Sept. 17, 2014, 12:07 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: knotifications
> 
> 
> Description
> -------
> 
> Remove fallback QSystemTray support
> 
> Currently when we make a StatusNotifierItem if it fails we try to create
> a classic X system tray icon via Qt's QSystemTray.
> 
> QSystemTray now (5.4) has a QPT backend that uses KStatusNotifierItem. This
> means it's possible to infinite loop.
> 
> This patch removes the legacy QSystemTray support to avoid that
> happening.
> 
> Downside: it means if a frameworks app is run on a platform without statusnotifier support but still has legacy X system tray support we would now gets no icons, but that would break on Wayland anyway.
> 
> 
> Diffs
> -----
> 
>   src/kstatusnotifieritem.h 7da1451 
>   src/kstatusnotifieritem.cpp 767537d 
>   src/kstatusnotifieritemprivate_p.h 051cc38 
> 
> Diff: https://git.reviewboard.kde.org/r/120243/diff/
> 
> 
> Testing
> -------
> 
> Ran statusnotifiertest (the one in plasma-workspace) with plasma running. Everything is fine.
> Ran without plasma running and nothing happened (without kded5 infinite looping)
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

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