Port system tray away from SNI dataengine

[Konrad Materka]

Hi,

I think I'm finally ready to take https://phabricator.kde.org/T13319 :)
What is the best approach to DataEngine migration? It was/is a singleton,
but System Tray can be instanced several times. In addition the SNI model
can be quite heavy, it will use DBus a lot and will cache values (including
images).Is it OK to put it in SystemTrayModel or should I create a
singleton/plugin?

Is there any example (similar migration) that I can check?

-- 
Regards,
Konrad Materka

Re: A lot of "Timers cannot be started from another thread" warnings

[Konrad Materka]

Nevermind...
https://invent.kde.org/frameworks/plasma-framework/-/merge_requests/178

sob., 26 gru 2020 o 11:51 Konrad Materka  napisał(a):

> Hi,
>
> In latest version compiled from sources, I have a lot of warnings like
> these:
> QObject::startTimer: Timers cannot be started from another thread
> QObject::killTimer: Timers cannot be stopped from another thread
> I don't have them if I start a stable version (5.20.4). I'm using KDE Neon.
>
> Is this only me?
>
> --
> Regards:
> Konrad Materka
>


-- 
Pozdrawiam:
Konrad Materka

A lot of "Timers cannot be started from another thread" warnings

[Konrad Materka]

Hi,

In latest version compiled from sources, I have a lot of warnings like
these:
QObject::startTimer: Timers cannot be started from another thread
QObject::killTimer: Timers cannot be stopped from another thread
I don't have them if I start a stable version (5.20.4). I'm using KDE Neon.

Is this only me?

-- 
Regards:
Konrad Materka

Re: Fix 'Connections' warnings in QML

[Konrad Materka]

Aleix,

Yes, I know frameworks have lower dependencies - I just wanted to fix QML
warnings in Plasma Workspace. The question is:
Is it OK to do this in one bug change, or should I split it into smaller?

Konrad

pon., 9 lis 2020 o 14:23 Aleix Pol  napisał(a):

> Hi Konrad,
> It's okay to fix them in Plasma. Most of the ones we still have though
> are in frameworks that still depend on Qt 5.12 and it's not trivial to
> address this problem over there.
>
> Aleix
>
> On Mon, Nov 9, 2020 at 10:25 AM Konrad Materka  wrote:
> >
> > Hi,
> >
> > With Qt 5.15 we have tons of warnings like:
> > QML Connections: Implicitly defined onFoo properties in Connections are
> deprecated.
> >
> > Qt 5.15 is now a minimum requirement for Plasma Workspace, so nothing is
> blokcking to migrate to new syntax.
> >
> > Is it OK to fix them in one massive MR or better to do that one by one
> (I mean one applet/module at a time)?
>

Fix 'Connections' warnings in QML

[Konrad Materka]

Hi,

With Qt 5.15 we have tons of warnings like:
QML Connections: Implicitly defined onFoo properties in Connections are
deprecated.

Qt 5.15 is now a minimum requirement for Plasma Workspace, so nothing is
blokcking to migrate to new syntax.

Is it OK to fix them in one massive MR or better to do that one by one (I
mean one applet/module at a time)?

-- 
Regards:
Konrad Materka

D26992: [SystemTray] Use unified data model everywhere

[Konrad Materka]

kmaterka marked an inline comment as done.
kmaterka added inline comments.

INLINE COMMENTS

> kossebau wrote in systemtray.h:100
> Thanks for your reply. Okay, so seems you did not hit anything related.
> 
> Thing is, you added
> ``
> Q_INVOKABLE Plasma::Service *serviceForSource(const QString );
> 
>   here, whereas the JavaScript code before was calling serviceForSource() on 
> a Plasma::DataSource class, which does not return the type "Plasma::Service 
> *", but "QObject *", cmp. its method definition
> 
> Q_INVOKABLE QObject *serviceForSource(const QString );
> 
>   and the JavaScript engine for a plain QObject simply exposes any 
> Q_INVOKABLEs and slot methods, that's why no extra registration would be 
> needed (AFAIK).
>   
>   So still a mystery to me why this here seems to work, too bad :)

Oh, OK, now I remember, sorry for misinforming you...

I haven't had any problems with this, it "Just Worked" :) Maybe it should 
return just plain QObject*, I'm not experienced in this area, what do you think?

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D26992

To: kmaterka, #plasma_workspaces, #plasma, davidedmundson, ngraham, broulik
Cc: kossebau, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26992: [SystemTray] Use unified data model everywhere

[Konrad Materka]

kmaterka marked an inline comment as done.
kmaterka added inline comments.

INLINE COMMENTS

> kossebau wrote in systemtray.h:100
> Hi. Seeing this code, I have a question: from what I understood so far, for 
> this method to be useable from the JavaScript inside QML, the type 
> "Plasma::Service *" needs to be registered with at least
> 
>   qRegisterMetaType("Plasma::Service*");
> 
> or
> 
>   qRegisterMetaType();
> 
> Yet I have not found any code in Plasma related projects which calls this.
> 
> The module org.kde.plasma.core only has code to register the unnamespaced 
> type name ""Service*" (indirectly via 
> `qmlRegisterInterface("Service");` which internally calls 
> the equivalent of `qRegisterMetaType("Service*");`.
> 
> And the namespace in the registered type name string seems important, things 
> broke elsewhere if the type signature used in the Q_INVOKABLE was different 
> WRT namespace compared to the registered type name string.
> 
> Do you remember anything related or would have some insights?

I don't know, this code was there before my changes, I've just done some 
refactoring here.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D26992

To: kmaterka, #plasma_workspaces, #plasma, davidedmundson, ngraham, broulik
Cc: kossebau, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29395: Refactor MediaController

[Konrad Materka]

kmaterka added a comment.


  Any update on this? I really like the idea.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D29395

To: cblack, #plasma
Cc: kmaterka, iasensio, broulik, trmdi, ngraham, plasma-devel, Orage, 
LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, ragreen, 
ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, ahiemstra, mart

Remove stale branches?

[Konrad Materka]

Hi,

After GitLab migration it is very easy to fork repository, so that
branches can live in forks.
Should we remove all personal branches from main repository? Or at
least stale ones?

-- 
Regards,
Konrad Materka

Re: GitLab - merging fixes from release branch into master

[Konrad Materka]

niedz., 24 maj 2020 o 20:17 Nate Graham  wrote:
> Land it on Plasma/5.19, then manually merge Plasma/5.19 into master,
> same as before.
OK, thank you for confirmation.

-- 
Regards:
Konrad Materka

GitLab - merging fixes from release branch into master

[Konrad Materka]

Hi,

What is the current procedure of merging fixes from release branch
back into master? For example I made a fix for Plasma/5.19 but I want
it in master as well. Merge Request from Plasma/5.19 into master has
conflicts, how to fix them in new GitLab flow?
Sorry if it was discussed already, but I was not able to find any information.

-- 
Regards,
Konrad Materka

D29827: Give users the ability to disable the microphone indicator

[Konrad Materka]

kmaterka added a comment.


  In D29827#673720 , @ngraham wrote:
  
  > In D29827#673615 , @meven wrote:
  >
  > > In D29827#673611 , 
@davidedmundson wrote:
  > >
  > > > It's an SNI, I thought the systemtray could already filter SNIs in the  
enties tab of the system tray?
  > >
  > >
  > > Currently it appears in the systray config only when the microphone is 
activated.
  > >  I guess we should make it more permanent.
  >
  >
  > Yes, that seems like a better option. Then the existing config UI will be 
used for this, and can be used to disable other SNIs too.
  
  
  It is not that easy. SNI is just a protocol for system tray icons. Currently 
it is not possible to disable SNI icon entirely from symtem tray settings - you 
can only hide it (to the hidden icons view). It is a responsibility of the 
application to decide if icon is needed or not and give user the option to 
disable the system tray icon (some applications have such option).
  I'm not sure if having an option to disable SNI icon is a good idea. If you 
don't want the icon, do not start an app in the first place or hide it or ask 
author of an app for an option to disable tray icon. 
  In case of Microphone Indicator, it will run and take resources, it will try 
to create an icon and will take the resources. If we give users an option to 
disable SNI icons in system tray settings it may look like disabling the 
service entirely, which is not true.
  
  IMO the initial idea is good - add an option to plasma-pa settings.

REPOSITORY
  R115 Plasma Audio Volume Applet

REVISION DETAIL
  https://phabricator.kde.org/D29827

To: kurmikon, ngraham, #vdg, #plasma
Cc: kmaterka, davidedmundson, meven, bcooksley, ngraham, plasma-devel, Orage, 
LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, ragreen, 
ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, ahiemstra, mart

D29544: [applets/systemtray] Show base icon when AttentionIcon not set

[Konrad Materka]

This revision was automatically updated to reflect the committed changes.
Closed by commit R120:00e64bdb3fa9: [applets/systemtray] Show base icon when 
AttentionIcon not set (authored by kmaterka).

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29544?vs=82321=82591

REVISION DETAIL
  https://phabricator.kde.org/D29544

AFFECTED FILES
  applets/systemtray/systemtraymodel.cpp

To: kmaterka, #plasma_workspaces, #plasma, ngraham, broulik, davidedmundson, 
mart
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29544: [applets/systemtray] Show base icon when AttentionIcon not set

[Konrad Materka]

kmaterka added a comment.


  Can we have it reviewed before 5.19 release?

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D29544

To: kmaterka, #plasma_workspaces, #plasma, ngraham, broulik, davidedmundson
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29544: [applets/systemtray] Show base icon when AttentionIcon not set

[Konrad Materka]

kmaterka added a comment.


  Firstly, AttentionIcon support was introduced in D24865 
. It had a bug, fixed for 5.18 in D29386 
. When I merged branch 5.18 (with D29386 
) into master, by mistake I forgot about 
AttentionIcon handling... This change fixes the incomplete merge.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D29544

To: kmaterka, #plasma_workspaces, #plasma, ngraham, broulik, davidedmundson
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29544: [applets/systemtray] Show base icon when AttentionIcon not set

