Skip to content

Allow for multiple actions per mouse event#705

Merged
tsipinakis merged 5 commits intodunst-project:masterfrom
mkrasnitski:list-conf-options
Apr 6, 2020
Merged

Allow for multiple actions per mouse event#705
tsipinakis merged 5 commits intodunst-project:masterfrom
mkrasnitski:list-conf-options

Conversation

@mkrasnitski
Copy link
Contributor

This PR introduces functions for parsing configuration options provided in dunstrc or on the command line as comma-separated lists of one or more values. Elements are separated by single commas, and trailing whitespace around commas is trimmed, although whitespace in the middle of elements is preserved. Additionally, it makes use of this new parsing by changing the mouse_left_click, mouse_middle_click, and mouse_right_click options to be lists instead of just strings. Current config files are not broken by this. Some tests are provided, though any suggestions or improvements to better the style and code coverage of said tests is appreciated.

Implements #690.

The user provides a comma-separated list of valid mouse actions
that will be performed one after another when a notification is
clicked. If any one of the provided actions is invalid, the value
reverts to its default state.
@codecov-io
Copy link

codecov-io commented Apr 5, 2020

Codecov Report

Merging #705 into master will increase coverage by 0.77%.
The diff coverage is 81.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #705      +/-   ##
==========================================
+ Coverage   66.40%   67.17%   +0.77%     
==========================================
  Files          29       29              
  Lines        4899     5006     +107     
==========================================
+ Hits         3253     3363     +110     
+ Misses       1646     1643       -3     
Flag Coverage Δ
#unittests 67.17% <81.25%> (+0.77%) ⬆️
Impacted Files Coverage Δ
src/x11/x.c 2.33% <0.00%> (-0.02%) ⬇️
src/option_parser.c 86.66% <90.62%> (+0.38%) ⬆️
src/settings.c 57.89% <100.00%> (+1.27%) ⬆️
src/utils.c 91.59% <100.00%> (+0.94%) ⬆️
test/option_parser.c 98.25% <100.00%> (+0.53%) ⬆️
test/dbus.c 99.11% <0.00%> (+0.44%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0d03802...92a2148. Read the comment docs.

@mkrasnitski
Copy link
Contributor Author

I would also like suggestions on changing the description of these options in the default dunstrc. Would it be prudent to make one of the options a multi-element list by default, or should the comment describing them be edited?

bebehei
bebehei previously requested changes Apr 5, 2020
@bebehei
Copy link
Member

bebehei commented Apr 5, 2020

But anyways, all in all the code looks nice and polished. I've seldomly seen a PR written with perfect style! Keep up the good work 👍

Lesson: don't reinvent the wheel.
Additionally, changed a `g_malloc` call to `g_malloc_n` for safety.
@mkrasnitski
Copy link
Contributor Author

Thanks for the encouragement! Fixes should be up. I decided to keep string_to_array as a wrapper function, since g_strsplit doesn't trim whitespace from tokens.

These make it more clear to the user that the mouse_click
options can be defined as lists.
@tsipinakis tsipinakis linked an issue Apr 6, 2020 that may be closed by this pull request
Copy link
Member

@tsipinakis tsipinakis left a comment

Choose a reason for hiding this comment

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

As @bebehei said, it's rare to see a perfect PR from the get-go. Keep up the good work!

@tsipinakis tsipinakis merged commit b4633a0 into dunst-project:master Apr 6, 2020
@tsipinakis
Copy link
Member

Would it be prudent to make one of the options a multi-element list by default, or should the comment describing them be edited?

I think that's fine as you've put it. In my case all programs I've come across close the notification by themselves when an action is triggered, so I'd consider this the 'default behavour'.

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.

Allow for multiple actions per mouse event

4 participants