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

Add output selection #60

Closed
wants to merge 1 commit into from
Closed

Conversation

vesim987
Copy link
Contributor

Hi, I added new mode to slurp for output selection. I had to update xdg-output-unstable-v1 to version 2, and also _POSIX_C_SOURCE to 200809 for strdup.

Right now it is setting the output name for both, point and output selection, the only difference is that in output selection it is setting the output geometry as the selection box.

@@ -613,7 +635,7 @@ static void handle_global(void *data, struct wl_registry *registry,
create_output(state, wl_output);
} else if (strcmp(interface, zxdg_output_manager_v1_interface.name) == 0) {
state->xdg_output_manager = wl_registry_bind(registry, name,
&zxdg_output_manager_v1_interface, 1);
&zxdg_output_manager_v1_interface, 2);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This may break if the compositor only supports version 1. If that is a concern, I think MIN(2, version) would be better.

Copy link
Contributor Author

@vesim987 vesim987 Aug 17, 2020

Choose a reason for hiding this comment

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

I think the -o feature w/o outputs names is kinda useless. But that is a question to @emersion, if we want to keep compatibility with version 1.

Copy link
Owner

Choose a reason for hiding this comment

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

I think it's fine to bump the minimum required version to 2.

@@ -708,7 +738,7 @@ int main(int argc, char *argv[]) {

int opt;
char *format = "%x,%y %wx%h";
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the -o option is used, should a different default format be used that includes the output name (or maybe is only the output name)?

break;
}
}
state->result.output = output;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it make sense to set this for a rectangular selection as well? You do run into the problem of what to do if there are multiple outputs, but the simplest think would be just to pick a point, like the top left, or center of the rectangle, and use that to pick the "best" output.

Choose a reason for hiding this comment

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

Slurp can take a predefined set of rectangles as input and limit possible selections to those rectangles (you can see this if you use the output selection example in the README).

I think the correct approach is to implement it this way, where there's no option to select anything other than the predefined rectangles which occupy each output entirely, and chosen by the rectangle the mouse is inside of.

Choose a reason for hiding this comment

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

Looks like you can just use add_choice_box like is done here https://github.com/emersion/slurp/blob/master/main.c#L761

Copy link
Collaborator

Choose a reason for hiding this comment

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

limit possible selections to those rectangles

That isn't quite right. At least from me trying it out in slurp 1.2.0, it seems it just creates predefined boxes you can select with a click, but it doesn't stop you from dragging your own box.

Choose a reason for hiding this comment

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

Hmmm, that sounds like a bug in the intended behavior, but @emersion would have to weigh in.

Copy link
Owner

Choose a reason for hiding this comment

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

No, this is intentional: the input boxes are just hints, the user can ignore the hint. I'd be fine with adding an option to prevent that.

Choose a reason for hiding this comment

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

Glad I asked.

@emersion
Copy link
Owner

emersion commented Aug 17, 2020

As @tmccombs said, I'd prefer this to be done with the format string. We could add %o, which prints the name of the output the selected box is in.

Then we don't even need a -o flag at all: selecting an output could be done with slurp -p -f "%o".

@tmccombs tmccombs mentioned this pull request Aug 23, 2020
tmccombs added a commit to tmccombs/slurp that referenced this pull request Aug 24, 2020
@tmccombs tmccombs mentioned this pull request Aug 24, 2020
tmccombs added a commit to tmccombs/slurp that referenced this pull request Aug 24, 2020
emersion pushed a commit to tmccombs/slurp that referenced this pull request Sep 4, 2020
emersion pushed a commit to tmccombs/slurp that referenced this pull request Sep 7, 2020
emersion pushed a commit that referenced this pull request Sep 7, 2020
@emersion
Copy link
Owner

emersion commented Sep 7, 2020

Superseded by #64

@emersion emersion closed this Sep 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants