-
Notifications
You must be signed in to change notification settings - Fork 361
feat: add mouse scroll action scripting to notificaiton #1489
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
base: master
Are you sure you want to change the base?
feat: add mouse scroll action scripting to notificaiton #1489
Conversation
demo video output.mp4 |
hello. thank you for the contribution. I like the idea however there are some things we might improve. I am busy right now so I will just leave some initial review. also please fix the errors |
dunstrc
Outdated
# These values can be strung together for each mouse event, and | ||
# will be executed in sequence. | ||
mouse_left_click = close_current | ||
mouse_middle_click = do_action, close_current | ||
mouse_right_click = close_all | ||
mouse_scroll_forward = forward_script | ||
mouse_scroll_backward = back_script |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a more standard way to say this is scroll up/down
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay will make those changes tonight and push it, I tried to follow the convention of x11 constant for mouse button, will make those changes
about the error, their seems to be memory leak issue in my code, will fix those issues to, thank you |
i am working on, it will update when I am done, rn now busy with other stuffs |
dont worry. let me know when its ready for review |
@bynect could you rerun the workflow, I think the leak was due the mouse_script_scroll not being freed, I think the last commits solves the issue. Could you verify that and let me know if that fixes |
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #1489 +/- ##
==========================================
- Coverage 64.90% 64.49% -0.42%
==========================================
Files 51 51
Lines 9024 9100 +76
Branches 1048 1065 +17
==========================================
+ Hits 5857 5869 +12
- Misses 3167 3231 +64
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@bynect Do i need to make 100% code coverage too ? or the task here is done for me. |
Sorry right now I can't review |
if (n) { | ||
if (act == MOUSE_CLOSE_CURRENT) { | ||
n->marked_for_closure = REASON_USER; | ||
} else if (act == MOUSE_DO_ACTION) { | ||
notification_do_action(n); | ||
} else if (act == MOUSE_OPEN_URL) { | ||
notification_open_url(n); | ||
} else if (act == MOUSE_SCROLL_SCRIPT){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you please properly indent this? we leave a space between if and (
also spaces between == and commas. but the content seems right
printf("}\n"); | ||
fflush(stdout); | ||
} | ||
|
||
/* see notification.h */ | ||
void notification_run_script(struct notification *n) | ||
{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this newline seems unnecessary
@@ -19,6 +18,12 @@ enum icon_position { | |||
ICON_OFF | |||
}; | |||
|
|||
enum mouse_action_type{ | |||
MOUSE_ACTION_OTHERS=0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
=0 is unnecessary
@@ -86,8 +91,17 @@ struct notification { | |||
|
|||
enum markup_mode markup; | |||
char *format; | |||
|
|||
enum mouse_action_type script_choice; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rather than having this choice variable inside notification I would rather add mouse_action parameter to run_script function. this seems a bit more clean and you don't risk forgetting to set the type of action beforehand
@@ -26,6 +26,8 @@ | |||
#define BTN_RIGHT (0x111) | |||
#define BTN_MIDDLE (0x112) | |||
#define BTN_TOUCH (0x14a) | |||
#define BTN_FORWARD (0x115) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe is better to change this as well to SCROLL_UP / SCROLL_DOWN ?
because there are the lateral mouse buttons called forward and backward and it could cause confusion eventually?
{ | ||
.name = "mouse_scroll_down", | ||
.section = "global", | ||
.description = "Action of right click event", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please update the description. also of scroll_up
|
||
const char *script = n->scripts[i]; | ||
const char *script; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i am not sure about this behaviour. script specified with always_run_script should run even if you use a button I think.
is there a reason for this structure of the code?
I left a review. mostly it is formatting. but there are some architecture changes I suggest. let me know what do you think |
okay I will make those changes and let you know, thank you for the time to review the code |
Adds mouse scroll action scripting to the notification,
how to use it
mouse_scroll_forward = forward_script
as andmouse_scroll_backward = back_script
[optional as it's by default]dunstrc
;[note:- script_mouse_forward/back takes single word script to run, you cannot put long script their, make a file of the script you want to execute, and give it a simple single word name and add that to path, do not forget to add executable flag
chmod +x my_script
]dunstify "changevolume" "volume=10%"
why we need this
TODOS:
Closes #1490