[Konrad Materka]

kmaterka created this revision.
kmaterka added reviewers: Plasma: Workspaces, Plasma, ngraham, broulik, 
davidedmundson.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
kmaterka requested review of this revision.

REVISION SUMMARY
  If status is set to NeedsAttention it tries to use Attention Icon. If 
Attention Icon is not set, empty/null QIcon is set. QML can't check if QIcon is 
null or not, as a result it tries to use empty icon and nothing is rendered.
  Set null QVariant if AttentionIcon is not valid so that QML check will work 
correctly.

TEST PLAN
  - Set NeedsAttention as a status and with no Attention Icon
  - Base Icon should render correctly

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D29544

AFFECTED FILES
  applets/systemtray/systemtraymodel.cpp

To: kmaterka, #plasma_workspaces, #plasma, ngraham, broulik, davidedmundson
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29529: Use INTERFACE IMPORTED target type instead of ALIAS for compatibility targets

[Konrad Materka]

kmaterka accepted this revision.
kmaterka added a comment.


  plasma-workspace compiles using cmake 3.10 without issues now.

REPOSITORY
  R111 KSysguard Library

BRANCH
  ancient_cmake_fix

REVISION DETAIL
  https://phabricator.kde.org/D29529

To: ahiemstra, #plasma, ngraham, kmaterka
Cc: kmaterka, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29302: Use KSysGuard namespace for KSysGuard library targets

[Konrad Materka]

kmaterka added a comment.


  In D29302#666138 , @ahiemstra 
wrote:
  
  > Which version of CMake are you using? I needed to promote the imported 
KSysGuard targets to "global" using set_target_property. Without that, I saw 
similar errors. The IMPORTED_GLOBAL property was added in CMake 3.11.
  
  
  Cmake version: 3.10.2
  
  Is is a default in Neon, based on Ubuntu 18.04.

REPOSITORY
  R111 KSysguard Library

REVISION DETAIL
  https://phabricator.kde.org/D29302

To: ahiemstra, #plasma, davidedmundson
Cc: kmaterka, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29302: Use KSysGuard namespace for KSysGuard library targets

[Konrad Materka]

kmaterka added a comment.


  With this change, plasma-workspace fails to build on my system (Neon User, 
kdesrc-build)
  
  Error message:
  
CMake Warning at 
/home/konrad/kde/usr/lib/x86_64-linux-gnu/cmake/KF5SysGuard/KF5SysGuardConfig.cmake:27
 (message):
  The KF5:: namespace for KSysGuard targets is deprecated.  Please use
  KSysGuard as namespace.
Call Stack (most recent call first):
  /home/konrad/kde/usr/share/ECM/find-modules/FindKF5.cmake:74 
(find_package)
  CMakeLists.txt:55 (find_package)


CMake Error at 
/home/konrad/kde/usr/lib/x86_64-linux-gnu/cmake/KF5SysGuard/KF5SysGuardConfig.cmake:34
 (add_library):
  add_library cannot create ALIAS target "KF5::SysGuard" because target
  "KSysGuard::SysGuard" is IMPORTED.
Call Stack (most recent call first):
  
/home/konrad/kde/usr/lib/x86_64-linux-gnu/cmake/KF5SysGuard/KF5SysGuardConfig.cmake:37
 (add_sysguard_target)
  /home/konrad/kde/usr/share/ECM/find-modules/FindKF5.cmake:74 
(find_package)
  CMakeLists.txt:55 (find_package)


CMake Error at 
/home/konrad/kde/usr/lib/x86_64-linux-gnu/cmake/KF5SysGuard/KF5SysGuardConfig.cmake:34
 (add_library):
  add_library cannot create ALIAS target "KF5::ProcessCore" because target
  "KSysGuard::ProcessCore" is IMPORTED.
Call Stack (most recent call first):
  
/home/konrad/kde/usr/lib/x86_64-linux-gnu/cmake/KF5SysGuard/KF5SysGuardConfig.cmake:38
 (add_sysguard_target)
  /home/konrad/kde/usr/share/ECM/find-modules/FindKF5.cmake:74 
(find_package)
  CMakeLists.txt:55 (find_package)


CMake Error at 
/home/konrad/kde/usr/lib/x86_64-linux-gnu/cmake/KF5SysGuard/KF5SysGuardConfig.cmake:34
 (add_library):
  add_library cannot create ALIAS target "KF5::ProcessUi" because target
  "KSysGuard::ProcessUi" is IMPORTED.
Call Stack (most recent call first):
  
/home/konrad/kde/usr/lib/x86_64-linux-gnu/cmake/KF5SysGuard/KF5SysGuardConfig.cmake:39
 (add_sysguard_target)
  /home/konrad/kde/usr/share/ECM/find-modules/FindKF5.cmake:74 
(find_package)
  CMakeLists.txt:55 (find_package)


CMake Error at 
/home/konrad/kde/usr/lib/x86_64-linux-gnu/cmake/KF5SysGuard/KF5SysGuardConfig.cmake:34
 (add_library):
  add_library cannot create ALIAS target "KF5::LsofUi" because target
  "KSysGuard::LsofUi" is IMPORTED.
Call Stack (most recent call first):
  
/home/konrad/kde/usr/lib/x86_64-linux-gnu/cmake/KF5SysGuard/KF5SysGuardConfig.cmake:40
 (add_sysguard_target)
  /home/konrad/kde/usr/share/ECM/find-modules/FindKF5.cmake:74 
(find_package)
  CMakeLists.txt:55 (find_package)


CMake Error at 
/home/konrad/kde/usr/lib/x86_64-linux-gnu/cmake/KF5SysGuard/KF5SysGuardConfig.cmake:34
 (add_library):
  add_library cannot create ALIAS target "KF5::SignalPlotter" because target
  "KSysGuard::SignalPlotter" is IMPORTED.
Call Stack (most recent call first):
  
/home/konrad/kde/usr/lib/x86_64-linux-gnu/cmake/KF5SysGuard/KF5SysGuardConfig.cmake:41
 (add_sysguard_target)
  /home/konrad/kde/usr/share/ECM/find-modules/FindKF5.cmake:74 
(find_package)
  CMakeLists.txt:55 (find_package)


-- Found KF5SysGuard: 
/home/konrad/kde/usr/lib/x86_64-linux-gnu/cmake/KF5SysGuard/KF5SysGuardConfig.cmake

REPOSITORY
  R111 KSysguard Library

REVISION DETAIL
  https://phabricator.kde.org/D29302

To: ahiemstra, #plasma, davidedmundson
Cc: kmaterka, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28208: Move sni icon handling logic from data engine to applet

[Konrad Materka]

kmaterka added a comment.


  @davidre can you extract your test improvement to a separate patch? it would 
help me in another issue and I don't want to "steal" your code :)

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D28208

To: davidre, kmaterka, broulik, mart, #plasma, #vdg, #frameworks
Cc: bruns, ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28180: [applets/systemtray] Hide/show expander arrow - regression fix

[Konrad Materka]

This revision was automatically updated to reflect the committed changes.
Closed by commit R120:b34e88da7a5d: [applets/systemtray] Hide/show expander 
arrow - regression fix (authored by kmaterka).

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28180?vs=82205=82213

REVISION DETAIL
  https://phabricator.kde.org/D28180

AFFECTED FILES
  applets/systemtray/package/contents/ui/ExpanderArrow.qml
  applets/systemtray/package/contents/ui/HiddenItemsView.qml

To: kmaterka, #plasma_workspaces, #plasma, ngraham, broulik, davidedmundson
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28180: [applets/systemtray] Hide/show expander arrow - regression fix

[Konrad Materka]

kmaterka updated this revision to Diff 82205.
kmaterka added a comment.


  Rebase

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28180?vs=78148=82205

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D28180

AFFECTED FILES
  applets/systemtray/package/contents/ui/ExpanderArrow.qml
  applets/systemtray/package/contents/ui/HiddenItemsView.qml

To: kmaterka, #plasma_workspaces, #plasma, ngraham, broulik, davidedmundson
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29386: [systemtray] Fix SNI icon not rendering

[Konrad Materka]

kmaterka added a comment.


  In D29386#663325 , @ngraham wrote:
  
  > Great thanks! Don't forget to merge `Plasma/5.18` into master, fixing the 
merge conflicts.
  
  
  Merged.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D29386

To: kmaterka, #plasma_workspaces, #plasma, davidedmundson, broulik, ngraham
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29386: [systemtray] Fix SNI icon not rendering

[Konrad Materka]

This revision was automatically updated to reflect the committed changes.
Closed by commit R120:77975468dc10: [systemtray] Fix SNI icon not rendering 
(authored by kmaterka).

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29386?vs=81903=81925

REVISION DETAIL
  https://phabricator.kde.org/D29386

AFFECTED FILES
  applets/systemtray/package/contents/ui/items/StatusNotifierItem.qml
  applets/systemtray/systemtray.cpp
  applets/systemtray/systemtray.h
  dataengines/statusnotifieritem/statusnotifieritemsource.cpp

To: kmaterka, #plasma_workspaces, #plasma, davidedmundson, broulik, ngraham
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29386: [systemtray] Fix SNI icon not rendering

[Konrad Materka]

kmaterka updated this revision to Diff 81903.
kmaterka added a comment.


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

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29386?vs=81803=81903

BRANCH
  Plasma/5.18

REVISION DETAIL
  https://phabricator.kde.org/D29386

AFFECTED FILES
  applets/systemtray/package/contents/ui/items/StatusNotifierItem.qml
  applets/systemtray/systemtray.cpp
  applets/systemtray/systemtray.h
  dataengines/statusnotifieritem/statusnotifieritemsource.cpp

To: kmaterka, #plasma_workspaces, #plasma, davidedmundson, broulik, ngraham
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29386: [systemtray] Fix SNI icon not rendering

[Konrad Materka]

kmaterka added a comment.


  In D29386#662960 , @ngraham wrote:
  
  > 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.

