[systemtray] Fix SNI icon not rendering
ClosedPublic

Authored by kmaterka on May 3 2020, 2:48 PM.

Details

Summary

In some rare situations SNI icons are not rendered. It happens randomly, only some users are affected. It does not happen on every login.
Only SNI icons are not rendered, Plasmoids are fine. Restarting plasmashell or re-adding systemtray applet helps.

In QML all roles need to be defined before model is used. When data engines is used a a source for a data model, all roles has to be defined and proper value assigned (not a null QVariant). StatusNotifierItemSource does this properly, but in some situations it sets null QVariant for Icon. Setting empty/null QVariant removes key/role from the date set (DataConteiner implementation). If data model was loaded earlier or later when Icon has proper value it will work properly. In some rare situation it is possible that data model is loaded when Icon has null value assigned (in other words - removed), role is removed from the data model and not avaiable to system tray applet.

This fix makes sure that there is always a value for Icon role. To check if icon is null native method has to be used - QML does not understand that QIcon can be null.

BUG: 419305
FIXED-IN: 5.18.5

Test Plan

I don't have any reliable method to reproduce this issue.

Diff Detail

Repository
R120 Plasma Workspace
Branch
Plasma/5.18
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 26274
Build 26292: arc lint + arc unit
kmaterka created this revision.May 3 2020, 2:48 PM
Restricted Application added a project: Plasma. · View Herald TranscriptMay 3 2020, 2:48 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
kmaterka requested review of this revision.May 3 2020, 2:48 PM

This patch is for "Plasma/5.18" branch only.
Can we have it merged before 5.18.5?

ngraham accepted this revision.May 4 2020, 1:54 PM

Great catch. The logic seems sane to me. Note that if you plan to land this on the stable branch, there will be merge conflicts that you'll need to resolve carefully, since it looks like this code has changed a bit between 5.18 and 5.19.

This revision is now accepted and ready to land.May 4 2020, 1:54 PM

Great catch. The logic seems sane to me. Note that if you plan to land this on the stable branch, there will be merge conflicts that you'll need to resolve carefully, since it looks like this code has changed a bit between 5.18 and 5.19.

I know, because I changed it :) In 5.19 entire data model was rewritten, so that this change might not be need or better approach can be used. I will check it.

kmaterka updated this revision to Diff 81903.May 4 2020, 2:44 PM

Additional check for real JavaScript null - to avoid conversion errors in native method.

ngraham accepted this revision.May 4 2020, 3:15 PM
This revision was automatically updated to reflect the committed changes.

Great thanks! Don't forget to merge Plasma/5.18 into master, fixing the merge conflicts.

Great thanks! Don't forget to merge Plasma/5.18 into master, fixing the merge conflicts.

Merged.