Bug 365606 - createActivity in plasma script unreliable
Summary: createActivity in plasma script unreliable
Status: RESOLVED FIXED
Alias: None
Product: kactivitymanagerd
Classification: Unclassified
Component: general (show other bugs)
Version: 5.7.0
Platform: openSUSE RPMs Linux
: NOR normal
Target Milestone: ---
Assignee: Ivan Čukić
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-07-13 11:29 UTC by Fabian Vogt
Modified: 2016-07-18 10:58 UTC (History)
4 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Fabian Vogt 2016-07-13 11:29:16 UTC
The default plasma startup script (https://github.com/KDE/plasma-desktop/blob/master/desktoppackage/contents/layout.js) creates an activity called "Desktop", but that does only appear sometimes in the activity switcher and desktopsForActivity(id) for that id also returns 0 in that case.
Adding while(desktopsForActivity(id).length == 0) sleep(10); does not make it work, so it's not a race condition in the script itself.

As a result, customizations in that script, like https://build.opensuse.org/package/view_file/openSUSE:Leap:42.1:Update/plasma5-openSUSE/plasma-change-defaults.diff?expand=1 do not work as expected, as they only touch the newly created activity. This customization, for instance, adds a folderview widget, but on Plasma 5.7.0 it does not appear after logging in.

This does not seem to be broken in Plasma < 5.7.0.

Reproducible: Sometimes
Comment 1 Luca Beltrame 2016-07-13 11:35:10 UTC
Adding notmart also to the CC, as this was discussed with him on IRC.
Comment 2 Ivan Čukić 2016-07-13 11:36:33 UTC
There were no (intentional) changes to kamd in recent releases. I'll investigate, but I think the problem is in plasmashell.
Comment 3 Marco Martin 2016-07-13 14:25:56 UTC
possible patch:

    diff --git a/shell/shellcorona.cpp b/shell/shellcorona.cpp                              
    index 43ce83d..a0b25a4 100644
    --- a/shell/shellcorona.cpp
    +++ b/shell/shellcorona.cpp
    @@ -1237,9 +1237,11 @@ Activity *ShellCorona::activity(const QString &id)
     void ShellCorona::insertActivity(const QString &id, Activity *activity)
     {
         m_activities.insert(id, activity);
    -    Plasma::Containment *c = createContainmentForActivity(id, m_views.count());
    -    if (c) {
    -        c->config().writeEntry("lastScreen", 0);
    +    for (int i = 0; i < m_views.count(); ++i) {
    +        Plasma::Containment *c = createContainmentForActivity(id, i);
    +        if (c) {
    +            c->config().writeEntry("lastScreen", i);
    +        }
         }
     }
Comment 4 Marco Martin 2016-07-13 17:04:33 UTC
It seems that when it doesn't work, the created activity is actually missing in ~/.config/kactivitymanagerdrc 
that wouldsuggest a problem in kactivitymanager?
Comment 5 Fabian Vogt 2016-07-13 17:19:37 UTC
Adding a sleep(100); after the call to loadTemplate seems to work around the issue reliably.
sleep(100) before loadTemplate does not have the desired effect.
Comment 6 Wolfgang Bauer 2016-07-13 17:22:00 UTC
I suspect this is a race condition in starting plasmashell and kactivitymanagerd.

When it doesn't work, I get the following line in .xsession-errors:
ShellCorona::checkActivities is called whilst activity daemon is still connecting

If it works, this line is not there.
Comment 7 Marco Martin 2016-07-13 18:00:15 UTC
it certainly seems a probable cause.
however, i see in shellcorona.cpp, line 303 
connect(m_activityConsumer, &KActivities::Consumer::serviceStatusChanged, this, &ShellCorona::load, Qt::UniqueConnection);

ivan: is this enough to make sure kactivitymanager is running and responding? something else needed?
Comment 8 Wolfgang Bauer 2016-07-13 20:41:01 UTC
(In reply to Marco Martin from comment #7)
> however, i see in shellcorona.cpp, line 303 
> connect(m_activityConsumer, &KActivities::Consumer::serviceStatusChanged,
> this, &ShellCorona::load, Qt::UniqueConnection);

Yes, but immediately afterwards load() is called anyway, which only bails out if the consumer status is Unknown.
Apparently it is NotRunning at that point though (if the problem occurs at least), so load() continues normally, calls checkActivities() and loads the default layout.

I verified that the status is NotRunning in checkActivities() when that message is printed.
Comment 9 Ivan Čukić 2016-07-13 20:54:26 UTC
I've been debugging this a bit.

The ScriptEngine::createActivity sometimes does not call addActivity, but setActivityName - seems like plasma does not react properly to some of the activities that were created, does not create containments for them although KAMD registers them, and then the knownActivities in the scriptengine.cpp is not valid.

@Marco - can you explain what the else branch with setActivityName is supposed to do? The logic behind this method looks strange to say the least - 'if Plasma does not have a containment for a specific activity, lets rename it'.

p.s. For activity creation debugging, I advise to use the test app built with kactivities library which shows the currently existing activities tests/activities-model/KActivitiesModelTestApp in parallel with plasma so that you (if said test app does not have bugs :) ) you can see what KAMD has.
Comment 10 Ivan Čukić 2016-07-15 18:29:49 UTC
After the first problematic part (the above one which is now fixed), the second problem is that sometimes insertActivity is called before activityAdded and sometimes after.

insertActivity is called be the script engine, and activityAdded when the activity manager notifies plasma of the new activity that has been created.

The fun thing is that this is not the core of the problem. Even when the order is forced, something is wrong - the script returns 0 for the number of desktops sometimes or most of the time.
Comment 11 Ivan Čukić 2016-07-15 18:49:44 UTC
Ok, the final problem found - ScriptEngine::desktopsForActivity tests whether the activity exists and it is called before the signal that the activity has been created arrived in plasma.

The problematic situation:
1. The script requests activity creation
2. Activity manager registers a new activity
3. desktopsForActivity tests whether activity exists, it does not as far as plasma is concerned
4. Plasma gets the signal that the new activity has been created

Now, this might be something to change in the activities framework - not to wait until the 'new activity' signal arrives before adding it to local cache, but I'm not sure it would be a good idea. There are currently no synchronization guarantees in kactivities.

(the weekend is a lonely time on IRC, so I'm posting the real-time findings here :) )
Comment 12 Ivan Čukić 2016-07-15 22:03:24 UTC
Changing KActivities to be more synchronous is a tad more intrusive than I hoped it would be - it is designed to have everything as asynchronous as possible.

The change in the ScriptEngine would be much simpler, and would IMO be what it should have done in the first place. The proposed patch is here:

https://phabricator.kde.org/D2187
Comment 13 Ivan Čukić 2016-07-18 10:58:36 UTC
Fixed in 3af24524702e92215a9cce4dcba6b421ac51be4e

    ScriptEngine exports the list of activities known to Plasma
    
    Summary:
    There is a small time-window when Plasma's internal data
    regarding activities is not synchronized with the internal
    data of the KActivities library.
    
    This leads to Plasma reporting a different list of activities
    than the list of activities it has containments for.
    
    This patch changes the activity list that the ScriptEngine reports
    to be the one that Plasma knows about.