REPOSITORY
  R120 Plasma Workspace

BRANCH
  Plasma/5.18

REVISION DETAIL
  https://phabricator.kde.org/D29386

To: kmaterka, #plasma_workspaces, #plasma, davidedmundson, broulik, ngraham
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29344: [applets/systemtray] Fix invisible plasmoid icons when Media Player is added

[Konrad Materka]

kmaterka added a comment.


  Separate Bug 420993  created for 
Media Player icon not rendering correctly.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D29344

To: kmaterka, #plasma_workspaces, #plasma, ngraham, broulik, apol, 
davidedmundson, mart
Cc: apol, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D29344: [applets/systemtray] Fix invisible plasmoid icons when Media Player is added

[Konrad Materka]

This revision was automatically updated to reflect the committed changes.
Closed by commit R120:026a29174279: [applets/systemtray] Fix invisible plasmoid 
icons when Media Player is added (authored by kmaterka).

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29344?vs=81842=81858

REVISION DETAIL
  https://phabricator.kde.org/D29344

AFFECTED FILES
  applets/systemtray/package/contents/ui/items/ItemLoader.qml
  applets/systemtray/systemtraymodel.cpp

To: kmaterka, #plasma_workspaces, #plasma, ngraham, broulik, apol, 
davidedmundson, mart
Cc: apol, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D29344: [applets/systemtray] Fix invisible plasmoid icons when Media Player is added

[Konrad Materka]

kmaterka updated this revision to Diff 81842.
kmaterka added a comment.


  Change warning message

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29344?vs=81724=81842

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D29344

AFFECTED FILES
  applets/systemtray/package/contents/ui/items/ItemLoader.qml
  applets/systemtray/systemtraymodel.cpp

To: kmaterka, #plasma_workspaces, #plasma, ngraham, broulik, apol, 
davidedmundson
Cc: apol, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D29344: [applets/systemtray] Fix invisible plasmoid icons when Media Player is added

[Konrad Materka]

kmaterka added a comment.


  In D29344#662586 , @ngraham wrote:
  
  > 5.19 hasn't branched yet, so code landed in master will still make it there.
  
  
  Maybe there was code freeze for frameworks? Or 5.18.5? Never mind :)
  
  > With this patch, I still see Media Player itself with a blank icon after 
opening Gwenview. Is that expected?
  
  This is a separate issue... :/ I will work on that and create separate fix.

INLINE COMMENTS

> ngraham wrote in ItemLoader.qml:33
> "Illegal" is one of those scary words we try not to use, even in console 
> spew. Consider changing this to "Invalid."
> 
> F8282506: Screenshot_20200503_214934.png 
> 

Old habits from Java world (IllegalStateException), I will change that.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D29344

To: kmaterka, #plasma_workspaces, #plasma, ngraham, broulik, apol, 
davidedmundson
Cc: apol, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D29386: [systemtray] Fix SNI icon not rendering

[Konrad Materka]

kmaterka added a comment.


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

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D29386

To: kmaterka, #plasma_workspaces, #plasma, davidedmundson, broulik, ngraham
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29356: [applets/systemtray] Fix value read in some onXyxChanged connections

[Konrad Materka]

This revision was automatically updated to reflect the committed changes.
Closed by commit R120:efe4b603e92f: [applets/systemtray] Fix value read in some 
onXyxChanged connections (authored by kmaterka).

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29356?vs=81726=81804

REVISION DETAIL
  https://phabricator.kde.org/D29356

AFFECTED FILES
  applets/systemtray/package/contents/ui/main.qml

To: kmaterka, #plasma_workspaces, #plasma, ngraham, apol, broulik, 
davidedmundson
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29386: [systemtray] Fix SNI icon not rendering

[Konrad Materka]

kmaterka created this revision.
kmaterka added reviewers: Plasma: Workspaces, Plasma, davidedmundson, broulik, 
ngraham.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
kmaterka requested review of this revision.

REVISION 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.

REPOSITORY
  R120 Plasma Workspace

BRANCH
  Plasma/5.18

REVISION DETAIL
  https://phabricator.kde.org/D29386

AFFECTED FILES
  applets/systemtray/package/contents/ui/items/StatusNotifierItem.qml
  applets/systemtray/systemtray.cpp
  applets/systemtray/systemtray.h
  dataengines/statusnotifieritem/statusnotifieritemsource.cpp

To: kmaterka, #plasma_workspaces, #plasma, davidedmundson, broulik, ngraham
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29380: [System Tray] Always draw the highlight behind the icon

[Konrad Materka]

kmaterka accepted this revision.
kmaterka added a comment.
This revision is now accepted and ready to land.


  OK, accepted

REPOSITORY
  R120 Plasma Workspace

