sway/ipc-json: don't get decoration box for containers in scratchpad

[bl4ckb0ne]

Fixes #7303

[emersion]

Regressed by a5a44ba

cc @baltitenger

[emersion]

I think we should still populate the deco rect for scratchpad windows.

[bl4ckb0ne]

I wasnt able to reproduce the issue from #7303, do the scratchpad containers have a geometry?

[emersion]

Yes. In other words this patch introduces a regression: deco_rect was sent before, but isn't anymore.

[rpigott]

Is #7303 not a dupe of #7123? Either way testing con->scratchpad here is not correct.

A scratchpad container has con->scratchpad == true

A scratchpad hidden container has con->scratchpad == true && !con->pending.workspace.

A scratchpad hidden split container child (parent is scratchpad hidden) has !con->pending.workspace, but only the top container in the tree has scratchpad == true.

#7126 added a !con->pending.workspace check but maybe it is better placed in this function.

We should keep the deco_rect field regardless, but its fine to return zeros for hidden containers imo.

[bl4ckb0ne]

@rpigott & @emersion thanks for the feedback, i made some changes to the patch.

[emersion]

Thanks, looks good now!

[rpigott]

Scratchpad hidden split containers do have children, so those children have siblings, and I'm not sure its a good idea to say they don't. E.g. in sway it is possible to move a container into a scratchpad hidden parent as long as you can match it with criteria. It might be reasonable to refuse these actions, but we shouldn't crash on them:

move to child of scratchpad hidden split container w/ this patch

#0  0x00005555555d79dc in list_find (list=0x0, item=0x555555ba8100) at ../common/list.c:71
#1  0x00005555555c796e in container_add_sibling (fixed=0x555555ba8100, active=0x555555c61770, after=true) at ../sway/tree/container.c:1469
#2  0x00005555555ae85f in container_move_to_container (container=0x555555c61770, destination=0x555555ba8100) at ../sway/commands/move.c:250
#3  0x00005555555af733 in cmd_move_container (no_auto_back_and_forth=false, argc=2, argv=0x55555587ea48) at ../sway/commands/move.c:568
#4  0x00005555555b0dc1 in cmd_move (argc=2, argv=0x55555587ea48) at ../sway/commands/move.c:1045
#5  0x000055555556c3a6 in execute_command (_exec=0x555555c4fa20 "move container to mark test", seat=0x5555558a48e0, con=0x0) at ../sway/commands.c:294
#6  0x0000555555578f13 in ipc_client_handle_command (client=0x555555c63820, payload_length=27, payload_type=IPC_COMMAND) at ../sway/ipc-server.c:638
#7  0x0000555555577b16 in ipc_client_handle_readable (client_fd=55, mask=1, data=0x555555c63820) at ../sway/ipc-server.c:263
#8  0x00007ffff78349e2 in wl_event_loop_dispatch () at /usr/lib/libwayland-server.so.0
#9  0x00007ffff7835197 in wl_display_run () at /usr/lib/libwayland-server.so.0
#10 0x000055555557ca9f in server_run (server=0x5555555fcb60 <server>) at ../sway/server.c:325
#11 0x000055555557bad8 in main (argc=3, argv=0x7fffffffde78) at ../sway/main.c:415

^ caller gets confused because a container with a parent has no siblings.

The problem in #7123 is that we can't really rely on the invariant !con->pending.workspace <=> "container is scratchpad hidden or child" in most places we would like because the layout code breaks this property by detaching containers all the time. The fix is to just check what we mean, !con->pending.workspace instead of container_is_scratchpad_hidden in container_get_siblings so it's safe and correct to use everywhere, which is the effect of #7126.

Callers will still need to check the return value of this function if they didn't previously guarantee it isn't called on detached containers. Some existing calls already guard against this directly:

sway/sway/tree/container.c

Lines 1565 to 1566 in c32a507

	if (child->pending.parent || child->pending.workspace) {
	list_t *siblings = container_get_siblings(child);

But I think prior to #7113 all callers of container_get_siblings adhered to this unspoken rule in one way or another. I wouldn't be at all surprised if there were more similar bugs. E.g. just below in *describe_node using the parent layout effectively performs the same check:

sway/sway/ipc-json.c

Lines 775 to 776 in c32a507

	if (container_parent_layout(node->sway_container) == L_STACKED) {
	count = container_get_siblings(node->sway_container)->length;

The scratchpad is special cased in a lot of places so this pitfall may have just been accidentally avoided. There is no point in keeping this footgun around so #7126 is a good change, except the container_is_scratchpad_hidden check is now redundant.

I'm guessing #7303 was already fixed by #7126 as I am not able to reproduce it on master. I think the current code in master is correct, except for the redundant check just mentioned, and that get_deco_rect should probably immediately return 0's when parent_layout == L_NONE instead of calling container_get_siblings and crashing (or waiting around to see that it is NULL on master):

sway/sway/ipc-json.c

Lines 529 to 531 in c32a507

	static void get_deco_rect(struct sway_container *c, struct wlr_box *deco_rect) {
	enum sway_container_layout parent_layout = container_parent_layout(c);
	list_t *siblings = container_get_siblings(c);

Yes this is all pretty annoying and ugly and could use a rework.

Stale

[rpigott]

Returning NULL for children of scratchpad hidden split containers may crash in certain operations. See comment.

[emersion]

I believe per the comment above this PR is not needed.