The subsurface protocol is a bit ambiguous regarding what should be considered a commit of a parent surface in different scenarios, and possibly at fault in others. The question is whether a deferred commit (commit during synchronized mode, then parent commits), should be recursive considered equivalent to wl_surface.commit of the parent surface. For cached surface states, this is pretty clearly stated that the state should "be applied when the parent's state gets applied", i.e. either if the parent surface goes effectively desynchronized or the state is applied by a commit coming from another parent. For positioning its different though, as the spec says that "the next wl_surface.commit on the parent surface will reset the sub-surface's position to the scheduled coordinates", and not "the next time the parent's surface state gets applied. The same applies for the z-order operations. The problem with the current wording though is that in synchronized mode, a pending positioning of the subsurface and a cached surface state of the subsurface does not get applied at the same time, as the state gets applied when its parent state gets applied, while the positioning requires an explicit wl_surface.commit on the parent surface. Another problem is when a subsurface goes from effectively synchronized to effectively desynchronized. Making that transition the cached surface state of the subsurface gets applied, but the positioning still needs an explicit wl_surface.commit on the parent surface. A correct implementation according to the current wording would result in making it impossible to make an atomic change to a subsurface tree as one would need to commit the surface states independently from the positioning (one commit to the top level surface that recursively applies all the cached states of the surfaces down the tree for the surface states, vs wl_surface.commit on every parent surface in the tree for the positioning). I don't know whether it can be considered a bug in the wording and just change "wl_surface.commit of the parent surface" to "surface state of the parent surface applied" to the positioning requests, as it would change the semantics. As a side note, weston does not follow the spec, as it applies the positioning recursively when a state is applied, not when the wl_surface.commit of the parent surface is called, so if we don't correct the spec, then we need to make weston follow it by only positioning a parents subsurfaces on an explicit wl_surface.commit of the parent surface. Please correct me if I got something wrong.
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
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.