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.

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

Settings refactor V2 #803

Merged
merged 38 commits into from
Jul 24, 2021
Merged

Conversation

fwsmit
Copy link
Member

@fwsmit fwsmit commented Jan 7, 2021

Our current way of dealing with settings is very messy. It also involves copy and pasting a considerate amount of code for adding a setting. This PR strives to make it so that you only need to add a few lines to an array, allowed_settings, to add a setting or rule.
This allows us to do much better checking of the settings both in the way settings are defined and the user's input.
If you make a mistake in defining a setting, this will most likely be caught by the tests, or you can easily add a test to account for this mistake. We can also easily extend the set of allowed rules without making the code more messy.
If a users misspells a setting in their dunstrc, instead of ignoring that setting dunst will warn the user that setting doesn't exist. This could also be extended to suggesting a similar setting from the list of settings.

Changes in this PR

  • refactor the settings code
  • refactor the rules code
  • refactor the command line parsing
  • remove all the now unused functions: string_parse_*, ini_get_*, option_get_* (cmd_line_get_* stays for now)
  • fix segfault in string_to_color
  • multiple sections with the same name are merged
  • rules don't respect global settings anymore. This makes the settings more consistent. See also:Add fullscreen as a global option #644 (comment)
  • rules can be placed in the global
  • Fixes Add fullscreen as a global option #644, by allowing rules in the global section.
  • warnings are emitted when an unknown option is passed

Deprecated by this PR

TODO:

  • Add warning for deprecated settings, instead of the default 'does not exists'
  • Set the same defaults as before
  • Fix warnings about unused variables in settings_data.h
  • Add some more tests, especially some tests with a test dunstrc

Possible enhancements:

  • Warn when the same setting is set multiple times in the same section

Implements: #412 #414
Fixes: #138

@codecov-io
Copy link

codecov-io commented Jan 7, 2021

Codecov Report

Merging #803 (dcc18ba) into master (3acffdb) will increase coverage by 1.78%.
The diff coverage is 90.02%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #803      +/-   ##
==========================================
+ Coverage   58.54%   60.32%   +1.78%     
==========================================
  Files          37       39       +2     
  Lines        5987     6022      +35     
==========================================
+ Hits         3505     3633     +128     
+ Misses       2482     2389      -93     
Flag Coverage Δ
unittests 60.32% <90.02%> (+1.78%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/draw.c 0.00% <0.00%> (ø)
src/dunst.c 9.82% <0.00%> (-0.37%) ⬇️
src/input.c 0.00% <0.00%> (ø)
src/notification.c 59.53% <0.00%> (-0.16%) ⬇️
src/wayland/wl.c 0.00% <0.00%> (ø)
test/dunst.c 100.00% <ø> (+21.42%) ⬆️
test/notification.c 100.00% <ø> (ø)
src/settings.c 56.52% <77.77%> (-2.01%) ⬇️
src/option_parser.c 83.68% <83.42%> (-2.42%) ⬇️
src/rules.c 78.49% <83.78%> (+29.24%) ⬆️
... and 12 more

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 3acffdb...dcc18ba. Read the comment docs.

Copy link
Member

@bebehei bebehei left a comment

Choose a reason for hiding this comment

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

Nice PR.

You might want to look into #475

I'm not sure about the current state of option parsing in dunst, but I'd prefer to have uniformed string parser functions. This can be used then in further work to restructure the option parser.

src/option_parser.c Outdated Show resolved Hide resolved
src/settings.h Outdated Show resolved Hide resolved
src/option_parser.c Outdated Show resolved Hide resolved
@fwsmit
Copy link
Member Author

fwsmit commented Jan 9, 2021

I'm not sure about the current state of option parsing in dunst, but I'd prefer to have uniformed string parser functions. This can be used then in further work to restructure the option parser.

Do you mean the same thing I mentioned here #803 (comment)? Then I'm totally down.

Nice PR.

You might want to look into #475

That's a good PR too. I wonder why it never got merged. There are quite a few small things in there that make the settings code better. But before we continue this implementation in C I think it's a good time to decide 2 things:

  • Do we want to continue this in C or port the entire settings code over to Rust.
  • I don't see any issues about this in this repo so it might be a bit short notice, but it's said a few times that we've outgrown the ini configuration format. Do we want to think about moving to a new configuration format entirely?

@fwsmit fwsmit force-pushed the settings-refactor branch 4 times, most recently from 5775ed7 to 6153c02 Compare January 29, 2021 20:40
@fwsmit
Copy link
Member Author

fwsmit commented Feb 7, 2021

This PR is almost ready, but I don't know what makes the last tests fail. A second pair of eyes would help a lot.
Restoring settings.c and config.h to the state from before the PR makes more dbus tests fail.
@bebehei could you take a look, since you made the dbus tests.
Edit: nvm I found the issue. It was a bad test that changed the memory for the test after it. Only some leaks to deal with now

@fwsmit fwsmit changed the title Proof of concept: Settings refactor Settings refactor V2 Feb 8, 2021
@fwsmit fwsmit force-pushed the settings-refactor branch 2 times, most recently from 738a100 to 4ae7c22 Compare February 9, 2021 21:39
@fwsmit fwsmit marked this pull request as ready for review March 1, 2021 21:31
@fwsmit
Copy link
Member Author

fwsmit commented Mar 1, 2021

This PR is finally in a reviewable state. I do not expect everything to work perfectly yet. There's probably some things that have changed from the old version. I will go through and test a bit more. The code is mostly ready, so you can go ahead and review it.

This is implemented by giving each setting a type. Based on the type,
the string will be parsed differently.
A generic enum parser is used as well. Instead of having a function for
every enum, only one function is needed. This will take some data with
possible enum values.

All data needed to parse the settings is defined in settings_data.h.
This is useful for keeping all data in one place and it's easier to
check if there are mistakes.
@fwsmit fwsmit force-pushed the settings-refactor branch 2 times, most recently from 06661bf to 6ae04b8 Compare June 7, 2021 18:55
@fwsmit
Copy link
Member Author

fwsmit commented Jun 8, 2021

@bebehei The tests are now also compiled with -Werror and that's making it harder to write tests (I'm using a language extension supported by clang and gcc, but clang gives a warning about it). I think it's best to compile only dunst itself with -Werror. I tried implementing that, but now the code coverage is giving errors. If you have time, could you take a look at that?

@tsipinakis Other than that, the PR is ready. You can take a look at the changes and I especially recommend looking at the first comment in this PR for a list of changes. Let me know if you don't agree with any of the choices I made

@tsipinakis
Copy link
Member

For the record, I've already started a review on this. About half-way through but life keeps getting in the way (and this is a huge PR).
No comments so far, amazing work!

@tsipinakis tsipinakis merged commit e231c63 into dunst-project:master Jul 24, 2021
@tsipinakis
Copy link
Member

It has taken.. official 6 months. Wow sorry for this 🙈. This has been a gigantic leap forward in the code quality and readability. Thanks a lot @fwsmit.. Now to review the others :)

@fwsmit fwsmit deleted the settings-refactor branch July 25, 2021 11:10
@fwsmit
Copy link
Member Author

fwsmit commented Jul 25, 2021

It has taken.. official 6 months. Wow sorry for this see_no_evil. This has been a gigantic leap forward in the code quality and readability. Thanks a lot @fwsmit.. Now to review the others :)

Thanks for reviewing. Those 6 months weren't all you, I was still developing the branch as well ;)

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.

Add fullscreen as a global option Fwd: dunst default shortcut silently steals well known emacs keybinding
4 participants