Improve multi-desktop folderview behavior
ClosedPublic

Authored by amantia on Feb 21 2018, 8:36 PM.

Details

Summary
  1. remove the items from the active map when they are on a disabled screen.

Fixes the case of Screen 1 showing Foo, Screen 2 showing Bar, Screen 1
switching to Bar (files from under Foo should disappear), Screen 2 switches to
Foo (files from under Foo should appear now on screen 2, they did not before)

  1. don't store twice an item in the m_itemsOnDisablesScreensMap. This could

happen before with setups having more than two screens.

  1. Remove m_firstScreenForPath and instead store the screen ids per url in

m_screensPerPath and use the one with lowest id from it for the first screen.
Help with detecting the first valid screen in 2+ screen scenarios.

  1. remove the isEmpty check in addScreen, that caused all items to be added

under a screen with "desktop:///" path, even if they belonged somewhere else.

  1. Save and restore the items-on-disabled-screen map to the config file.

This should help in having a consistent behavior after restarting plasma.
It could help with bug 389745

BUG: 390676
CCBUG: 389745

Diff Detail

Branch
Plasma/5.12
Lint
No Linters Available
Unit
No Unit Test Coverage
amantia requested review of this revision.Feb 21 2018, 8:36 PM
amantia created this revision.
amantia set the repository for this revision to R119 Plasma Desktop.
Restricted Application added a project: Plasma. · View Herald TranscriptFeb 21 2018, 8:40 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
hein added a comment.Feb 28 2018, 10:18 AM

Since this seems to change the map format, is this safe to upgrade for users with existing config?

It adds a new entry, doesn't change an existing one. If that is not there, I see no problems, the ScreenMapper::readDisabledScreensMap() will not initialize the m_itemsOnDisabledScreensMap. That map was not saved previously, thus the folderview behaving differently after a restart, as the map was lost.

hein accepted this revision.Feb 28 2018, 10:21 AM

Ok, sounds and looks convincing, thanks!

This revision is now accepted and ready to land.Feb 28 2018, 10:21 AM
hein added a comment.Feb 28 2018, 10:22 AM

This should go into 5.12 btw (ditto D10729)

Yes, I will push first to 5.12 both of them

I can't put to 5.12 as it seems https://phabricator.kde.org/D9325 didn't get into the 5.12 branch, and I don't want two versions of this patch. May I cherry-pick that commit to 5.12 and apply this patch?

hein added a comment.Mar 1 2018, 9:16 PM

I can't put to 5.12 as it seems https://phabricator.kde.org/D9325 didn't get into the 5.12 branch, and I don't want two versions of this patch. May I cherry-pick that commit to 5.12 and apply this patch?

Go ahead!

amantia updated this revision to Diff 28998.Mar 8 2018, 10:56 AM

Moved to 5.12

amantia closed this revision.Mar 8 2018, 10:57 AM