Handle xdg popups and subsurfaces

[ghost]

I started this, because it happened quite often that menues were off-screen. Damage tracking (in its current state) completely ignores them (#38), so that’s one more issue fixed here. One last thing was rendering them above (almost) everything, but I think that can wait for another PR (I’m not sure how soon or late it would be done).

A problem that’s still present is that subsurfaces are moved into the output only for the left (0,0) output, but it’s the same in Sway so I guess I can’t do anything about it. [Edit 2021-08-18: Actually, it seems to have been that way even before. My test client (Sublime Merge, with its button tooltips being the subsurfaces) doesn’t care about the enter event at all.]

I have left a few questions around in the code (grep -rF 'Q:'). Also, I don’t know how much of the code is unused, but some of it may be useful later (though I can remove it of course).

---

TODO

• [x] check some more Sway commits, e.g. 8529141, caed15a
• [x] sort out the Q:uestions
• [x] finish TODOs
• [x] test that it works correctly
  • [x] also on moving things around
  • [x] make sure I understand how it’s supposed to work
• fix issues
  • [x] librewolf almost frozen
  • [x] submenues mispositioned
  • [ ] chromium [title]s not unconstrained? WONTFIX (in this PR), they are subsurfaces and if they can be fixed, it would likely require messing with wlr_surface_send_enter (or something like that) which should be called for more than just them it’s just weird
• [ ] also do something with layer shell? (or keep it for later?)
• [x] check edge cases, esp. invalid view/parent pointer
• [x] add comments where something wasn’t clear
• [x] squash into a single commit
  • [x] remove the CI fix
  • [x] credit Sway for about half of the code

[ghost]

Damn, GH detected even part-fix as a keyword. Well, just letting you know that if you want to keep #38 open until it’s completely fixed, you should unlink it.

[ghost]

TODO: handle moving everything -> unconstrain / send_enter again.

[buffet]

Thanks for all this work!

[buffet]

Oh, also how does this behave when the view is moved (from lua)?

And is there any specific app you're using to test this?

[ghost]

If we set child->map when the owner/root view gets mapped/unmapped, we incorrectly restore that when it's mapped again.

(see the other thread on this)

Same with sub-subsurfaces, if those get unmapped we don't unmap the child, it would make more sense if we crawl up the tree and check if they're all mapped, similar to how sway does it.

I’m not sure I understand you here. But if it’s just for consistency with view unmapping, the message of the Sway commit I linked above can maybe explain why it isn’t needed.

Thanks for all this work!

I must admit (again and again) that I couldn’t have done it without Sway, because I wouldn’t know what and where to do. And because I’m miserably bad at >100loc ‘atomic’ changes :P

Oh, also how does this behave when the view is moved (from lua)?

It doesn’t do anything AFAICT. See the TODO above ;)

And is there any specific app you're using to test this?

I don’t know of one that would be guaranteed to behave correctly, but I haven’t done any research (one more TODO, yay). For now Sublime Merge (coz its button titles were what I saw misbehave, although it’s partly due to SM too) and Vivaldi (that was useful just to check the damage tracking, coz I can show the menu without anything else changing).

PS: I’ll keep adding commits until it’s ready for merge and stash them at the last moment. It’s a bit more convenient for me, except I have to make sure you know this so you don’t merge an ugly commit history.

[ghost]

Added a TODO at the top. The draft PR state is just for consistency with the checklist.

[buffet]

I’m not sure I understand you here.

I meant it would behave weirdly if we would also listen to the view's map, and completely overwrite the child's state.

Related:
If a child is mapped, and the corresponding view gets unmapped and mapped again, is the child's map event fired again, or is the assumption that it stays unmapped correct (which would make sense intuitively)?

[ghost]

Sorry if it doesn’t make sense. I still do believe that wlroots handle all parent (un)maps, but I can’t find the proof. This is an approximate reflection of my thoughts (only being added to as it’s becoming clearer to me, I didn’t remove anything) before I’ll be sure.

