Cleanup virutal desktops in deleted
ClosedPublic

Authored by davidedmundson on Nov 29 2018, 10:29 PM.

Details

Summary

virtual desktops can be destroyed, active clients update, but deleted
keeps a cache.

Someone needs to do cleanup to avoid dangly pointers.

Test Plan

I couldn't find a case of someone calling desktops mid way through an
animation, so it's only a hypothetical bug.

Diff Detail

Repository
R108 KWin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
Restricted Application added a project: KWin. · View Herald TranscriptNov 29 2018, 10:29 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
davidedmundson requested review of this revision.Nov 29 2018, 10:29 PM
zzag added a subscriber: zzag.Nov 29 2018, 10:50 PM

If a deleted client is/was on a single virtual desktop, then it can end up on all desktops. We probably don't want that.

Technically sure, but putting it on any arbitrary desktop is just as bad - the correct thing would be to introduce a whole new concept for a crazy edge case that I don't think can really come up.

Workspace::updateClientVisibilityOnDesktopChange doesn't include deleted. Deleted isn't emitting any signals either - so the only case we could see this be relevant is where we're viewing multiple desktops and polling dekstops.
In theory I think that could include desktop grid and desktop cube, but that would require triggering multiple actions faster than I can manage even on the slowest animation speed.

zzag added a comment.Nov 29 2018, 11:36 PM

Technically sure, but putting it on any arbitrary desktop is just as bad - the correct thing would be to introduce a whole new concept for a crazy edge case that I don't think can really come up.

Shouldn't we just destroy Deleted in that case? (if m_desktops becomes empty)

I don't want to rip out a window whilst effects are referencing it.
Most happen to connect to windowDeleted and tidy up; but I don't think we can rely on that.

zzag added a comment.Jan 7 2019, 11:55 AM

Most happen to connect to windowDeleted and tidy up; but I don't think we can rely on that.

Effects that reference closed windows have to connect to windowDeleted. We clearly state this in documentation.

Documentation also clearly states:

  • This means that a closed window is not referenced any more.

We're proposing deleting it whilst it is referenced.

zzag added a comment.Jan 7 2019, 12:00 PM
This comment was removed by zzag.
zzag added a subscriber: graesslin.Jan 7 2019, 12:17 PM

I re-read windowDeleted's documentation. It looks like it can have several meanings depending on how you read it.

I still think it would be fine to destroy Deleted even if some effects still reference it. The documentation states that effects should perform cleanup when windowDeleted is emitted, so it won't cause big problems. Though it would be great to re-clarify when windowDeleted is emitted.

Maybe @graesslin can weigh in here.

Hmmm, difficult. I think it would be wrong to delete the Deleted just because a desktop is destroyed. I don't see a problem with the deleted ending up on all desktops. No effect would start an animation and those animating the window will continue to do so.

zzag accepted this revision.Jan 10 2019, 3:42 PM
zzag added inline comments.
deleted.cpp
144

Please put a whitespace before :

This revision is now accepted and ready to land.Jan 10 2019, 3:42 PM
This revision was automatically updated to reflect the committed changes.