Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set the current seat in seatop handlers #5950

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

rpigott
Copy link
Member

@rpigott rpigott commented Jan 13, 2021

Fixes #5939, see comment.

The functions in container.c all like to reference the current seat, so we better make sure it is set before we do anything that may deal with focus. Currently, when the seatop_ commands run, the handler_context.seat can be anything, even NULL, so I've chosen to set it in all the seatop_begin* functions. I don't suppose it matters for some, like floating_resize, but I set it in all of them anyway for symmetry.

container_fullscreen_disable(con2);
}

struct sway_seat *seat = input_manager_current_seat();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only line changed in this commit. Otherwise these three functions were lifted directly from commands/swap.c

@@ -1423,7 +1423,7 @@ struct sway_container *container_split(struct sway_container *child,
}
}

struct sway_seat *seat = input_manager_get_default_seat();
struct sway_seat *seat = input_manager_current_seat();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why container_split should use the default seat while the reset use the current seat, so I've changed it.

@emersion
Copy link
Member

config->handler_context is mostly meant for command handlers, not for tree manipulation functions. Can we instead pass the seat via an argument to container_swap?

@rpigott
Copy link
Member Author

rpigott commented Jan 15, 2021

config->handler_context is mostly meant for command handlers, not for tree manipulation functions. Can we instead pass the seat via an argument to container_swap?

Technically yes, but I think this way is better. My reasoning is as follows:

The container* functions need a reference to a seat whenever they may mess with the focus, but none accept a seat argument. They all get a seat reference currently with input_manager_current_seat() with the exception of container_split, which I believe is an error, hence the 3rd commit.

The implementation of input_manager_current_seat is:

struct sway_seat *input_manager_current_seat(void) {
	struct sway_seat *seat = config->handler_context.seat;
	if (!seat) {
		seat = input_manager_get_default_seat();
	}
	return seat;
}

So they do already reference the handler_context.seat to use the correct seat when invoked from the command handlers.

Well the seatop actions are just like commands, and setting handler_context.seat in seatop_begin* is analogous to setting it in execute_command. This change basically just treats the seatop actions like the commands that they are. If anything I think it should go further and set the handler_context node/container etc.

@rpigott
Copy link
Member Author

rpigott commented Jan 16, 2021

Actually I suppose it would be better to analogize each tiling dropzone with a command, rather than the whole tiling_move operation. In that case seatop_begin* is the wrong place to set handler_context.seat, it should be when the action ends before we start modifying the tree. Similarly the other seatops should only set it when they actually do something.

Ronan Pigott added 3 commits January 16, 2021 12:23
We may also swap containers from the mouse actions.
Ensures that the container operations which result from cursor
actions reference the right seat
@emersion
Copy link
Member

I'm not a fan of setting global state like this. We did it because it was the less intrusive way to do it for commands, but I'd prefer not to make more stuff rely on it.

@rpigott rpigott marked this pull request as draft March 17, 2021 23:16
@rpigott
Copy link
Member Author

rpigott commented Mar 17, 2021

I generally agree, and I think there are still bugs in the PR as written. I'll need to think about this one a bit, so I've converted to a draft for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Switching windows places after a reload kills Sway
2 participants