I'd like to get some feedback on the approach, however. I think this is
actually a nice minimal fix, as we can't really change how the id is generated. I may be missing something here.

This obviously needs some testing. It shouldn't break existing setups, but I can't really verify if it actually works, since I don't have identical screens. In *theory*, I think this is a sound fix for this quite serious bug.

I have two identical screens set up as part of my 3 monitor set. I can test this.

asking the evil question ;-) Can we autotest this?

I have identical screens so can test when I'm back (Tuesday) if you give me some specific thing to test.

Mine's mostly been working since I did b8b2126dbfb1d0cf5c771baf23a29e1130a70fba in kscreen which also makes sure we don't match the same output twice.

Can you talk through what your patch is doing.
I think I'm missing something as - if the names are also identical you'll surely skip both screens when trying to match an output - and if they're not the same, you'll not acheive anything?

@davidedmundson Sure.

With this patch

• we detect the case when multiple identical outputs are connected (same hash)
• each of the duplicate outputs also gets matched against the output name

The problem I'm fixing is that currently (without my patch) the ordering of outputs matters, so if an output appears later on, it ends up in a different place in the list. This may confuse the current algorithm which relies on the ordering of config->connectedOutputs() since it'll match the first and remove that -- there's no guarantee that the first match is the right one, however. It works in the most likely case, when all outputs are added the same order as in the config file, but breaks once I mix up this ordering.

The config file also holds the output's name, and since that's usually the connector's name and likely to be unique, it's a good indicator for identifying an output.

If you look at the config file in https://bugs.kde.org/show_bug.cgi?id=325277 (grep for e7d18f), this becomes perhaps a bit clearer.

In the second iteration of this patch, I've tightened up the continue condition a bit (and fixed it, I used the wrong json object), so it's less likely to skip outputs when the name is empty.

I've added an autotest which sets up a 3x2 video wall and loads the config from the bugreport. This test fails without the change in findOutputs() and passes with it. I've played a bit around with it, and if I add the output programmatically in the same order as the config file has them (alphanumeric) it works without my patch, changing the ordering makes the tests fail without, and pass with my patch.

tl,dr; I think your patch makes it work in the most likely cases, but may not be enough when displays are being added in a different order.

@lbeltrame Does it work for you?

If you can try this patch, that'd be awesome (also if it already works for you, to check if the patch breaks anything).

For now I didnt notice any problems (with version 1 of the patch), at least no regressions (notice that I wasn't able to reproduce the issue described in the bug since a long time).

Is there any difference worth testing in the new revision?

Looks good to me

With this patch

• we detect the case when multiple identical outputs are connected (same hash)
• each of the duplicate outputs also gets matched against the output name

OK, that makes sense.

But I think then you need to revert the patch I did and always pass the entire output list to findOutput
Otherwise only the first duplicate will know it's a duplicate and go through the name check.

@davidedmundson Good point, I'll revert that one and will give it another good round of testing. It'd be nice if you could test once you're back next week (same for @lbeltrame, of course.)

I've been using this all week without ill side effects. In fact, this solves an issue I had when testing notmart's p-w branch.

So far, no bad side effects that I can tell, even with the newer version.

@davidedmundson Did you also get to try it?