I think it’s all done by wlroots, Sway also only cares about the child’s own map/unmap and the view’s unmap (just because of the use-after-free). Though I’m still stuck with subsurface_consider_map and subsurface_unmap – that is, I only see it bubbling through the subsurfaces, but not the initial trigger on view map (only on subsurface commit). (Are all surfaces expected to commit even if they were unmapped by their parent when they want to map again? Wlroots commit on all synchronized child surfaces automatically.)

So the question is left to desynchronized subsurfaces only.

---

This is maybe just a link dump for myself, but could be useful

descendant popups are destroyed on unmap https://github.com/swaywm/wlroots/blob/46c42e55c6e0b08d9e7db989d6a10f073525e999/types/xdg_shell/wlr_xdg_surface.c#L33..L36

TODO: check that the unmap damaging cannot use-after-free too https://github.com/swaywm/wlroots/blob/46c42e55c6e0b08d9e7db989d6a10f073525e999/types/wlr_surface.c#L1136..L1140

wlroots commit on all synchronized child surfaces when their parent commits, but there’s nothing about desynchronized ones https://github.com/swaywm/wlroots/blob/46c42e55c6e0b08d9e7db989d6a10f073525e999/types/wlr_surface.c#L579..L584

[heavyrain266]

Sorry if it doesn’t make sense. I still do believe that wlroots handle all parent (un)maps, but I can’t find the proof.

Nope, you have to map, unmap, commit and uncommit stuff yourself everytime and actually doesn't matter if it's wlroots or libwayland, wlrotos isn't that much high level to do stuff like that for yourself.

I think it’s all done by wlroots, Sway also only cares about the child’s own map/unmap and the view’s unmap (just because of the use-after-free). Though I’m still stuck with subsurface_consider_map and subsurface_unmap – that is, I only see it bubbling through the subsurfaces, but not the initial trigger on view map (only on subsurface commit). (Are all surfaces expected to commit even if they were unmapped by their parent when they want to map again? Wlroots commit on all synchronized child surfaces automatically.)

Honestly, from my experience Sway isn't that good reference, even if it's made by authors of wlroots. They do a lot of stuff in different way than kiwmi or whatever you would like to do it and without studying whole codebase and trying to tralasnate it to your own desires takes too much time imo, from my expiernce I was always looking into something like hikari, taiwins and river (requires some Zig knowledge) and other newer and relativery smaller compositors. (I'm using libwayland anyways so for me everything is strange and too high level, I might wrong so don't mind that)

I'd tried implementing 2-3 protocols based on sway and it was pretty hard to get into, it was idle inhibitor (for idle management), xdg-activation and something else also ralatively small and couldn't even get it to work by looking at sway's code. So will read more kiwmi code and do everything myself.

[ghost]

Nope, you have to map, unmap, commit and uncommit stuff yourself everytime and actually doesn't matter if it's wlroots or libwayland, wlrotos isn't that much high level to do stuff like that for yourself.

The last link shows that I only need about mapping desynchronized subsurfaces IIUC. If you can deduct something different from it, let me know 😉

Honestly, from my experience Sway isn't that good reference, even if it's made by authors of wlroots. They do a lot of stuff in different way than kiwmi or whatever you would like to do it and without studying whole codebase and trying to tralasnate it to your own desires takes too much time imo, from my expiernce I was always looking into something like hikari, taiwins and river (requires some Zig knowledge) and other newer and relativery smaller compositors.

This seemed relatively easy to pick from Sway. Anyway, thanks for the suggestions, I’ll have a look at them next time I’m lost 👍

[ghost]

The behaviour after 7476079c51c91bb8f720e3e932e71e1d8a823362 is different from most compositors I’ve tried (weston, sway, hikari, river, taiwins; but wayfire behaves the same; unfortunately I have no other compositors currently). But I believe it is correct, so it should be okay? (Then it would be time for me to fix those compositors.)

