Add options to specify mouse event #530
Conversation
There was a problem hiding this comment.
I found some small nitpicks but other than that looks good, thanks for working on this!
(For reference relevant issue: #246)
EDIT: Can you also update the manpage with the new settings?
dunstrc
Outdated
| # * do_action: If the notification has exactly one action, or one is marked as default, | ||
| # invoke it. If there are multiple and no default, open the context menu. | ||
| # * close_current: Close current notification. | ||
| # * push_all: Push all waiting and displayed notifications to history. |
There was a problem hiding this comment.
close_all would be a more consistent name for this IMO.
dunstrc
Outdated
| # invoke it. If there are multiple and no default, open the context menu. | ||
| # * close_current: Close current notification. | ||
| # * push_all: Push all waiting and displayed notifications to history. | ||
| [mouse] |
There was a problem hiding this comment.
I'd prefer it if this was in the global section as well as mouse_left_click, mouse_middle_click etc. I think sections are best reserved for rules/more customization down the line rather than setting groups.
src/settings.h
Outdated
| enum separator_color { SEP_FOREGROUND, SEP_AUTO, SEP_FRAME, SEP_CUSTOM }; | ||
| enum follow_mode { FOLLOW_NONE, FOLLOW_MOUSE, FOLLOW_KEYBOARD }; | ||
| enum markup_mode { MARKUP_NULL, MARKUP_NO, MARKUP_STRIP, MARKUP_FULL }; | ||
| enum mouse_action { do_action, close_current, push_all }; |
There was a problem hiding this comment.
Please match the enum value style as above, all caps and prefixed with a relevant string e.g. MOUSE_DO_ACTION
src/x11/x.c
Outdated
| break; | ||
| } | ||
|
|
||
| if (act == push_all) { |
There was a problem hiding this comment.
There's some undefined behaviour here since act is uninitialized by default - are there any other mouse button events? Not sure, but it's best to handle it in any case. (probably ignore the event in the default case)
src/settings.c
Outdated
| { | ||
| char *c = option_get_string( | ||
| "mouse", | ||
| "middle_click", "-middel_click", "do_action", |
src/settings.c
Outdated
| else if (strcmp(c, "push_all") == 0) | ||
| settings.right_click = push_all; | ||
| else { | ||
| LOG_W("Unknown right_click position: '%s'", c); |
There was a problem hiding this comment.
Error message should be Unknown right_click action and similarly for the other errors.
src/settings.c
Outdated
| ); | ||
|
|
||
| if (strlen(c) > 0) { | ||
| if (strcmp(c, "do_action") == 0) |
There was a problem hiding this comment.
This is a lot of copy-paste code, it'll be easier to split this into a function (see the parse functions at the top of the file as an example)
* Move mouse_left/middle/right_click to global section * Match the enum value style * Ignore unknow mouse event * Split copy-paste code into a function * Fix typo
|
Thank you for correcting me. I just uploaded my new code. |
bebehei
left a comment
bebehei
left a comment
There was a problem hiding this comment.
The code looks mergeable.Thank you for implementing this! And also thanks to @tsipinakis giving already a very good review!
src/settings.c
Outdated
| { | ||
| char *c = option_get_string( | ||
| "global", | ||
| "mouse_left_click", "-left_click", "close_current", |
There was a problem hiding this comment.
I guess, the only thing, which is still against current consistency: Please put the default value in the defaults struct in the file config.h. There is a full settings_t allocated and you can specify the default value there.
Use
char *c = option_get_string(
"global",
"mouse_left_click", "-left_click", NULL,
And then do it similarly to markup_mode:
Lines 160 to 167 in 6d0e20e
|
Done~ |
|
Thanks again, merged! |
|
when are you gonna release this? |
|
@ForsakenHarmony We'll release this with 1.4.0. The milestone is linked here: https://github.com/dunst-project/dunst/milestone/6 |
4 participants
Edit (@bebehei): Fixes #246