D25580: [System Tray] Unified data model for System Tray items

2019-12-17 Thread Konrad Materka 
kmaterka added a comment.


  In D25580#578732 , @trmdi wrote:
  
  > Please make sure it would not bring this bug back: 
https://phabricator.kde.org/R120:6fcf9a5e03ba573fd0bfe30125f4c739b196a989
  
  
  I double checked that. If I understand correctly the root cause of BUG 393630 
was the use of `plasmoid.rootItem` - it is not introduced again.

REPOSITORY
  R120 Plasma Workspace

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

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

D25580: [System Tray] Unified data model for System Tray items

2019-12-16 Thread Tranter Madi 
trmdi added a comment.


  Please make sure it would not bring this bug back: 
https://phabricator.kde.org/R120:6fcf9a5e03ba573fd0bfe30125f4c739b196a989

REPOSITORY
  R120 Plasma Workspace

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

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

D25580: [System Tray] Unified data model for System Tray items

2019-12-16 Thread Konrad Materka 
This revision was automatically updated to reflect the committed changes.
Closed by commit R120:6d2fa8e6b812: [System Tray] Unified data model for System 
Tray items (authored by kmaterka).

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25580?vs=71232=71658

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

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

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

D25580: [System Tray] Unified data model for System Tray items

2019-12-15 Thread Nicolas Fella 
nicolasfella accepted this revision.
nicolasfella added a comment.
This revision is now accepted and ready to land.


  LGTM

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

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

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

D25580: [System Tray] Unified data model for System Tray items

2019-12-10 Thread Konrad Materka 
kmaterka updated this revision to Diff 71232.
kmaterka added a comment.


  Review fix

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25580?vs=71231=71232

BRANCH
  master

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

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

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

D25580: [System Tray] Unified data model for System Tray items

2019-12-10 Thread Konrad Materka 
kmaterka updated this revision to Diff 71231.
kmaterka marked 5 inline comments as done.
kmaterka added a comment.


  Review fix

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25580?vs=70464=71231

BRANCH
  master

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

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

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

D25580: [System Tray] Unified data model for System Tray items

2019-12-10 Thread Konrad Materka 
kmaterka marked 5 inline comments as done.
kmaterka added inline comments.

INLINE COMMENTS

> nicolasfella wrote in ConfigEntries.qml:68
> You can try doing
> 
>   for (item of systemTrayModel) {
>   ]
> 
> But I don't know if that will work

Unfortunately model is not iterable. This code will be removed in D22176 
 anyway.

> nicolasfella wrote in systemtraymodel.cpp:60
> roles.insert(ItemType, ...) should be enough

I'm using `enum class` instead of simple `enum`, so explicit cast is required.

> nicolasfella wrote in systemtraymodel.cpp:74
> This could be a candidate for std::find_if

It would be nice, but does QStandardItemModel have an iterator?

> nicolasfella wrote in systemtraymodel.cpp:153
> if (contains) {
> 
>   return stuff
> 
> }
> 
> otherStuff()

OK, changed.

> nicolasfella wrote in systemtraymodel.h:77
> What's the purpose of the *Changed roles?

All of these roles are copied from StatusNotifierItemSource. It might be an 
overkill to support all.
I don't know exactly what was the intention to use "*Changed" roles, there is 
only one comment: `// record what has changed`. I found usages in old KDE4 code:

> ./kde4/kde-workspace/plasma-workspace/applets/systemtray/plugin/protocols/dbussystemtray/dbussystemtraytask.cpp



  void DBusSystemTrayTask::dataUpdated(const QString , const 
Plasma::DataEngine::Data )
  {
  // 
  if (properties["TitleChanged"].toBool() || becomeValid) {
  QString title = properties["Title"].toString();
  // ...
  }
  // ...

REPOSITORY
  R120 Plasma Workspace

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

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

D25580: [System Tray] Unified data model for System Tray items

2019-12-08 Thread Nicolas Fella 
nicolasfella added a comment.


  Looks very good in general. A few style nitpicks

INLINE COMMENTS

> ConfigEntries.qml:68
>  var list = [];
> -for (var i = 0; i < statusNotifierModel.count; ++i) {
> -var item = statusNotifierModel.get(i);
> -list.push({
> -"index": i,
> -"taskId": item.Id,
> -"name": item.Title,
> -"iconName": item.IconName,
> -"icon": item.Icon
> -});
> -}
> -var lastIndex = list.length;
> -for (var i = 0; i < plasmoid.applets.length; ++i) {
> -var item = plasmoid.applets[i]
> +for (var i = 0; i < systemTrayModel.count; i++) {
> +var item = systemTrayModel.get(i);

You can try doing

  for (item of systemTrayModel) {
  ]

But I don't know if that will work

> systemtraymodel.cpp:60
> +
> +roles.insert(static_cast(BaseRole::ItemType), 
> QByteArrayLiteral("itemType"));
> +roles.insert(static_cast(BaseRole::ItemId), 
> QByteArrayLiteral("itemId"));

roles.insert(ItemType, ...) should be enough

> systemtraymodel.cpp:74
> +QStandardItem *dataItem = nullptr;
> +for (int i = 0; i < rowCount(); i++) {
> +QStandardItem *currentItem = item(i);

This could be a candidate for std::find_if

> systemtraymodel.cpp:153
> +{
> +if (!m_services.contains(source)) {
> +Plasma::Service *service = m_dataEngine->serviceForSource(source);

if (contains) {

  return stuff

}

otherStuff()

> systemtraymodel.h:77
> +Title,
> +TitleChanged,
> +ToolTipChanged,

What's the purpose of the *Changed roles?

REPOSITORY
  R120 Plasma Workspace

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

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

D25580: [System Tray] Unified data model for System Tray items

2019-11-27 Thread Konrad Materka 
kmaterka added a comment.


  This is a simplified version of D23413  
needed for the D22176  to move forward.

REPOSITORY
  R120 Plasma Workspace

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

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

D25580: [System Tray] Unified data model for System Tray items

2019-11-27 Thread Konrad Materka 
kmaterka created this revision.
kmaterka added reviewers: Plasma, broulik.
kmaterka added a project: Plasma.
kmaterka requested review of this revision.

REVISION SUMMARY
  Currently there are two different sources of SystemTray items:
  
  - Plasmoids
  - Status Notifier
  
  Thi change adds new model that holds both Plasmoids and SNIs.

TEST PLAN
  Add/disable applets. Start/stop SNI app.

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

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

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

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