add --ignore-buttons option #41
Conversation
include/types.h
Outdated
| @@ -6,11 +6,17 @@ typedef struct match_t { | |||
| int len; | |||
| } match_t; | |||
|
|
|||
| typedef struct which_buttons_ignore_t { | |||
There was a problem hiding this comment.
Let's name this ignore_buttons_t
include/types.h
Outdated
| @@ -6,11 +6,17 @@ typedef struct match_t { | |||
| int len; | |||
| } match_t; | |||
|
|
|||
| typedef struct which_buttons_ignore_t { | |||
| unsigned char how_much_symbols; | |||
There was a problem hiding this comment.
This should be "how many", but I'd actually like to just name this something like "count" or "length", that's simpler and clear enough.
include/types.h
Outdated
| @@ -6,11 +6,17 @@ typedef struct match_t { | |||
| int len; | |||
| } match_t; | |||
|
|
|||
| typedef struct which_buttons_ignore_t { | |||
| unsigned char how_much_symbols; | |||
| unsigned char *symbols; | |||
There was a problem hiding this comment.
This should be unsigned int * and maybe rename to buttons.
man/unclutter-xfixes.man
Outdated
| @@ -32,6 +32,11 @@ rather just the desktop background. | |||
| Ignore mouse scroll events (buttons 4 and 5) so that scrolling doesn't unhide | |||
| the cursor. | |||
|
|
|||
| *--ignore-buttons*:: | |||
| Ignore mouse buttons clicks so that clicks doesn't unhide the cursor. Buttons | |||
There was a problem hiding this comment.
Let's rephrase this a bit:
Defines the mouse buttons which do not unhide the cursor when clicked. You can pass multiple button numbers by separating them with ','.
The rest should be removed. I don't think we need to add the complexity of supporting ranges, enumerating the buttons is easy enough to be acceptable. It's not like there are 200 buttons.
Maybe we can also add this to --ignore-buttons:
This is a shortcut for --ignore-buttons 4,5.
src/event.c
Outdated
| } | ||
| } | ||
|
|
||
| if (config.ignore_buttons.how_much_symbols |
There was a problem hiding this comment.
There's a lot of duplication going on here. We should refactor this a bit to something like this:
static boolean is_button_ignored(const XIRawEvent *data) {
if (config.ignore_scrolling && (data->detail == 4 || data->detail == 5)) {
return true;
}
for (int i = 0; i < config.ignore_buttons.count; i++) {
if (data -> detail == config.ignore_buttons.buttons[i]) {
return true;
}
}
return false;
}
// …
if (cookie->evtype == XI_RawButtonPress) {
const XIRawEvent *data = (const XIRawEvent *) cookie->data;
if (is_button_ignored(data)) {
XFreeEventData(display, cookie);
continue;
}
}
|
Oh, and please do drop a comment when you updated the PR so I get notified. Thanks! |
|
Updated |
There was a problem hiding this comment.
It's looking mostly good now, thanks! I'd still like to change a few things around a bit, but we should be able to merge this afterwards. Thank you, and of course feel free to let me know if you disagree with something (I'm not actually extremely experienced in C).
src/util.c
Outdated
| @@ -17,3 +17,36 @@ long parse_int(char *str) { | |||
|
|
|||
| return parsed; | |||
| } | |||
|
|
|||
| unsigned long parse_buttons_numbers(char *str, | |||
| ignore_buttons_t *ignore_buttons) { | |||
There was a problem hiding this comment.
The formatting here is weird with no indentation at all. Can we just move this onto the previous line?
src/util.c
Outdated
| unsigned long parse_buttons_numbers(char *str, | ||
| ignore_buttons_t *ignore_buttons) { | ||
| ignore_buttons->buttons = (unsigned int *)calloc(10, sizeof(unsigned int)); | ||
| // no check for "more then UCHAR_MAX" |
There was a problem hiding this comment.
(nit: then -> than)
But actually, I think we should just remove this "batching" logic for the allocated size and reallocate as we go along. We usually only expect a few entries here, we never expect many, so having to do 2–3 reallocs is totally fine since it only happens on startup anyway and reduces the complexity of this function.
src/util.c
Outdated
| max_symbols * sizeof(unsigned int)); | ||
| if (p == NULL) { | ||
| free(ignore_buttons->buttons); | ||
| bail("Cannot reallocate memory for ignore-buttons\n"); |
There was a problem hiding this comment.
nit: The \n isn't needed here, bail calls ELOG which already adds this.
src/util.c
Outdated
| // realloc | ||
| if (ignore_buttons->count >= max_symbols - 1) { | ||
| max_symbols += max_symbols; | ||
| unsigned int *p = (unsigned int *)realloc(ignore_buttons->buttons, |
There was a problem hiding this comment.
nit: can we name this buttons, since that's what it is?
src/util.c
Outdated
| // no check for "more then UCHAR_MAX" | ||
| unsigned char max_symbols = 10; | ||
|
|
||
| char *strtok_value = strtok(str, ","); |
src/unclutter.c
Outdated
| config.ignore_buttons.count = 0; | ||
| } else { | ||
| // shrink array size | ||
| config.ignore_buttons.buttons = (unsigned int *)realloc( |
There was a problem hiding this comment.
This will become unnecessary with my comments below about reallocating as we go.
src/util.c
Outdated
| }; | ||
|
|
||
| if (!ignore_buttons->count) { | ||
| return ULONG_MAX; |
There was a problem hiding this comment.
I think this should never realistically happen, so I would bail here as well. Then we can also remove the ULONG_MAX returns and instead assert(false) since it will be a dead code path, right? The caller won't need to handle this since we'll always be bailing if something goes wrong.
What is the meaning of this word? |
|
"nit" stands for nitpick, so a very tiny detail like a typo or the like. |
|
Thanks! |
fix #40