Bug 88857

Hmm. So the model I had in mind when writing the spec was the following:

A sub-surface's state is all the state with the wl_surface that acts as a sub-surface. However, the sub-surfaces position and z-order are with the *parent*, not the sub-surface. The parent dictates its own state (contents), and a part of those contents is the position and z-order of its immediate sub-surfaces. So, when new parent state gets applied, as part of that also the sub-surfaces' positions and z-order get applied.

This is all to keep the parent surface contents in sync with the sub-surface position and z-order. This should happen regardless of sync or desync mode of the sub-surface (or the parent, if the parent is itself a sub-surface).

If a change in the parent's content requires a change in the sub-surface's *size*, then you have to do the synchronization dance with the code driving the sub wl_surface.

The switch from synchronized to desynchronized sub-surface mode with pending position or z-order changes is sort of a corner case. I don't think carefully written clients should ever hit that, so what happens there is not very interesting IMO. Or am I overlooking something here, perhaps some sub-sub-surface scenario?

Does this help?

It is quite possible the current wording is a result from lazy thinking.

Ah, right, the position should be considered part of the parents surface, true. The issue that exists is though that the protocol explicitly says "on wl_surface.commit", which is not necessarily the same time that the parent surface gets its content updated. AFAIU the content of a surface is applied either on

 - wl_surface.commit,
 - wl_subsurface.set_desync, or
 - when parent surface state is applied

So if we have this "tree", with one toplevel surface with a single subsurface which itself has one single subsurface:

     T
     |
     s1
     |
     s2

If the following flow happens:

 1. s1::wl_subsurface.set_sync
 2. s1::wl_surface.attach
 3. s1::wl_surface.commit
 4. s2::wl_subsurface.set_desync
 5. s2::wl_subsurface.set_position
 6. s2::wl_surface.attach
 7. s2::wl_surface.commit
 8. s1::wl_subsurface.set_desync
 9. s1::wl_surface.attach
10. s1::wl_surface.commit

According to the spec the contents of s1 and s2 would be applied at 8, but the position would be applied at 10, but if I understand you correctly, the intention is that both the position and both s1's and s2's content should be applied at 8.

It can be observed by running the test case in
https://bugzilla.gnome.org/show_bug.cgi?id=731494
on weston and mutter, and observe that they behave differently because of this detail.

It could maybe be fixed by changing the protocol to no longer require an explicit parent commit, but just an "parent content applied", I think, but can we make such changes now?

(In reply to Jonas Ådahl from comment #2)
> Ah, right, the position should be considered part of the parents surface,
...
> It could maybe be fixed by changing the protocol to no longer require an
> explicit parent commit, but just an "parent content applied", I think, but
> can we make such changes now?

My intention was that parent content updates include the sub-surface's position change, which means that "both ... applied at 8" is the intended behaviour. Weston's implementation works that way, doesn't it?

Requests s1::wl_surface.attach and s2::wl_subsurface.set_position are logically part of the same state update for s1. Your example sequence confuses that when the grouping implies that s2::wl_subsurface.set_position would be related to s2::wl_surface requests.

In other words, analogous to the Wayland paradigm that a client cannot know the absolute position of its windows, the code driving updates to s2 does not know and does not control where the s2 surface appears. It may not even know it is a sub-surface. The one who positions the s2 surface is the code driving the s1 surface: the same code that drives s1::wl_surface requests drives also s2::wl_subsurface requests.

As to which way to correct the things, I suppose that depends on how much would break because of the changes. Naturally I'd like it to be what I intended, because that's the case I have thought out. The other way is unfamiliar to me and would need thinking to see if it can work.

Its not whether the example is reasonable or not, its just about what the spec dictates, and what implementations (server side) should do. I believe what makes most sense is to correct the wording to not require an explicit parent wl_surface.commit. I'd believe that'd break least things, and it is in line with the original idea; but my concern was more whether we can "fix bugs" in the spec this way?

Sure we can fix bugs in the spec, if the world becomes more happy that way.

The reason I hesitate to say "yes, just fix the spec" is that I don't have a good idea of what is already used in the wild in this particular case. But I would like to say yes.

Would be nice to hear what Jasper thinks.

Fixed by:

commit 8b2fbf872280a68f82ab6aae68a89fd58cc9996b
Author: Jonas Ådahl <jadahl@gmail.com>
Date:   Tue Mar 3 15:40:58 2015 +0800

    protocol: Change wording of subsurface placement scheduling