better clean up of duplicate containments
ClosedPublic

Authored by mart on Jan 5 2017, 1:53 PM.

Details

Summary

due to old multiscreen bugs, sometimes the appletsrc file
gets polluted with a lot of containments with same activity id
and lastScreen, in some cases even hundreds
(see https://bugs.kde.org/show_bug.cgi?id=371858)
in that case we can't be 100% sure what containment will be loaded
at startup, leading to an herratical behavior.
it was trying to clean up duplicates but wasn't really effective
now base upon lastScreen (so we catch other activities as well)
and manually remove the destroyed containment from
m_desktopContainments (which may sole some multiscreen
related bug, such as 371991)

BUG:371858
CCBUG:371991

Test Plan

started a session with the corrupted appletsrc from the bugreport,
file gets cleaned out of duplicates

Diff Detail

Repository
R120 Plasma Workspace
Branch
arcpatch-D3981
Lint
No Linters Available
Unit
No Unit Test Coverage
mart updated this revision to Diff 9759.Jan 5 2017, 1:53 PM
mart retitled this revision from to better clean up of duplicate containments.
mart updated this object.
mart edited the test plan for this revision. (Show Details)
mart added a reviewer: Plasma.
Restricted Application added a project: Plasma. · View Herald TranscriptJan 5 2017, 1:53 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
mart updated this object.Jan 5 2017, 1:54 PM
davidedmundson added inline comments.
shell/shellcorona.cpp
1922

Why are you doing this?

mart added inline comments.Jan 5 2017, 2:19 PM
shell/shellcorona.cpp
1922

cont is being deleted there, i'm making sure it's not in m_desktopContainments anymore for not even an instant.
desktopContainmentDestroyed wasn't even called before as it's disconnecting the connection before destroying

davidedmundson requested changes to this revision.Jan 5 2017, 2:23 PM
davidedmundson added a reviewer: davidedmundson.
davidedmundson added inline comments.
shell/shellcorona.cpp
1922

So to make it clear what this code is actually doing:

we're disconnecting from a method that automatically does cleanup
..and now you're adding back in *the exact same cleanup* manually

What is the point of that?

This revision now requires changes to proceed.Jan 5 2017, 2:23 PM
davidedmundson added inline comments.Jan 5 2017, 2:30 PM
shell/shellcorona.cpp
1909–1910

catches also containments of other activities

No it doesn't - you're looping through m_desktopContainments.value(activity) so only things of the same activity.
Either your comment is nonsense, or your code is, or both.

mart added inline comments.Jan 5 2017, 2:49 PM
shell/shellcorona.cpp
1909–1910

yes it does.
what i mean is:
insertContainment() called with an activity that is not the current one (which will happen at startup, because insertcontainment is called for every containment, also for those in activities other than the current one), it still catches it
i can change the comment a bit, but it really does what i was meaning

1922

i could just remove that disconnect, the thing is, i'm always hesitant to change things in ways that may have had a reason.
that disconenct was added somewhere back in 2014, i can test removing it if it still has a reason

mart added inline comments.Jan 5 2017, 3:08 PM
shell/shellcorona.cpp
1922

apparently if it's managed just by the signal it seems to be too "late", as the containments don't seem to be cleaned in a single startup from the config file and i sometimes get crashes when i change activity

mart updated this revision to Diff 9766.Jan 5 2017, 3:28 PM
mart edited edge metadata.
  • remove the disconnect
mart updated this revision to Diff 9979.Jan 10 2017, 2:28 PM
mart edited edge metadata.
  • fix desktopContainmentDestroyed
davidedmundson accepted this revision.Jan 10 2017, 3:03 PM
davidedmundson edited edge metadata.
This revision is now accepted and ready to land.Jan 10 2017, 3:03 PM
This revision was automatically updated to reflect the committed changes.