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.

Sign up for GitHub

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

Jump to bottom

Implement bar bindsym #2798

Merged
merged 3 commits into from
Oct 10, 2018
Merged

Implement bar bindsym #2798

merged 3 commits into from
Oct 10, 2018

Conversation

RedSoxFan
Copy link
Member

@RedSoxFan RedSoxFan commented Oct 8, 2018 •
edited by emersion
Loading

Implements bar [<bar-id>] bindsym [--release] button<n> <command>.

Tests:

  • bar bindsym button1 nop should prevent switching to a workspace when left clicking on the workspace buttons
  • The following should prevent switching the workspace when scrolling up and down on the workspace buttons 
    bar bindsym button4 nop
bar bindsym button5 nop
  • bar bindsym --release button1 layout toggle all should
    • (If on a workspace button), switch to the workspace clicked (or the auto_back_and_forth workspace) and change it's layout (this tests makes sure it doesn't override the pressed behaviour)
    • (If not on a workspace button), change the layout of the current workspace

Fixes #1521

ianyfan
ianyfan requested changes Oct 9, 2018
View reviewed changes
sway/commands/bar/bindsym.c Outdated
binding->button = 0;
if (strncasecmp(argv[0], "button", strlen("button")) == 0 &&
strlen(argv[0]) == strlen("button0")) {
binding->button = argv[0][strlen("button")] - '1' + 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not - '0'?

sway/commands/bar/bindsym.c Outdated
strlen(argv[0]) == strlen("button0")) {
binding->button = argv[0][strlen("button")] - '1' + 1;
}
if (binding->button == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should there be a bounds check here? Like > 0 && <= 9? I'm not sure if it's possible to do something funky like buttonA which probably shouldn't be allowed.

sway/commands/bar/bindsym.c
"Unable to allocate bar binding");
}

binding->release = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is technically unnecessary due to calloc always initialising it to false, but I guess it doesn't hurt to be explicit.

sway/config/bar.c Outdated
if (!binding) {
return;
}
if (binding->command) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Check here is unnecessary since freeing a NULL pointer is valid.

sway/config/bar.c Outdated
// TODO: Free mouse bindings
while (bar->bindings->length) {
struct bar_binding *binding = bar->bindings->items[0];
list_del(bar->bindings, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems unnecessary, just iterate over the list and free each item without deleting.

swaybar/config.c Outdated
@@ -74,6 +77,13 @@ void free_config(struct swaybar_config *config) {
free(config->font);
free(config->mode);
free(config->sep_symbol);
while (config->bindings->length) {
struct swaybar_binding *binding = config->bindings->items[0];
list_del(config->bindings, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Make this consistent with the way bar bindings are freed in the other place.

swaybar/bar.c Outdated
bool released = state == WL_POINTER_BUTTON_STATE_RELEASED;
for (int i = 0; i < bar->config->bindings->length; i++) {
struct swaybar_binding *binding = bar->config->bindings->items[i];
wlr_log(WLR_DEBUG, "Checking [%u, %d] against [%u, %d, %s]",
Copy link
Contributor

Choose a reason for hiding this comment

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

Put the command in quotes. Also maybe add the word 'binding' into the message?

Though I'm not sure if I agree with having a debug message here. It might be better to only log if the binding is actually found.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops. I meant to remove that.
I added a debug message for the binding being executed.

swaybar/bar.c Outdated
@@ -180,6 +201,11 @@ static void wl_pointer_axis(void *data, struct wl_pointer *wl_pointer,
return;
}

if (check_bindings(bar, wl_axis_to_x11_button(axis, value),
WL_POINTER_BUTTON_STATE_PRESSED)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should send both a pressed event and released event. (at least, that seems to be the behaviour in i3)

@ianyfan
Copy link
Contributor

ianyfan commented Oct 9, 2018 •
edited
Loading

Closes Ref #1521

ddevault
ddevault approved these changes Oct 9, 2018
View reviewed changes
Copy link
Contributor

@ddevault ddevault left a comment

Choose a reason for hiding this comment

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

Tested it and it works (still need ianyfan's approval though)

@RedSoxFan
Copy link
Member Author

Closes #1521

I don't think it does. I think all that's missing is a bar config update ipc event though. I'll add that real quick

@RedSoxFan
Copy link
Member Author

RedSoxFan commented Oct 9, 2018 •
edited
Loading

Closes #1521

I don't think it does. I think all that's missing is a bar config update ipc event though. I'll add that real quick

Actually swaybar doesn't currently subscribe to barconfig_update so I'd rather separate it from this one

@ianyfan
Copy link
Contributor

ianyfan commented Oct 9, 2018

#2751 adds that, so I guess wait until that is merged.

@ianyfan
Copy link
Contributor

ianyfan commented Oct 9, 2018

Actually, I'm not sure whether that PR is asking for bindings to be added at runtime, it's not very clear. In any case, it's so old that I don't actually think it's a problem to close it, and to open a new PR with more specific request.

@ianyfan
Copy link
Contributor

ianyfan commented Oct 9, 2018

Should we allow a way to not skip default behaviour? Just a suggestion.

@RedSoxFan
Copy link
Member Author

Actually, I'm not sure whether that PR is asking for bindings to be added at runtime, it's not very clear. In any case, it's so old that I don't actually think it's a problem to close it, and to open a new PR with more specific request.

Actually, ya. That doesn't really seem related. That might be better closed by #2751 though

Should we allow a way to not skip default behaviour? Just a suggestion.

Do you mean having both the bindsym and default behaviour get triggered? You could just use --release for bindsyms and leave the default behaviour on pressed. I guess a --before-pressed could be added to have something execute before the default behaviour.

@ddevault ddevault merged commit 0a36d14 into swaywm:master Oct 10, 2018
@ddevault
Copy link
Contributor

Thanks!

@RedSoxFan RedSoxFan deleted the bar-bindsym branch October 10, 2018 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ddevault ddevault ddevault approved these changes

@ianyfan ianyfan ianyfan approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@RedSoxFan @ianyfan @ddevault