I currently use this for testing (sorry, the code is not nice to read). Build intructions are inside. Expected behaviour: 1 sec black + red subsurfaces, 1 sec none, 1 sec black + red, 1 sec black + orange, repeat.

[ghost]

FIXME: librewolf mostly frozen (not a problem with 9d2c3adb6e52ab05d28a99d2c3e1d60b09458c1b, likely caused by 7476079c51c91bb8f720e3e932e71e1d8a823362)

Edit: interestingly, it is 4afa6507326660682ad9bbf2bbc621dac2d9a153 what introduced this misbehaviour.

[heavyrain266]

Subsurfaces, popups and xwayland are probably most annoying part of compositor...

[buffet]

The behaviour after 7476079 is different from most compositors I’ve tried (weston, sway, hikari, river, taiwins; but wayfire behaves the same; unfortunately I have no other compositors currently). But I believe it is correct, so it should be okay? (Then it would be time for me to fix those compositors.)

I currently use this for testing (sorry, the code is not nice to read). Build intructions are inside. Expected behaviour: 1 sec black + red subsurfaces, 1 sec none, 1 sec black + red, 1 sec black + orange, repeat.

How exactly is it different?

[ghost]

The behaviour after 7476079 is different from most compositors I’ve tried (weston, sway, hikari, river, taiwins; but wayfire behaves the same; unfortunately I have no other compositors currently). But I believe it is correct, so it should be okay? (Then it would be time for me to fix those compositors.) I currently use this for testing (sorry, the code is not nice to read). Build intructions are inside. Expected behaviour: 1 sec black + red subsurfaces, 1 sec none, 1 sec black + red, 1 sec black + orange, repeat.

How exactly is it different?

The other compositors ignore that the black subsurface (which is the parent of the red/orange one) is unmapped and render the child anyway. It becomes: 1 sec black + red subsurfaces, 1 sec red, 1 sec black + red, 1 sec black + orange, repeat.

If the subsurfaces are desynced, sway (not retested others yet) reduces it to 4 secs black + red, repeat.

[ghost]

39bee48e07adb896173b1a0f31b548f4a0ca7b0e:

Intentionally contains misformatted code, because the CI didn't fail on previous commit (even though clang reformatted it now).

…and it still succeeded.

First thing i can think of, maybe the glob doesn’t expand as expected. (Does the CI use a shell for the commands? Does the shell support **? (IIRC it’s zsh-specific))

Edit: see efb4868803dfc39c0e0aaaae17de78c73ae71e3e.

[buffet]

Yeah, it's the **. It works in bash, though the extglob extension has to be enabled. I think a better way to archive this would be using xargs like this find -name .git -prune -o -type f -name '*.[ch]' -print | xargs -d '\n' clang-format -i to correctly deal with files with spaces.

I'll commit that to master, to have it available everywhere. Good job spotting that!

[ghost]

…and something about the checklist. Otherwise (after cleaning up the commit history) I think this is ready for merge.

• test that it works correctly

AFAICT it does, but this is always hard to prove.

• also do something with layer shell? (or keep it for later?)

Since it cannot interact with the mouse yet (which is quite important for testing, innit?), I’d keep it for later.

• add comments where something wasn’t clear

I’m going to check the discussions now, but if you happen to reread the code and find something where a comment would fit, please do let me know.

[buffet]

AFAICT it does, but this is always hard to prove

Seems fine from my testing as well.

layer-shell

Yeah, I think that's good for later, this is already a lot and it doesn't really seem related. Worst case there's stuff to fix.

comments

I'll check again.

[buffet]

Thanks again! By the way, is there any way to communicate with you outside of GitHub issues? Might be useful in the future.

[ghost]

By the way, is there any way to communicate with you outside of GitHub issues?

I usually use e-mail (see my commits), and I have a matrix account (same nick on matrix.org). Other than that, no.