BRANCH
  draw-behind-icons (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D29380

To: filipf, kmaterka, #plasma
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29380: [System Tray] Always draw the highlight behind the icon

[Konrad Materka]

kmaterka requested changes to this revision.
kmaterka added a comment.
This revision now requires changes to proceed.


  The same treatment is needed in `HiddenItemsView` to. Just move `z: -1` to 
`CurrentItemHighLight` component so that your fix will cover both cases.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D29380

To: filipf, kmaterka, #plasma
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29356: [applets/systemtray] Fix value read in some onXyxChanged connections

[Konrad Materka]

kmaterka added a comment.


  Fix exctracted from D29344 , to make 
things clear - one commit, one change.

INLINE COMMENTS

> main.qml:81
>  target: plasmoid
> -function onUserConfiguringChanged(userConfiguring) {
> -if (userConfiguring) {
> +function onUserConfiguringChanged() {
> +if (plasmoid.userConfiguring) {

`Plasma::Applet::userConfiguringChanged` sends value, but 
`Plasma::AppletInterface::userConfiguringChanged` does not (signal has no 
parameters).

> main.qml:91
>  
> -function onExtraItemsChanged(extraItems) {
> -plasmoid.nativeInterface.allowedPlasmoids = extraItems
> +function onExtraItemsChanged() {
> +plasmoid.nativeInterface.allowedPlasmoids = 
> plasmoid.configuration.extraItems

I'm pretty sure this worked before... Maybe something changed in 
`QQmlPropertyMap` in Qt 5.14?

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D29356

To: kmaterka, #plasma_workspaces, #plasma, ngraham, apol
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29356: [applets/systemtray] Fix value read in some onXyxChanged connections

[Konrad Materka]

kmaterka created this revision.
kmaterka added reviewers: Plasma: Workspaces, Plasma, ngraham, apol.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
kmaterka requested review of this revision.

REVISION SUMMARY
  Some `onXyzChanged` signals do not send value - fix connection to use value 
directly from the sender.

TEST PLAN
  Disable or enable plasmoid in System Tray setting - it should work correctly.

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D29356

AFFECTED FILES
  applets/systemtray/package/contents/ui/main.qml

To: kmaterka, #plasma_workspaces, #plasma, ngraham, apol
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29344: [applets/systemtray] Fix invisible plasmoid icons when Media Player is added

[Konrad Materka]

kmaterka updated this revision to Diff 81724.
kmaterka added a comment.


  Extract unrelated fix to seprate patch

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29344?vs=81698=81724

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D29344

AFFECTED FILES
  applets/systemtray/package/contents/ui/items/ItemLoader.qml
  applets/systemtray/systemtraymodel.cpp

To: kmaterka, #plasma_workspaces, #plasma, ngraham
Cc: apol, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D29344: [applets/systemtray] Fix invisible plasmoid icons when Media Player is added

[Konrad Materka]

kmaterka added inline comments.

INLINE COMMENTS

> apol wrote in main.qml:92
> Why's this needed?

This is needed to fix different bug. I will create a separate patch for this.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D29344

To: kmaterka, #plasma_workspaces, #plasma, ngraham
Cc: apol, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D29344: [applets/systemtray] Fix invisible plasmoid icons when Media Player is added

[Konrad Materka]

kmaterka added a comment.


  This fix should go with Plasma 5.19, if it is not too late (code freeze).

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D29344

To: kmaterka, #plasma_workspaces, #plasma, ngraham
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29344: [applets/systemtray] Fix ivisible plasmoid icons when new plasmoid added

[Konrad Materka]

kmaterka created this revision.
kmaterka added reviewers: Plasma: Workspaces, Plasma, ngraham.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
kmaterka requested review of this revision.

REVISION SUMMARY
  After adding a new plasmoid (applet), for example a new Media Player, several 
empty icons are rendered in the system tray which hides existing icons.
  The bug occurs after opening Gwenview while the Media Player applet is NOT in 
use.
  The systray icons reappear when Gwenview is closed or another source starts 
playing.
  
  BUG: 418662

TEST PLAN
  Start Gwenview when Media Player is not in use. All icons should render 
correctly.

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D29344

AFFECTED FILES
  applets/systemtray/package/contents/ui/items/ItemLoader.qml
  applets/systemtray/package/contents/ui/main.qml
  applets/systemtray/systemtraymodel.cpp

To: kmaterka, #plasma_workspaces, #plasma, ngraham
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29130: [Applet] Show in system tray iff at least one vault is open

[Konrad Materka]

kmaterka added a comment.


  IMO it is the best **not** to add new status in System Tray - it is already 
quite confusing with "Disabled" state, the 3 other (Always hidden, Shown when 
relevant and Always shown) are OK and self-explanatory.
  "Shown when relevant" might be tricky and we will never please everyone. As 
stated in previous comment, this is tricky.
  
  Currently the decision is in the developer's hand only. There can be a 
configuration option, but too many options is also confusing. For configuration 
option, there additional questions: how to describe it with simple label? will 
users understand the purpose? what if applet is standalone (not in systemtray)? 
will user ever use it? is it easily discoverable? etc. If someone doesn't like 
default behavior it is always possible to force Shown/Hidden state. After all, 
it is not that users delete all vaults and creates new one on everyday basis.
  
  Personally I like the idea to hide Vaults icon when no vault is opened. I 
have one configured, but I use it very rarely, so for **me** it is better :) 
From the other side, Bluetooth and Network icons are visible not only when 
something is connected but also when connection is possible 
(Bluetooth/networking  is enabled). So maybe it would be more consistent if we 
leave it as it is now, wouldn't it? For someone who is using vaults often 
hiding Vaults icon might be really annoying. I don't know what's the best...
  
  BTW. For reference, MS Windows tries to hide as much as possible. But they 
are trying to kill system tray, so sometimes it is taken to the extreme and 
annoying.

REPOSITORY
  R845 Plasma Vault

REVISION DETAIL
  https://phabricator.kde.org/D29130

To: ksmanis, #plasma, #vdg, ivan
Cc: kmaterka, broulik, ngraham, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D28631: Remove Icons from system tray item tooltips

[Konrad Materka]

kmaterka accepted this revision.
kmaterka added a comment.
This revision is now accepted and ready to land.


  In D28631#643325 , @ngraham wrote:
  
  > Remove no-longer-relevant reference to icon in `AbstractItem.qml`
  
  
  Thanks, accepted.

REPOSITORY
  R120 Plasma Workspace

BRANCH
  remove-systemtray-tooltip-icons (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D28631

To: ngraham, #plasma, davidre, broulik, kmaterka
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28631: Remove Icons from system tray item tooltips

[Konrad Materka]

kmaterka requested changes to this revision.
kmaterka added a comment.
This revision now requires changes to proceed.


  In D28631#643116 , @davidre wrote:
  
  > I would argue that the implementation is free to ignore it 
  >  `Data structure that describes extra information associated to this item, 
that can be visualized for instance by a tooltip (or by any other mean the 
visualization consider appropriate.`
  >  The appropriate one being not at all in this case ;)
  
  
  OK, makes sense.
  
  @ngraham Only one little detail: please remove **`icon` ** from comment `/* 
subclasses need to assign to this tooltip properties` in `AbstractItem.qml`

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D28631

To: ngraham, #plasma, davidre, broulik, kmaterka
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28208: Move sni icon handling logic from data engine to applet

[Konrad Materka]

kmaterka added a comment.


  I looked at the `IconItem` implementation. Before add theme directory support 
I would like to perform some refactoring - D28470 
.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D28208

To: davidre, kmaterka, broulik, mart, #plasma, #vdg
Cc: bruns, ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28631: Remove Icons from system tray item tooltips

[Konrad Materka]

kmaterka added a comment.


  For SNI, maybe we should consider showing an icon, but only if it is 
explicitly set? It is part of the specification 

 after all. From the other side it will be inconsistent...

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D28631

To: ngraham, #plasma, davidre, broulik, kmaterka
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28208: Move sni icon handling logic from data engine to applet

[Konrad Materka]

kmaterka added a comment.


  In D28208#638846 , @davidre wrote:
  
  > Yes it's not pretty but I had no other Idea. One option would be trying to 
manually search for the icon in the path?
  
  
  Not a good idea, theme path only point to root of more complex structure like 
`hicolor/22x22/apps/...`. This is also code/logic duplication.
  
  > Or if all else fails for now use the old icon property in case 
`IconThemePath` is set.
  
  As a temporary workaround - yes. Final goal is to add custom icon loader 
support to PlasmaCore.IconItem. I checked code, I see great potential for 
additional refactoring there :)

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D28208

To: davidre, kmaterka, broulik, mart, #plasma, #vdg
Cc: bruns, ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28208: Move sni icon handling logic from data engine to applet

[Konrad Materka]

kmaterka added a comment.


  The best would be to add theme path support to `PlasmaCore.IconItem`. I will 
take a look at this tomorrow.

INLINE COMMENTS

> davidre wrote in systemtraymodel.cpp:331
> This works now, but I don't know how to manage the KIconLoader. The 
> KIconEngines are managed by QIcon (they take ownership of the engine). One 
> idea would be to add it to the item maybe?

I don't like this idea. Now it is just a copy from one model to another without 
much of improvements - in fact now it complicates it even more.  KIconLoader is 
already created in StatusNotifierItemSource, it has little justification to 
create another one in data layer.

> systemtraymodel.cpp:339
> +if(iconThemePath.isEmpty()) {
> +dataItem->setData(iconName, static_cast(role));
> +} else {

Beside formatting (spaces, minor thing), I liked the logic before - the 
presentation layer decided what to render and how, model just provided data.
In addition, *Icon role can have two different types now - it should work with 
`dynamicRoles` enabled, but sometimes it has unexpected consequences. I had a 
bad experience with dynamic data type changes.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D28208

To: davidre, kmaterka, broulik, mart, #plasma, #vdg
Cc: bruns, ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28208: Move sni icon handling logic from data engine to applet

[Konrad Materka]

kmaterka added a comment.


  In D28208#634593 , @davidre wrote:
  
  > Do you know if `IconThemePath` is specified somewhere?
  
  
  Initial commit:
  
https://github.com/KDE/kde-workspace/commit/4c553dbf39f4189290387bba35589200ca051084
  Someone mentioned this property here:
  https://lists.freedesktop.org/archives/xdg/2015-December/013620.html
  https://mail.kde.org/pipermail/plasma-devel/2015-December/047019.html
  But he tried to create new specification. What we need is to update existing 
one to reflect all the changes...
  
  > What I could observe is that some apps create a folder hierachy under this 
path and palce their icon there. I currently don't have any idea how to handle 
this in qml. Maybe we need to do this in the model / c++  side again?
  
  Currently it is implemented in `statusnotifieritemsource.cpp`, it 
reconfigures KIconLoder to use themePath as additional icon location. 
  The best would be to have a similar implementation in `PlasmaCore.IconItem`, 
but this is a lot to change. `IconItem` is using global instance only 
`KIconLoader::global()`, while `StatusNotifierItemSource` has custom one (when 
needed) and fallbacks to the global one 
(`StatusNotifierItemSource::iconLoader()`).

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D28208

To: davidre, kmaterka, broulik, mart, #plasma, #vdg
Cc: bruns, ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28208: Move sni icon handling logic from data engine to applet

[Konrad Materka]

kmaterka added a comment.


  In D28208#634593 , @davidre wrote:
  
  > Do you know if `IconThemePath` is specified somewhere?
  
  
  Initial commit:
  
  What I could observe is that some apps create a folder hierachy under this 
path and palce their icon there. I currently don't have any idea how to handle 
this in qml. Maybe we need to do this in the model / c++  side again?
  
  > For example 
  >  `Arguments: [Variant(QString): 
"/tmp/.org.chromium.Chromium.3WvReV/icons"]` as IconThemepath 
  >  and inside is 
`/tmp/.org.chromium.Chromium.3WvReV/icons/hicolor/22x22/apps/chrome_app_indicator2_91c0c89ea21003eb35a74c9caa4718cc.png`

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D28208

To: davidre, kmaterka, broulik, mart, #plasma, #vdg
Cc: bruns, ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28208: Move sni icon handling logic from data engine to applet

[Konrad Materka]

kmaterka added a comment.


  > Some of the icons are not rendering now, for example:
  >  Discord - iconName: chrome_app_indicator2_83406baa15c6e4f2766a6e3714fbee20
  >  Skype1 - iconName: chrome_app_indicator2_01cdeaaa54803d9c9c158a5cdcb1fbfb
  >  I'm pretty sure I saw a workaround for that somewhere, but I can't recall 
where...
  
  Found it! You need to handle `IconThemePath`, which can be difficult... :/

INLINE COMMENTS

> StatusNotifierItem.qml:30
>  subText: model.ToolTipSubTitle
>  icon: model.ToolTipIcon !== "" ? model.ToolTipIcon : model.Icon ? 
> model.Icon : model.IconName
>  textFormat: Text.AutoText

You need to change here as well.

> StatusNotifierItem.qml:61
> +PlasmaCore.IconItem {
> +id: overlayIconItem
> +source: {

add `parent: iconItem.parent`

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D28208

To: davidre, kmaterka, broulik, mart, #plasma, #vdg
Cc: bruns, ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28259: [applets/systemtray] Default value for ItemLoader.source

[Konrad Materka]

This revision was automatically updated to reflect the committed changes.
Closed by commit R120:5b99315f75f9: [applets/systemtray] Default value for 
ItemLoader.source (authored by kmaterka).

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28259?vs=78406=78408

REVISION DETAIL
  https://phabricator.kde.org/D28259

AFFECTED FILES
  applets/systemtray/package/contents/ui/items/ItemLoader.qml

To: kmaterka, #plasma_workspaces, #plasma, ngraham
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28208: Move sni icon handling logic from data engine to applet

[Konrad Materka]

kmaterka added a comment.


  Two problems:
  
  After running `plasmashell --replace &` I have these messages in the console 
output:
  
  > 
file:///home/konrad/kde/usr/share/plasma/plasmoids/org.kde.plasma.private.systemtray/contents/ui/items/StatusNotifierItem.qml:57:5:
 QML IconItem: Cannot anchor to an item that isn't a parent or sibling.
  >  
file:///home/konrad/kde/usr/share/plasma/plasmoids/org.kde.plasma.private.systemtray/contents/ui/items/StatusNotifierItem.qml:57:5:
 QML IconItem: Cannot anchor to an item that isn't a parent or sibling.
  
  You need to rebase, in latest change parent of the icon changed to `parent: 
taskIcon.iconContainer`, overlay icon should have the same parent:
  
id: overlayIconItem
parent: iconItem.parent
  
  Some of the icons are not rendering now, for example:
  Discord - iconName: chrome_app_indicator2_83406baa15c6e4f2766a6e3714fbee20
  Skype1 - iconName: chrome_app_indicator2_01cdeaaa54803d9c9c158a5cdcb1fbfb
  I'm pretty sure I saw a workaround for that somewhere, but I can't recall 
where...

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D28208

To: davidre, kmaterka, broulik, mart, #plasma, #vdg
Cc: bruns, ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28259: [applets/systemtray] Default value for ItemLoader.source

[Konrad Materka]

kmaterka created this revision.
kmaterka added reviewers: Plasma: Workspaces, Plasma.
Herald added a project: Plasma.
kmaterka requested review of this revision.

REVISION SUMMARY
  When none of the valid conditions are met, function should return default 
value for ItemLoader source. If not, error is printed in logs.

TEST PLAN
  No warning about undefined value for url

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D28259

AFFECTED FILES
  applets/systemtray/package/contents/ui/items/ItemLoader.qml

To: kmaterka, #plasma_workspaces, #plasma
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28109: [applets/systemtray] Simplify icon size logic

[Konrad Materka]

This revision was automatically updated to reflect the committed changes.
Closed by commit R120:a9d941b38e05: [applets/systemtray] Simplify icon size 
logic (authored by kmaterka).

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28109?vs=78185=78386

REVISION DETAIL
  https://phabricator.kde.org/D28109

AFFECTED FILES
  applets/systemtray/package/contents/ui/ExpanderArrow.qml
  applets/systemtray/package/contents/ui/HiddenItemsView.qml
  applets/systemtray/package/contents/ui/items/AbstractItem.qml
  applets/systemtray/package/contents/ui/items/PlasmoidItem.qml
  applets/systemtray/package/contents/ui/items/StatusNotifierItem.qml
  applets/systemtray/package/contents/ui/main.qml

To: kmaterka, #plasma_workspaces, #plasma, ngraham, broulik, davidedmundson, 
apol
Cc: mart, davidedmundson, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra

D28208: Move sni icon handling logic from data engine to applet

[Konrad Materka]

kmaterka added inline comments.

INLINE COMMENTS

> davidre wrote in StatusNotifierItem.qml:90
> Hmm to me it looks it doesn't even have an margin. Or am I reading   ` 
> p.drawPixmap(QRect(m_iconPixmap.width()-size, m_iconPixmap.height()-size, 
> size, size), overlay->pixmap(size, size), QRect(0,0,size,size));` wrong?

Yes, you are correct, my mistake. So `StatusNotifierItemSource` has no margin 
and `KIconLoader` has 5% of icon size - another inconsistency.
What about 'always round when multiplying by a non-integer value'? I don't know 
exactly why, but AFAIK not-rounded number can cause inconsistent behavior in 
QML.
@ngraham can you give a word of explanation?

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D28208

To: davidre, kmaterka, broulik, mart, #plasma, #vdg
Cc: ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28208: Move sni icon handling logic from data engine to applet

[Konrad Materka]

kmaterka added inline comments.

INLINE COMMENTS

> StatusNotifierItem.qml:58
>  }
> +// IconItem.overlays only supports names so we need a second item for 
> the overlay, using the same
> +// positioning that KIconLoader::drawOverlays uses that IconItem uses 
> internally

Should overlay pixmaps be supported in `PlasmaCore.IconItem`? I guess that 
would require changes in `KIconLoader` as well.

> StatusNotifierItem.qml:72
> +width: {
> +if (iconItem.width < 32) {
> +return 8;

In StatusNotifierItemSource::overlayIcon() 

 it is "width <= 32". There is a pre-existing inconsistency between 
StatusNotifierItemSource and IconItem/KIconLoader.

> davidre wrote in StatusNotifierItem.qml:84
> Good to know! That was a  straight copy from 
> https://cgit.kde.org/kiconthemes.git/tree/src/kiconloader.cpp#n478 :D
> I'm sure we can find better sizing though, I think this is to small, for sure.

StatusNotifierItemSource::overlayIcon() 

 also has some sizes hard coded. I quickly checked, looks like the values are 
the same.
You can use the same syntax here:

  if (iconItem.width <= units.iconSize.medium) {
  return units.iconSize.small / 2;
  }
  if (iconItem.width <= units.iconSize.large) {
  return units.iconSize.small;
  }

> ngraham wrote in StatusNotifierItem.qml:90
> always round when multiplying by a non-integer value

In StatusNotifierItemSource::overlayIcon() 

 it has a different logic to position overlay. Margin is always the size of 
overlay icon. Another pre-existing inconsistency. Which one to choose? I think 
that `KIconLoader` is more important.

> systemtraymodel.h:97
>  IconThemePath,
>  IconsChanged,
>  Id,

You can safely remove all *Changed as well. Or maybe better in a separate 
commit?

> statusnotifieritemsource.cpp:90
>  //by setting everything up-front so that we have all role names when we 
> call the first checkForUpdate()
> +// TODO Plasma 6 remove combined "*Icon" properties and only expose the 
> raw "*IconName" and "*IconPixmap" properties
>  setData(QStringLiteral("AttentionIcon"), QIcon());

I would suggest to extend removal to all *Changed roles. These are not used 
(were in KDE/Plasma 4).

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D28208

To: davidre, kmaterka, broulik, mart, #plasma, #vdg
Cc: ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28180: [applets/systemtray] Hide/show expander arrow - regression fix

[Konrad Materka]

kmaterka added a comment.


  In D28180#632058 , @ngraham wrote:
  
  > This works, but I wonder if there's a conceptually cleaner way to do it. 
Not that I know of one, so I'll wait for a #plasma 
 person to comment.
  
  
  Yes, this code looks smelly, but I don't have better idea now.
  
  Anyone can take a look and help with this?

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D28180

To: kmaterka, #plasma_workspaces, #plasma, ngraham, broulik, davidedmundson
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28183: [kioslave/desktop] Attempt at making test more robust

[Konrad Materka]

kmaterka accepted this revision.
kmaterka added a comment.
This revision is now accepted and ready to land.


  In D28183#632057 , @davidedmundson 
wrote:
  
  > That won't work as is because I would need to re-run
  >
  >   const KFileItem fileItem = lister->findByUrl(destUrl);
  
  
  I knew there had to be a catch, it would be too easy...
  
  Probably the best would be to make KDirNotify consistent, but probably that's 
not that easy. OK, one small wait won't kill the world, right? :)

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D28183

To: davidedmundson, #plasma, kmaterka
Cc: kmaterka, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28109: [applets/systemtray] Simplify icon size logic

[Konrad Materka]

kmaterka updated this revision to Diff 78185.
kmaterka added a comment.


  Rebase

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28109?vs=78184=78185

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D28109

AFFECTED FILES
  applets/systemtray/package/contents/ui/ExpanderArrow.qml
  applets/systemtray/package/contents/ui/HiddenItemsView.qml
  applets/systemtray/package/contents/ui/items/AbstractItem.qml
  applets/systemtray/package/contents/ui/items/PlasmoidItem.qml
  applets/systemtray/package/contents/ui/items/StatusNotifierItem.qml
  applets/systemtray/package/contents/ui/main.qml

To: kmaterka, #plasma_workspaces, #plasma, ngraham, broulik
Cc: mart, davidedmundson, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra

D28109: [applets/systemtray] Simplify icon size logic

[Konrad Materka]

kmaterka updated this revision to Diff 78184.
kmaterka added a comment.


  Rebase

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28109?vs=77865=78184

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D28109

AFFECTED FILES
  applets/systemtray/package/contents/ui/ExpanderArrow.qml
  applets/systemtray/package/contents/ui/HiddenItemsView.qml
  applets/systemtray/package/contents/ui/items/AbstractItem.qml
  applets/systemtray/package/contents/ui/items/PlasmoidItem.qml
  applets/systemtray/package/contents/ui/items/StatusNotifierItem.qml
  applets/systemtray/package/contents/ui/main.qml

To: kmaterka, #plasma_workspaces, #plasma, ngraham, broulik
Cc: mart, davidedmundson, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra

D28185: [applet/systemtray] Use model binding instead of copy

[Konrad Materka]

This revision was automatically updated to reflect the committed changes.
Closed by commit R120:17153c4ae204: [applet/systemtray] Use model binding 
instead of copy (authored by kmaterka).

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28185?vs=78181=78183

REVISION DETAIL
  https://phabricator.kde.org/D28185

AFFECTED FILES
  applets/systemtray/package/contents/ui/items/AbstractItem.qml
  applets/systemtray/package/contents/ui/items/ItemLoader.qml
  applets/systemtray/package/contents/ui/items/PlasmoidItem.qml
  applets/systemtray/package/contents/ui/items/StatusNotifierItem.qml

To: kmaterka, #plasma_workspaces, #plasma, ngraham, broulik, davidedmundson, 
apol
Cc: apol, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D28185: [applet/systemtray] Use model binding instead of copy

[Konrad Materka]

kmaterka marked an inline comment as done.
kmaterka added inline comments.

INLINE COMMENTS

> broulik wrote in AbstractItem.qml:36
> You can simplify this to
> 
>   model.status || PlasmaCore.Types.UnknownStatus

Done

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D28185

To: kmaterka, #plasma_workspaces, #plasma, ngraham, broulik, davidedmundson, 
apol
Cc: apol, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D28185: [applet/systemtray] Use model binding instead of copy

[Konrad Materka]

kmaterka updated this revision to Diff 78181.
kmaterka added a comment.


  Simplify JS

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28185?vs=78178=78181

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D28185

AFFECTED FILES
  applets/systemtray/package/contents/ui/items/AbstractItem.qml
  applets/systemtray/package/contents/ui/items/ItemLoader.qml
  applets/systemtray/package/contents/ui/items/PlasmoidItem.qml
  applets/systemtray/package/contents/ui/items/StatusNotifierItem.qml

To: kmaterka, #plasma_workspaces, #plasma, ngraham, broulik, davidedmundson, 
apol
Cc: apol, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D28185: [applet/systemtray] Us model binding instead of copy

[Konrad Materka]

kmaterka added a comment.


  In D28185#631972 , @apol wrote:
  
  > I'm not convinced this is all that better.
  
  
  Without this `effectiveStatus` never changes. Right now it works by happy 
coincidence - Item is simple destroyed and created in a different, as a side 
effect new value of `effectiveStatus` is used.
  This change tries to fix my mistake introduced in D26992 
. I used QML Loader incorrectly... :(
  Anyway, I changed it a little bin, please check now.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D28185

To: kmaterka, #plasma_workspaces, #plasma, ngraham, broulik, davidedmundson
Cc: apol, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D28185: [applet/systemtray] Us model binding instead of copy

[Konrad Materka]

kmaterka updated this revision to Diff 78178.
kmaterka added a comment.


  Bind model as well

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28185?vs=78169=78178

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D28185

AFFECTED FILES
  applets/systemtray/package/contents/ui/items/AbstractItem.qml
  applets/systemtray/package/contents/ui/items/ItemLoader.qml
  applets/systemtray/package/contents/ui/items/PlasmoidItem.qml
  applets/systemtray/package/contents/ui/items/StatusNotifierItem.qml

To: kmaterka, #plasma_workspaces, #plasma, ngraham, broulik, davidedmundson
Cc: apol, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D28183: [kioslave/desktop] Attempt at making test more robust

[Konrad Materka]

kmaterka added a comment.


  In D28183#631966 , @davidedmundson 
wrote:
  
  > The failure was always on this line:
  >
  > >   QCOMPARE(fileItem.targetUrl(), QUrl::fromLocalFile(destFilePath));
  
  
  Oh, I see. So maybe this:
  `QTRY_COMPARE_WITH_TIMEOUT(fileItem.targetUrl(), 
QUrl::fromLocalFile(destFilePath), 250)`
  ?

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D28183

To: davidedmundson, #plasma
Cc: kmaterka, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28183: [kioslave/desktop] Attempt at making test more robust

[Konrad Materka]

kmaterka added a comment.


  In theory `QTRY_VERIFY` should wait up to 5 seconds until condition is met. 
`qWait(250)` shouldn't change much...

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D28183

To: davidedmundson, #plasma
Cc: kmaterka, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28185: [applet/systemtray] Us model binding instead of copy

[Konrad Materka]

kmaterka created this revision.
kmaterka added reviewers: Plasma: Workspaces, Plasma, ngraham, broulik, 
davidedmundson.
kmaterka added a project: Plasma.
kmaterka requested review of this revision.

REVISION SUMMARY
  The `effectiveStatus` parameter was passed as a copy, use proper binding.

TEST PLAN
  No visible impact

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D28185

AFFECTED FILES
  applets/systemtray/package/contents/ui/items/AbstractItem.qml
  applets/systemtray/package/contents/ui/items/ItemLoader.qml
  applets/systemtray/package/contents/ui/items/PlasmoidItem.qml
  applets/systemtray/package/contents/ui/items/StatusNotifierItem.qml

To: kmaterka, #plasma_workspaces, #plasma, ngraham, broulik, davidedmundson
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28107: Fix overlayIcon sometimes not visible

[Konrad Materka]

kmaterka added a comment.


  In D28107#630144 , @davidre wrote:
  
  > It seems it is possible to do this (removing stuff from the data engine) 
after all. I will try to work on this in the next time
  
  
  IMO, ideally `StatusNotifierItemSource` should just expose most of properties 
from this specification without any changes:
  
https://www.freedesktop.org/wiki/Specifications/StatusNotifierItem/StatusNotifierItem/
  For some complex types it should be allowed to do conversion etc.
  
  Summary of properties:
  
  | SNI property| In specification | Simple/Complex | DataSource 
property| Comments  
|
  | Category| Yes  | Simple | Category  
 | Direct copy  
 |
  | Id  | Yes  | Simple | Id
 | Direct copy  
 |
  | Title   | Yes  | Simple | Title 
 | Direct copy  
 |
  | Status  | Yes  | Simple | Status
 | Direct copy  
 |
  | WindowId| Yes  | Simple | WindowId  
 | Direct copy  
 |
  | ItemIsMenu  | Yes  | Simple | ItemIsMenu
 | Direct copy  
 |
  | AttentionMovieName  | Yes  | Simple | 
AttentionMovieName | Direct copy
   |
  | ToolTip | Yes  | Complex| ToolTipTitle, 
ToolTipSubTitle, ToolTipIcon | Converted to separate properties, additional 
logic|
  | Menu| Yes  | Complex| - 
 | Converted to QMenu, special handling 
 |
  | IconThemePath   | No!  | Simple | IconThemePath 
 | Direct copy  
 |
  | IconName| Yes  | Simple | IconName  
 | Only if IconPixmap is empty  
 |
  | IconPixmap  | Yes  | Complex| Not available 
 | Used as part of Icon 
 |
  | OverlayIconName | Yes  | Simple | OverlayIconName   
 | Only if OverlayIconPixmap is empty   
 |
  | OverlayIconPixmap   | Yes  | Complex| Not available 
 | Used as part of Icon 
 |
  | -   | -| Simple | Icon  
 | Complicated logic to create it from all Icon 
properties   |
  | AttentionIconName   | Yes  | Simple | AttentionIconName 
 | Only if AttentionIconPixmap is empty 
 |
  | AttentionIconPixmap | Yes  | Not available  | Used as part of 
AttentionIcon  |
   |
  | -   | -| Simple | AttentionIcon 
 | Complicated logic to create it from both 
AttentionIcon properties |
  |
  
  I skipped: IconsChanged, StatusChanged, TitleChanged, ToolTipChanged, these 
are not relevant (and not used anymore).
  
  I would suggest to:
  
  - In `StatusNotifierItemSource`:
- Always set IconName, OverlayIconName, AttentionIconName, regardless if 
IconPixmap is set or not
- Add new properties for all pixmaps, convert them to proper type (there is 
function for that: `imageVectorToPixmap()`)
  - Icon logic should be in `StatusNotifierItem.qml`
- Ignore "Icon" property
- Implement similar logic in QML, everything is available in 
`PlasmaCore.IconItem`
- Use icon name first, then pixmap (align to specification: "Visualizations 
are encouraged to prefer icon names over icon pixmaps if both are available")
  
  Then, is a separate diff, remove unused properties/logic from 
StatusNotifierItemSource (in Plasma 6?).
  
  This ensures backward compatibility. I'm pretty sure 
`StatusNotifierItemSource` is not used outside of Plasma, but if it this is 
part of the public API then needs to stay for now.
  Presentation logic is 

D28180: [applets/systemtray] Hide/show expander arrow - regression fix

[Konrad Materka]

kmaterka added inline comments.

INLINE COMMENTS

> ExpanderArrow.qml:31
>  implicitHeight: implicitWidth
> -visible: root.hiddenLayout.contentItem.children.length > 0
> +visible: root.hiddenLayout.itemCount > 0
>  

ideally `root.hiddenLayout.count` should be used, but `count` is not updated 
until ListView is rendered.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D28180

To: kmaterka, #plasma_workspaces, #plasma, ngraham, broulik, davidedmundson
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28180: [applets/systemtray] Hide/show expander arrow - regression fix

[Konrad Materka]

kmaterka created this revision.
kmaterka added reviewers: Plasma: Workspaces, Plasma, ngraham, broulik, 
davidedmundson.
kmaterka added a project: Plasma.
Herald added a subscriber: plasma-devel.
kmaterka requested review of this revision.

REVISION SUMMARY
  Expander arrow should hide when all icons are visible and no icon is hidden. 
When dialog with hidden items is hidden ListView does not update its count 
property - property is update on redraw. As a result expander arrow is still 
visible/hidden until user clicks on any plasmoid to show dialog.

TEST PLAN
  - select/deselect "Show all items" in settings, save, expander arrow should 
hide/show imidiatelly
  - select almost all items to "Always shown", leave "Keyboard indicator" as 
"Shown when relevant". Press Casp Lock, expander arrow should hide/show 
correctly

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D28180

AFFECTED FILES
  applets/systemtray/package/contents/ui/ExpanderArrow.qml
  applets/systemtray/package/contents/ui/HiddenItemsView.qml

To: kmaterka, #plasma_workspaces, #plasma, ngraham, broulik, davidedmundson
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28109: [applets/systemtray] Simplify icon size logic

[Konrad Materka]

kmaterka added a comment.


  In D28109#629974 , @ngraham wrote:
  
  > This looks like a sensible refactor to me, and it solves the issue that I 
was having with icon size changing based on the expanded popup visibility. I 
can't detect any regressions in sizing with horizontal or vertical panels of 
various sizes. However please wait to land this until somebody else possessing 
greater familiarity with this codebase such as @broulik, @mart, or 
@davidedmundson reviews it too.
  
  
  @ngraham the problem is when diff is marked as reviewed, they don't see it :)

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D28109

To: kmaterka, #plasma_workspaces, #plasma, ngraham, broulik
Cc: mart, davidedmundson, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra

D28107: Fix overlayIcon sometimes not visible

[Konrad Materka]

kmaterka requested changes to this revision.
kmaterka added a comment.
This revision now requires changes to proceed.


  Yep, icons in SNI are a mess...
  Icons are processed in `StatusNotifierItemSource`, including overlay - that's 
were this should be fixed. From you comment I see it won't be easy...
  This is a workaround, it works, but makes the code even more messy.
  
  So... if you are brave enough, you can rewrite icon handling in SNI. Idea is 
to:
  
  - remove custom icon handling code from `StatusNotifierItemSource`
  - pass data directly to QML
  - move icon display logic to QML
  
  What do you think?

INLINE COMMENTS

> systemtraymodel.cpp:321
> + */
> +if(!icon.value().name().isEmpty()) {
> +updateItemData(dataItem, data, Role::OverlayIconName);

code formatting (missing space after if)

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D28107

To: davidre, kmaterka, #plasma, broulik, davidedmundson
Cc: anthonyfieroni, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28096: Don't overwrite status

[Konrad Materka]

kmaterka added a comment.


  > Oh you're right. It was not wokring for me on 5.18 so I changed this and it 
worked. But matter of fact it worked on master all allong :)
  
  5.18 is not using this data model, in 5.18 it is used only in configuration 
page. In 5.19 this model is everywhere.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D28096

To: davidre, broulik, kmaterka, #plasma, davidedmundson
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28096: Don't overwrite status

[Konrad Materka]

kmaterka requested changes to this revision.
kmaterka added a comment.
This revision now requires changes to proceed.


  > The qml checks the value against the enum but updateItemData inserts the 
string.
  
  These are two different roles:
  "status" with enum value
  "Status" from SNI data
  
  The same happens for "category" role.
  
  And yeah, I know this is confusing... The best would be to remove all unused 
roles, especially all *Changed. I just copied all from 
`StatusNotifierItemSource`, some of them are deprecated from Plasma 4 times.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D28096

To: davidre, broulik, kmaterka, #plasma, davidedmundson
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28109: [applets/systemtray] Simplify icon size logic

[Konrad Materka]

kmaterka added inline comments.

INLINE COMMENTS

> ExpanderArrow.qml:31
>  implicitHeight: implicitWidth
> -visible: root.hiddenLayout.contentItem.children.length > 0
> +visible: root.hiddenLayout.count > 0
>  

This fixes issue (regression) with expander arrow not hiding when all items are 
visible. Probably I should create separate commit...

> ItemLoader.qml:28
>  itemLoader.setSource("PlasmoidItem.qml", {
> - "applet": model.applet,
> - "effectiveStatus": model.effectiveStatus
> + "model": model
>   })

The `effectiveStatus` was copied from the model, not binded. Separate commit?

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D28109

To: kmaterka, #plasma_workspaces, #plasma, ngraham, broulik
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28109: [applets/systemtray] Simplify icon size logic

[Konrad Materka]

kmaterka created this revision.
kmaterka added reviewers: Plasma: Workspaces, Plasma, ngraham, broulik.
kmaterka added a project: Plasma.
kmaterka requested review of this revision.

REVISION SUMMARY
  This change simplifies icon size logic - now it is controled only in one 
place: AbstractItem. This gives additional benefits, like icons perfectly 
centered.

TEST PLAN
  - Highlights should always be centered

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D28109

AFFECTED FILES
  applets/systemtray/package/contents/ui/ExpanderArrow.qml
  applets/systemtray/package/contents/ui/HiddenItemsView.qml
  applets/systemtray/package/contents/ui/items/AbstractItem.qml
  applets/systemtray/package/contents/ui/items/ItemLoader.qml
  applets/systemtray/package/contents/ui/items/PlasmoidItem.qml
  applets/systemtray/package/contents/ui/items/StatusNotifierItem.qml
  applets/systemtray/package/contents/ui/main.qml

To: kmaterka, #plasma_workspaces, #plasma, ngraham, broulik
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27160: [applets/mediacontroller] Visually refresh media controller plasmoid

[Konrad Materka]

kmaterka added inline comments.

INLINE COMMENTS

> cblack wrote in ExpandedRepresentation.qml:261
> This advice doesn't seem to be applicable here, as the `implicitWidth` of the 
> ColumnLayout child isn't able to be mutated due to the fact that all of its 
> child items (text) have fixed `implicitWidth`s.

True, I didn't want to copy whole answer from Stack Overflow :) In this case 
`Layout.preferredWidth` should not be set to `parent.width / 2`, better to use 
`Layout.preferredWidth = 50`. I sometimes add % in a comment: 
`Layout.preferredWidth = 50//%`.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D27160

To: cblack, #vdg, #plasma, manueljlin, ngraham
Cc: kmaterka, iasensio, ndavis, broulik, gvgeo, davidedmundson, ngraham, 
manueljlin, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27958: [SystemTray] Fix item size for very narrow panel

[Konrad Materka]

kmaterka added a comment.


  In D27958#627658 , @ngraham wrote:
  
  > After a few days of living with it, I've found that this patch causes the 
following issue with semi-random tray items getting smaller when a pop-up is 
opened:
  >
  > F8176616: vokoscreenNG-2020-03-14_23-35-16.webm 

  >
  > Could you investigate?
  
  
  Yes, I will take a look at this.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D27958

To: kmaterka, #plasma_workspaces, #plasma, ngraham, broulik
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27160: [applets/mediacontroller] Visually refresh media controller plasmoid

[Konrad Materka]

kmaterka added inline comments.

INLINE COMMENTS

> kmaterka wrote in ExpandedRepresentation.qml:261
> My last comment was not precise. Just check this 
>  for all details :)

As a general rule, you shouldn't refer to parents width in Layouts, including: 
`Layout.maximumWidth: parent.width`. It a (very) common mistake :)

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D27160

To: cblack, #vdg, #plasma, manueljlin, ngraham
Cc: kmaterka, iasensio, ndavis, broulik, gvgeo, davidedmundson, ngraham, 
manueljlin, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27160: [applets/mediacontroller] Visually refresh media controller plasmoid

[Konrad Materka]

kmaterka added inline comments.

INLINE COMMENTS

> kmaterka wrote in ExpandedRepresentation.qml:261
> I have not seen your code yet, I will check tomorrow. Probably because you 
> have something like this:
> 
>   Layout.preferredWidth: parent.width / 2
>   Layout.preferredHeight: parent.height / 2
> 
> Check the last answer from: How to design a multi-level fluid layout in QML 
> .
>  In short words: when possible, do not use Layout.preferredWidth, prefer 
> implicitWidth instead. If you want to have 50-50 ratio between items, set 
> implicitWidth in both items to the same value.

My last comment was not precise. Just check this 
 for all details :)

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D27160

To: cblack, #vdg, #plasma, manueljlin, ngraham
Cc: kmaterka, iasensio, ndavis, broulik, gvgeo, davidedmundson, ngraham, 
manueljlin, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27160: [applets/mediacontroller] Visually refresh media controller plasmoid

[Konrad Materka]

kmaterka added inline comments.

INLINE COMMENTS

> cblack wrote in ExpandedRepresentation.qml:261
> I can't seem to reproduce a binding loop here.

I have not seen your code yet, I will check tomorrow. Probably because you have 
something like this:

  Layout.preferredWidth: parent.width / 2
  Layout.preferredHeight: parent.height / 2

Check the last answer from: How to design a multi-level fluid layout in QML 
.
 In short words: when possible, do not use Layout.preferredWidth, prefer 
implicitWidth instead. If you want to have 50-50 ratio between items, set 
implicitWidth in both items to the same value.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D27160

To: cblack, #vdg, #plasma, manueljlin, ngraham
Cc: kmaterka, iasensio, ndavis, broulik, gvgeo, davidedmundson, ngraham, 
manueljlin, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27958: [SystemTray] Fix item size for very narrow panel

[Konrad Materka]

This revision was automatically updated to reflect the committed changes.
Closed by commit R120:85f646eb5649: [SystemTray] Fix item size for very narrow 
panel (authored by kmaterka).

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27958?vs=77321=77370

REVISION DETAIL
  https://phabricator.kde.org/D27958

AFFECTED FILES
  applets/systemtray/package/contents/ui/items/PlasmoidItem.qml
  applets/systemtray/package/contents/ui/main.qml

To: kmaterka, #plasma_workspaces, #plasma, ngraham, broulik
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27958: [SystemTray] Fix item size for very narrow panel

[Konrad Materka]

kmaterka added a comment.


  In D27958#625431 , @broulik wrote:
  
  > Not sure if it is just me but the vertical alignment looks slightly off
  >  F8169131: Screenshot_20200310_172654.PNG 

  >  Or maybe that's the random different sizes
  
  
  This is just an illusion. I checked and literally counted pixels, centers of 
icons are aligned (both horizonaly and vertically). "de" icon is visually 
"heavier" at the bottom, the arrow on the right is heavier on top which makes 
impresion that these two are not aligned.

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D27958

To: kmaterka, #plasma_workspaces, #plasma, ngraham, broulik
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27466: Increase the size of system tray icon hitboxes on the System Tray Plasmoid

[Konrad Materka]

kmaterka added a comment.


  Fix for narrow panel in D27958 

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D27466

To: The-Feren-OS-Dev, #vdg, niccolove, ngraham
Cc: broulik, kmaterka, gvgeo, ngraham, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, 
ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, ahiemstra, mart

D27958: [SystemTray] Fix item size for very narrow panel

[Konrad Materka]

kmaterka added a comment.


  I don't like how icon size and item size (container = icon + padding) are 
calculated and used... Firstly best icons size is calculated, but then padding 
are added. This combined size is used as item size (container size):
  
  - SNI icons are rounded to nearest icon size (implicitly, because  
PlasmaCore.IconItem is used)
  - Plasmoids are scaled to the size of the container, I don't know exactly how 
it work, probably because default compact representation is just an icon.
  
  By happy coincident it is (?) working correctly for all sizes.
  
  There is also a lot of anchoring involved, all items are aligned to left 
(which looks bad with expander arrow, gap is bigger) etc. But all of this can 
be fixed in separate patch.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D27958

To: kmaterka, #plasma_workspaces, #plasma, ngraham, broulik
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27958: [SystemTray] Fix item size for very narrow panel

[Konrad Materka]

kmaterka created this revision.
kmaterka added reviewers: Plasma: Workspaces, Plasma, ngraham, broulik.
kmaterka added a project: Plasma.
kmaterka requested review of this revision.

REVISION SUMMARY
  For very narrow panel item size is too big (item + paddings exceed panel 
size). This adds a check to not exceed poanel size.

TEST PLAN
  - Make panel very narrow - icons should fit into panel
  - Repeat for vertical/horizontal

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D27958

AFFECTED FILES
  applets/systemtray/package/contents/ui/items/PlasmoidItem.qml
  applets/systemtray/package/contents/ui/main.qml

To: kmaterka, #plasma_workspaces, #plasma, ngraham, broulik
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27466: Increase the size of system tray icon hitboxes on the System Tray Plasmoid

[Konrad Materka]

kmaterka added a comment.


  In D27466#624574 , @broulik wrote:
  
  > > I believe that D26992 , moves the 
icons down.
  >
  > It looks like with that patch the icon sizes differ between plasmoids and 
SNIs
  
  
  That was my work, definitely that should not be the outcome. :) I will check 
it.
  
  The main problem here is that itemSize (regardless if it is plasmoid or SNI) 
is calculated in a very complicated and tricky way. If I remember correctly, 
firstly icon size is calculated, then margin is added but it is not a real 
margin - simply SNI icons are rounded to closes icon size and then by happy 
coincident we get margin. I guess it needs to be reworked... I can do that, 
just need few days for investigation.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D27466

To: The-Feren-OS-Dev, #vdg, niccolove, ngraham
Cc: broulik, kmaterka, gvgeo, ngraham, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, 
ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, ahiemstra, mart

D27466: Increase the size of system tray icon hitboxes on the System Tray Plasmoid

[Konrad Materka]

kmaterka added a comment.


  In D27466#624447 , @broulik wrote:
  
  > This causes layouting issues in narrow panels:
  >  F8167100: Screenshot_20200309_085328.PNG 

  >  The icons are also too far apart
  
  
  For small icons this does not look nice. BaseSize is increased by the same 
`units.smallSpacing /2` regardless of the icon size, which is a great 
proportion for small icons.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D27466

To: The-Feren-OS-Dev, #vdg, niccolove, ngraham
Cc: broulik, kmaterka, gvgeo, ngraham, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, 
ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, ahiemstra, mart

D26992: [SystemTray] Use unified data model everywhere

[Konrad Materka]

This revision was automatically updated to reflect the committed changes.
kmaterka marked an inline comment as done.
Closed by commit R120:39975673cb6a: [SystemTray] Use unified data model 
everywhere (authored by kmaterka).

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26992?vs=76028=77156

REVISION DETAIL
  https://phabricator.kde.org/D26992

AFFECTED FILES
  applets/systemtray/package/contents/ui/ExpanderArrow.qml
  applets/systemtray/package/contents/ui/HiddenItemsView.qml
  applets/systemtray/package/contents/ui/items/AbstractItem.qml
  applets/systemtray/package/contents/ui/items/ItemLoader.qml
  applets/systemtray/package/contents/ui/items/PlasmoidItem.qml
  applets/systemtray/package/contents/ui/items/StatusNotifierItem.qml
  applets/systemtray/package/contents/ui/main.qml
  applets/systemtray/sortedsystemtraymodel.cpp
  applets/systemtray/sortedsystemtraymodel.h
  applets/systemtray/systemtray.cpp
  applets/systemtray/systemtray.h
  applets/systemtray/systemtraymodel.cpp
  applets/systemtray/systemtraymodel.h

To: kmaterka, #plasma_workspaces, #plasma, davidedmundson, ngraham, broulik
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26992: [SystemTray] Use unified data model everywhere

[Konrad Materka]

kmaterka marked an inline comment as done.
kmaterka added inline comments.

INLINE COMMENTS

> davidedmundson wrote in AbstractItem.qml:34
> Lets look at fixing in a follow up patch
> 
> QML can do enums now

OK, I will change that in a separate patch

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D26992

To: kmaterka, #plasma_workspaces, #plasma, davidedmundson, ngraham, broulik
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

Re: Plasma Sprint 2020 in Augsburg, Germany

[Konrad Materka]

Hi,

What is the chance of this event being cancelled due to coronavirus? I have
to book tickets on advance, unfortunately flight is the most reasonable.

-- 
Regards,
Konrad Materka

D27845: Replace Task Manager with Icons-Only-Task Manager in the default panel, and thicken it

[Konrad Materka]

kmaterka added a comment.


  Should System Tray icons be larger then?

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D27845

To: ngraham, #vdg, #plasma
Cc: kmaterka, gvgeo, davidedmundson, ndavis, KonqiDragon, niccolove, 
The-Feren-OS-Dev, plasma-devel, Orage, LeGast00n, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27466: Increase the size of system tray icon hitboxes on the System Tray Plasmoid

[Konrad Materka]

kmaterka added a comment.


  In D27466#620470 , @gvgeo wrote:
  
  > IMO this is the wrong way to do these changes.
  >
  > 1. I don't see "itemSize" to be used anywhere else(didn't check) than 
"tasksRow" where we already add a smallSpacing. It would be best to increase 
the size in one place.
  > 2. For tablets already allow to use  bigger size icons. There is no need to 
artificially increase the size this way, if they need bigger size can increase 
the panel height.
  > 3. Can expose plasmoid.configuration.iconSize to the user. Otherwise can be 
removed.
  
  
  SystemTray is quite messy in a lot of places. How conveniently, there is a 
change with some refactoring and cleanup (D26992 
) waiting for a review :)

BRANCH
  D27466 (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D27466

To: The-Feren-OS-Dev, #vdg, niccolove, ngraham
Cc: kmaterka, gvgeo, ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26992: [SystemTray] Use unified data model everywhere

[Konrad Materka]

kmaterka added a comment.


  In D26992#614426 , @ngraham wrote:
  
  > UI looks good except for one thing: the changes to show a highlight effect 
on hover in the popup are unrelated and should be done in a separate patch 
(preferably in coordination with #VDG  
folks since we're moving towards the idea of using this in other places too).
  
  
  This change should not introduce any visual change but one: add transition 
(main.qml, line 170).
  I guess you are referring to HiddenItemsView.qml, are you? I had to change 
Column to ListView to support model so almost everything changed or moved. 
CurrentItemHighLight and PlasmaComponents.HighLight was there from the very 
beginning.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D26992

To: kmaterka, #plasma_workspaces, #plasma, davidedmundson, ngraham, broulik
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26992: [SystemTray] Use unified data model everywhere

[Konrad Materka]

kmaterka updated this revision to Diff 76028.
kmaterka added a comment.


  Second (and last) change extracted from this one is in master. Rebased, ready 
for review.

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26992?vs=75134=76028

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D26992

AFFECTED FILES
  applets/systemtray/package/contents/ui/ExpanderArrow.qml
  applets/systemtray/package/contents/ui/HiddenItemsView.qml
  applets/systemtray/package/contents/ui/items/AbstractItem.qml
  applets/systemtray/package/contents/ui/items/ItemLoader.qml
  applets/systemtray/package/contents/ui/items/PlasmoidItem.qml
  applets/systemtray/package/contents/ui/items/StatusNotifierItem.qml
  applets/systemtray/package/contents/ui/main.qml
  applets/systemtray/sortedsystemtraymodel.cpp
  applets/systemtray/sortedsystemtraymodel.h
  applets/systemtray/systemtray.cpp
  applets/systemtray/systemtray.h
  applets/systemtray/systemtraymodel.cpp
  applets/systemtray/systemtraymodel.h

To: kmaterka, #plasma_workspaces, #plasma, davidedmundson, ngraham, broulik
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27088: [applets/SystemTray] Implement sorting in the model

[Konrad Materka]

This revision was automatically updated to reflect the committed changes.
Closed by commit R120:6d86c690d133: [applets/SystemTray] Implement sorting in 
the model (authored by kmaterka).

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27088?vs=76026=76027

REVISION DETAIL
  https://phabricator.kde.org/D27088

AFFECTED FILES
  applets/systemtray/CMakeLists.txt
  applets/systemtray/package/contents/ui/ConfigEntries.qml
  applets/systemtray/sortedsystemtraymodel.cpp
  applets/systemtray/sortedsystemtraymodel.h
  applets/systemtray/systemtray.cpp
  applets/systemtray/systemtray.h
  applets/systemtray/systemtraymodel.cpp
  applets/systemtray/systemtraymodel.h

To: kmaterka, #plasma_workspaces, #plasma, davidedmundson, ngraham, broulik
Cc: mart, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra

D27088: [applets/SystemTray] Implement sorting in the model

[Konrad Materka]

kmaterka updated this revision to Diff 76026.
kmaterka added a comment.


  Review fixes

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27088?vs=75112=76026

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D27088

AFFECTED FILES
  applets/systemtray/CMakeLists.txt
  applets/systemtray/package/contents/ui/ConfigEntries.qml
  applets/systemtray/sortedsystemtraymodel.cpp
  applets/systemtray/sortedsystemtraymodel.h
  applets/systemtray/systemtray.cpp
  applets/systemtray/systemtray.h
  applets/systemtray/systemtraymodel.cpp
  applets/systemtray/systemtraymodel.h

To: kmaterka, #plasma_workspaces, #plasma, davidedmundson, ngraham, broulik
Cc: mart, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra

D27088: [applets/SystemTray] Implement sorting in the model

[Konrad Materka]

kmaterka added inline comments.

INLINE COMMENTS

> davidedmundson wrote in sortedsystemtraymodel.cpp:47
> I think calling QSortFilterProxyModel::lessThan(left, right); would do the 
> same
> 
> then you don't need compareDisplayAlphabetically
> 
> your implementation looks fine though, so do whichever

The question of being implicit or explicit. I like your idea more, will change 
that.

> davidedmundson wrote in sortedsystemtraymodel.h:35
> We tend not to write virtual at the start now we have the clearer override 
> keyword

Another embarrassing lack of C++ knowledge... Especially this "new" (hehe, 
C++11) feature :)

> davidedmundson wrote in systemtraymodel.cpp:115
> The applet is still alive after removeApplet is called. "this" is still alive
> 
> dataItem is not.
> 
> If an applet is added, removed   and then (potentially during applet 
> teardown) it changes its title, this would crash.
> 
> I don't know if that's a realistic scenario or not,  but I would still maybe 
> disconnect all signals from applet -> this on removeApplet?

During lifetime of the SystemTray, dataItems are never removed, they are just 
marked as not "renderable". Item is still alive, it is needed in configuration 
page (you can enable applet again in the configuration).

Anyway, it is a good idea to disconnect signals, for the sake of consisteny. In 
addition, dataItems are deleted eventually which may (?) lead to a crash on 
shutdown.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D27088

To: kmaterka, #plasma_workspaces, #plasma, davidedmundson, ngraham, broulik
Cc: mart, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra