Stupid me: please ignore this fragment. It survived some editing but should
have been deleted.

>     The funny thing is, when plasmashell is restarted after the crash, 
>     all widgets stays on the wrong screen.

When disconnecting DP-0, in my debug-build I do see
    ASSERT: "m_desktopViewforId.value(idx) == desktopView" in file plasma-workspace-5.8.4/shell/shellcorona.cpp

situated in ShellCorona::removeDesktop. This method is called after primaryOutputChanged() returned.

When reconnecting DP-0, I do not gain any new information. primaryOutputChanged() is called and returns cleanly.

So my guess is: primaryOutputChanged() does not work as intended. But the crash on the disconnect actually leads to a restart which leads to the intended result.

Side-Note: I am using qt 5.7, gcc 6.2, frameworks 5.28, plasma 5.8.4, and applications 16.08.3.

Hi!

I will try to dig through this, slowly... Right now I am trying to understand what happens in removeDesktop to trigger the Q_ASSERT in hopes that I find a good indication of what goes against expectations when the primary display switches away from the disconnected screen.

So. In my case, when removing DP-0, removeDesktop is called with
    DesktopView: 0x19e2fd0
    desktopView->screenToFollow()->name() is LVDS-0
[Comment: This seems reasonable, as the view on LVDS-0 is going to be replaced by the one from DP-0. On the other hand, removeDesktop was called by screenRemoved, where I would have expected the QScreen for DP-0 to be removed. Thus I am assuming now a mixup in the screen numbering, when the logical screen order is modified in terms of the primary screen. But as I have no clue of the underlying APIs, what they should do etc. pp., this is just a wild guess.]

In the end, this situation leads to (after mapping via the ScreenPool)
    idx = 0

Now, I cannot say much on panels, as I don't use one for like 10 years. Thus I will ignore the next loop.

Of course, the desktop view associated to idx via m_desktopViewforId now doesn't match the method parameter. Still, I don't what should happen in this case, e.g., should the primary view be transferred from the disconnected screen to the still available, or should the first view reconfigure itself to take on the contents of the other screen. Thus, I don't know if the assert is wrong. But as stated above, my intuition is, that the screen numbering in the internal representations mixes up.

Hope this helps,

Jan

There aren't many (0!) comments indicating what should happen.
But from looking at other parts of the code,
    QScreen *oldPrimary = m_desktopViewforId.value(0)->screen();

the primary DesktopView always has id 0.

Now, primaryOutputChanged is called before removeDesktop. As far as I understand it, the screen for the primary desktopView is switched and thus the assert does
not hold anymore, when removeDesktop is called now.

Okay. Looks like the m_screenPool-Numbering is inconsistend with the m_desktopViewforId-Numbering. THerefore, in primaryOutputChanged, the id for the old as well as the new primary DesktopView is == 0 and no swap takes place!

In general, desktopForScreen does not return the correct DesktopView. This should be the reason, why the assert fails. When patching desktopForScreen, the Q_ASSERT does not explode.

When using the patched desktopForScreen insted of the m_screenPool in primaryOutputChanged, then it does not explode, too, and the primary desktop is at the right place.

I will attach the patch now.

Created attachment 102480 [details]
Remove a bizzare include.

This patch removes a ktexteditor include in shellcorona.cpp.

Created attachment 102481 [details]
Reimplement desktopForScreen without using m_ScreenPool, now providing correct results and fixing the primary output switching problem.

Of course, for the final patches I forgot to go to the root folder. They have to be applied directly in the shell folder...

Unluckily, this does not solve problems with another Testcase:
Config 1: VGA-0 + DP-4 (primary)
Config 2: LVDS-0 (primary)

Here things are still going south and plasma falls out.
No matter if I remove the laptop in a running state from the docking station or in sleep mode, chances are high, that plasmashell doesn't even register all screen changes and is in a somewhat bizarre state.
For example in Config 2, only the part of the desktop view is shown, where DP-4 overlaps with the coordinate system of LVDS-0.

I think this patch https://phabricator.kde.org/D3519 will fix your problem. I hope it will be merged soon.

(In reply to Jakub Gocoł from comment #11)
> I think this patch https://phabricator.kde.org/D3519 will fix your problem.
> I hope it will be merged soon.

Hi! I found your patch and am already testing with it. Unluckily, testing involves not only plasma crashes but sometimes freezes when suspending & switching monitors quite often. This means, I won't have useful results before next week.
Especially in the case where the primary screen is on an external monitor and LVDS-0 (the internal display) seems to crash in any combination of all our patches. :-( The switch-displays-logic seems to go boinc in this case. But this case is really annoying to reproduce.
For the simple case with one additional external display I do think so, too. :-)

Try to reproduce and then describe simple scenarios in as few steps as possible. Because you described so many scenarios, I don't understand what exactly works and what doesn't work. 

1. In first message, in point 1, you described scenario with change of primary screen. Did my patch resolved that issue for you?
2. Can you provide exact steps to crash plasma? I don't understand what exactly do you do.

*** Bug 373098 has been marked as a duplicate of this bug. ***

(In reply to Jakub Gocoł from comment #11)
> I think this patch https://phabricator.kde.org/D3519 will fix your problem.
> I hope it will be merged soon.

Hi!

I was unable to reproduce this crash anymore. It looks like your patch fixes the problem. I don't know what happend with the last crash I had seen.

Thanks!

(In reply to Jakub Gocoł from comment #11)
> I think this patch https://phabricator.kde.org/D3519 will fix your problem.
> I hope it will be merged soon.

any news on this? it's marked as accepted since a while but still not submitted, can you push it?

(In reply to Jan-Matthias Braun from comment #8)
> Created attachment 102481 [details]
> Reimplement desktopForScreen without using m_ScreenPool, now providing
> correct results and fixing the primary output switching problem.

can you push it?
the patch makes sense, i can push it if needed

Git commit bdfa0f3fb726f4c639d2ea86d829c25c89779025 by David Edmundson, on behalf of Jakub Gocoł.
Committed on 15/12/2016 at 13:42.
Pushed by davidedmundson into branch 'Plasma/5.8'.

Move updating of primary screen in screenpool after fetching its id

Summary: We need old id of new primary screen. After we update primary screen in screenpool, it will always return id = 0. It causes invalid m_desktopViewforId mapping and  panel doesn't move next time when we change primary screen.

Test Plan:
Preconditions:
Computer with 1 display, running plasmashell

Test steps:
1. Connect one external screen (first screen is primary)
2. Change primary screen to second screen
3. Change primary screen back to first screen
4.  Unplug second screen

Expected:
In step 3 panel moves to first screen
In step 4 plasmashell keeps running

Actual (before change):
In step 3 panel remains on second display
In step 4 plasmashell crashes

Reviewers: #plasma, davidedmundson

Reviewed By: #plasma, davidedmundson

Subscribers: plasma-devel

Tags: #plasma

Differential Revision: https://phabricator.kde.org/D3519

M  +1    -2    shell/shellcorona.cpp

https://commits.kde.org/plasma-workspace/bdfa0f3fb726f4c639d2ea86d829c25c89779025

(In reply to Marco Martin from comment #17)
> (In reply to Jan-Matthias Braun from comment #8)
> > Created attachment 102481 [details]
> > Reimplement desktopForScreen without using m_ScreenPool, now providing
> > correct results and fixing the primary output switching problem.
> 
> can you push it?
> the patch makes sense, i can push it if needed

As I was fiddling in unknown code, of which I didn't (and still don't) know exactly know everywhere how it should behave, I am unsure about the use of my patch. I do have the impression, that the crash which occurs when the new primary display was not in use before, e.g., when two external displays are disconnected and the laptop display is reactivated to become the primary display, was connected to my try on a fix.

This could be possible, because in this case the used hash shouldn't provide a reference to the new primary screen.

But I did not test this hypothesis, because the other patch on phabricater seems to fix the issue better. I think, I saw this crash without my patch too., but ... this whole multi-display thingy is quite unstable in my experience, still, and therefore ugly to test & debug.

You see, I am somewhat guarded against using my patch. The other patch does good, so lets go with that. Probably that guy knew better. :-)