what does the test suite say about it? Are all tests still passing? Also do you think it's possible to create a test case for it? I think I tried in the past and didn't succeed.

In D13084#267846, @graesslin wrote:

what does the test suite say about it? Are all tests still passing? Also do you think it's possible to create a test case for it? I think I tried in the past and didn't succeed.

Tests are still passing. I thought about a test case, but it would be difficult to simulate and wouldn't tell us much since this is a very specific corner case. The problem here is that the position of the internal QWindow from the QML file is set before the PlasmaShellSurface is installed, which then overwrites the QML position value with the invalid value in the updatePosition call.

This is a very specific case only concerning internal windows with predefined geometry values. For such windows you probably don't want to react on any PlasmaShellSurface events in general or only on non-location data, but this would need a larger discussion on what should be available to internal windows from the protocol and what not as well as careful consideration on how to separate these special cases.

Besides that what makes it somewhat more broken, is that as in D13037 discussed, interfaces for internal windows might never be bound because the roundtrip is not blocking I assume because the client is the server as well.

Can we get this merged now as a quick fix for the bug? In the future we might want to discuss the overall structure of internal windows and their Wayland interfaces.

I fully support only having one code path to do placement, not two out of sync.

So why do we have this "|| rect.isValid()" ?

In D13084#283789, @davidedmundson wrote:

I fully support only having one code path to do placement, not two out of sync.

So why do we have this "|| rect.isValid()" ?

The internal window code seems to be somewhat brittle in the Wayland session in general. So I wasn't sure if just disabling all geometry calls through the Plasma shell surface wouldn't break on other unexpected ends. So I wanted to just block the obvious wrong case, when it tries to set the geometry to an invalid rect. But if you think we should directly just in general disallow any geometry updates through the PlasmaShellSurface, I can change the diff.

But if you think we should directly just in general disallow any geometry updates through the PlasmaShellSurface

IMHO yes. (through either this patch or in the QPA or in dialog) Having two providers of the same information is wrong, we should fix code paths to make sense, not just where we see bugs.
If it is brittle, we need to fix that regardless.

In D13084#285051, @davidedmundson wrote:

But if you think we should directly just in general disallow any geometry updates through the PlasmaShellSurface

IMHO yes. (through either this patch or in the QPA or in dialog) Having two providers of the same information is wrong, we should fix code paths to make sense, not just where we see bugs.
If it is brittle, we need to fix that regardless.

Unfortunately there are code areas where the way to set it is through the Plasmashell API and other areas where it is through the internal setting mechanism.

Unfortunately there are code areas where the way to set it is through the Plasmashell API and other areas where it is through the internal setting mechanism.

Sure, we have surfaces with one, some with another. I don't think that's feasible to avoid.

I don't think there is a valid case where a single surface needs both mechanisms. That's the case I'd like to see fixed at the root.

In D13084#287268, @davidedmundson wrote:

I don't think there is a valid case where a single surface needs both mechanisms. That's the case I'd like to see fixed at the root.

For sure. But this needs some deeper research on how to restructure the internal window code. On the other side the current situation on Wayland with a centered TabBox is unbearable because it is not correctly positioned and/or not updated at all. This patch fixes the problem in the short term before we can look into reworking the internal window code on a larger scale.

What do you propose then? It makes sense in kwayland-integration to create the Plasma Shell Surface interface for the skip taskbar and skip pager properties and for Dialog to call the setState function on these properties.

One can shield against unset positions by checking the isPositionSet call to the Plasma Shell Surface. But another problem is that the m_clientSize value is not defined in all cases early enough.

One can shield

shielding means we've already gone wrong, and you'll still get broken shadows on the first tabbox.

Propositions for fixing Dialog

1. Fix roundtrip in kwin's QPA. Would mean a nested event loop until we hit eventsRead on the connectionThread.

Technically correct, technically disgusting.

2. Have a static/shared client registry that we process in kwin in a safe way ahead of any Dialog construction then port Plasma::Dialog/KWindowSystem et al. to use that. Might be a good idea as it will reduce some pointless traffic.

3. Move all PlasmaShellSurface code to be in KWindowSystem (and therefore share the same registry). Will probably require new public API, but if we expose setState over this shell, surely it makes sense to expose setPosition over this shell too? Has the potential to clean up Plasma code.

In D13084#291552, @davidedmundson wrote:

One can shield

shielding means we've already gone wrong, and you'll still get broken shadows on the first tabbox.

With this patch shadows work fine on first tabbox. If they are broken elsewhere it might be a different issue.

Propositions for fixing Dialog

1. Fix roundtrip in kwin's QPA. Would mean a nested event loop until we hit eventsRead on the connectionThread. Technically correct, technically disgusting.
2. Have a static/shared client registry that we process in kwin in a safe way ahead of any Dialog construction then port Plasma::Dialog/KWindowSystem et al. to use that. Might be a good idea as it will reduce some pointless traffic.
3. Move all PlasmaShellSurface code to be in KWindowSystem (and therefore share the same registry). Will probably require new public API, but if we expose setState over this shell, surely it makes sense to expose setPosition over this shell too? Has the potential to clean up Plasma code.

If we want to do any of these ideas, this will need more time to figure out which one works and how to do this one then. Since this won't be possible to do for 5.14, we should use the workaround here for this release. Because centered TabBoxes are not functional at all on Wayland at the moment.

No.

If you want to act like a dictator telling people what do to without reasoning, I can do that too.

EDIT: I give you 5 minutes for an apology and reasoning why we should do a broken release without a temporary workaround or I'll land this patch.

I have explained multiple times above.
You can't just ignore my comments and badger me repeatedly till I give in and accept it anyway.

The initial investigation was wrong, the rationale was wrong, the patch is wrong, you did *nothing* to address any of this except repeatedly push me to accept this anyway??!

I do not remotely owe you an apology.

In D13084#319377, @davidedmundson wrote:

I have explained multiple times above.

But you haven't. You did "some investigation" without coming up with a proper overall solution, instead throwing three solution ideas at the wall, maybe none of them working in the end, but certainly not doable until 5.14 beta. You can prove me wrong if you want to. Would be happy about it.

You can't just ignore my comments and badger me repeatedly till I give in and accept it anyway.

If it felt this way to you, tell me. But from my side I gave you over a month to come up with a different solution, so I'm not overly badgering you.

The initial investigation was wrong, the rationale was wrong, the patch is wrong, you did *nothing* to address any of this except repeatedly push me to accept this anyway??!

This patch is there to solve a specific bug people complained about. In my tests it works without problems, so let's roll with it until we have a "right investigation" with a "right rationale" and a "right patch", whatever this means.

I do not remotely owe you an apology.

Think what you want.