Fix ksysguard not starting on plasmoid click
ClosedPublic

Authored by maxrd2 on Sep 2 2017, 12:27 AM.

Details

Summary

Plasmoid now uses KRun::openService() to launch ksysguard instead of KRun::openUrl() and apps data source.

After plasmoid has been running for some time (several hours, maybe days), clicking it would produce error:
"file:///usr/share/plasma/plasmoids/org.kde.plasma.systemloadviewer/contents/ui/SystemLoadViewer.qml:389: TypeError: Cannot read property 'entryPath' of undefined"
instead of launching ksysguard.

Ksysguard now starts without delay after click, since url doesn't have to be looked up.

Diff Detail

Repository
R114 Plasma Addons
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
maxrd2 created this revision.Sep 2 2017, 12:27 AM
Restricted Application added a subscriber: plasma-devel. · View Herald TranscriptSep 2 2017, 12:27 AM
maxrd2 added a reviewer: sars.Sep 3 2017, 9:47 AM

Thanks, but do you know why the apps datasource doesn't have that entry after a while?

maxrd2 added a comment.EditedSep 3 2017, 10:20 AM

Am not sure why apps.data["org.kde.ksysguard.desktop"] becomes undefined.
Think it might be a bug in PlasmaCore.DataSource? I haven't noticed any log messages apart that TypeError.

Will try to see if sourceDisconnected/Removed is signaled at some point.
It will probably take some time before i manage to get more info this, issue starts happening after applet's been running long time.

I have also noticed with this change that there is no (few second) delay after clicking the applet and ksysguard starting to launch.
IMO that is welcome improvement especially when some process starts using 100% cpu or eating ram.

maxrd2 removed a reviewer: sars.Sep 3 2017, 11:00 AM

The problem I have is that if we see a bug, and then work round it instead of understanding it, we still have a bug - and that will just come up again in a different place again and again.

I have also noticed with this change that there is no (few second) delay after clicking the applet and ksysguard starting to launch.

That comes back to the exact same question; why is there a few second delay when getting a (supposedly) pre-cached entry from the DataEngine.


The other viable option, which IMHO would make this code far far neater is adding a

KRunProxy::openService(const QString name); in kdeclarative
which uses KService::serviceByName() instead of serviceByDesktopPath

then we can kill the apps dataengine completely and not have to worry about it.

maxrd2 added a comment.Sep 3 2017, 1:00 PM

Great will make those changes/additions then.
Do you know why was KService::serviceByName() deprecated in KDE 4? (https://api.kde.org/4.x-api/kdelibs-apidocs/kdecore/html/classKService.html#a2c5b00fd381b9843b6ba99da692c90ae)

maxrd2 added a comment.Sep 3 2017, 1:03 PM

Also seems KService::serviceByName() was removed from KF5.. so it's ok just to re-add it?
https://github.com/KDE/kservice/blob/master/src/services/kservice.h

sorry ::serviceByDesktopName() isn't deprecated and does what we want.

maxrd2 updated this revision to Diff 19134.Sep 3 2017, 1:49 PM
maxrd2 edited the summary of this revision. (Show Details)

Added KRun::openService() method

Restricted Application added a project: Frameworks. · View Herald TranscriptSep 3 2017, 1:49 PM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
maxrd2 updated this revision to Diff 19136.Sep 3 2017, 1:50 PM

Plasmoid uses KRun::openService() to launch ksysguard

maxrd2 added a comment.Sep 3 2017, 1:58 PM

There it goes... updated the qml and added KRunProxy changes here: D7676

davidedmundson accepted this revision.Sep 3 2017, 2:08 PM
This revision is now accepted and ready to land.Sep 3 2017, 2:08 PM
maxrd2 added a comment.Sep 3 2017, 2:23 PM

I don't have full developer account so i can't land this one. It also needs D7676 for this to work.

We have a slight problem (and I'm sorry as this is my fault)

Plasma 5.10 will depend on frameworks 5.38.
This was just tagged and doesn't include your krun change.

If this was a feature I would be really strict and make us wait till Plasma 5.11.

As this is a bug fix, and frameworks 5.39 will be out just before Plasma 5.10, I think we can grant an exception. If a user does use 5.37, it'll just give a warning and continue.

I'm going to wait until after the beta is released to merge this change.

Thanks, but do you know why the apps datasource doesn't have that entry after a while?

I've managed to get some more insight into original problem.
It seems that after some system updates, desktop database gets updated, and that triggers a bunch of onSourcesChanged+onSourcesRemoved events, followed by a bunch onSourcesChanged+onSourceAdded events.
Since I'm on rolling distro i get updates very frequently and this problem happens to me pretty frequently.

After .desktop database is updated these events are fired on 'apps' dataengine:

.... bunch of events for various .desktop files....
Sep 08 00:44:19
  onSourcesChanged()
  onSourceDisconnected() org.kde.ksysguard.desktop
  onConnectedSourcesChanged()
  onSourceRemoved() org.kde.ksysguard.desktop
.... bunch of events for various .desktop files....
Sep 08 00:47:01
  onSourceAdded() org.kde.ksysguard.desktop
.... bunch of events for various .desktop files....
Sep 08 00:47:43
  onSourceRemoved() org.kde.ksysguard.desktop
.... bunch of events for various .desktop files....
Sep 08 00:49:54
  onSourceAdded() org.kde.ksysguard.desktop
.... bunch of events for various .desktop files....

No onSourceConnected() event was fired for "org.kde.ksysguard.desktop" after it has been disconnected -> removed -> added -> removed -> added.
It wasn't connected to datasource again after it was updated and re-added and thus apps.data["org.kde.ksysguard.desktop"] became undefined.

I can look into fixing/changing apps dataengine and making it re-connect "lost" sources if that is desired.
Is that supposed to happen automatically or is qml script supposed to re-connect "lost" sources during onSourceAdded event?

cfeck added a subscriber: cfeck.Sep 28 2017, 11:47 AM

David, the beta was released.

This revision was automatically updated to reflect the committed changes.