dunst-project / dunst

Lightweight and customizable notification daemon
https://dunst-project.org
Other
4.42k stars 338 forks source link

Refactor length parsing and make offset a length #1330

Closed bynect closed 2 months ago

bynect commented 2 months ago

Move sanitization from the length parser to the settings file. Also make offset a length with the (N,N) syntax. offset still accepts the old syntax for backward compatibility.

This will lay the foundations for implementing #991 and dynamic height.

This is a partial reversal of commit acf0a0f, @fwsmit what do you think?

codecov-commenter commented 2 months ago

Codecov Report

Attention: Patch coverage is 78.00000% with 11 lines in your changes are missing coverage. Please review.

Project coverage is 65.95%. Comparing base (07b68f0) to head (9de5769). Report is 35 commits behind head on master.

Files Patch % Lines
src/settings.c 42.85% 8 Missing :warning:
src/option_parser.c 85.00% 3 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1330 +/- ## ========================================== + Coverage 65.71% 65.95% +0.23% ========================================== Files 50 50 Lines 8041 8209 +168 Branches 925 962 +37 ========================================== + Hits 5284 5414 +130 - Misses 2757 2795 +38 ``` | [Flag](https://app.codecov.io/gh/dunst-project/dunst/pull/1330/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dunst-project) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/dunst-project/dunst/pull/1330/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dunst-project) | `65.95% <78.00%> (+0.23%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dunst-project#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

fwsmit commented 2 months ago

Looks fine to me. Don't have time to review the whole thing though

bynect commented 2 months ago

Can I go ahead?

zappolowski commented 2 months ago

I haven't looked into the code in detail yet, but treating offset as a length of (min, max) seems odd as the values stored therein have completely different meaning, though they have the same structure. What is puzzling me is that min from it is just used during parsing for validation and otherwise ignored. If this is the case, why bother having it at all?

Maybe I am missing the bigger picture here.

bynect commented 2 months ago

I haven't looked into the code in detail yet, but treating offset as a length of (min, max) seems odd as the values stored therein have completely different meaning, though they have the same structure. What is puzzling me is that min from it is just used during parsing for validation and otherwise ignored. If this is the case, why bother having it at all?

Maybe I am missing the bigger picture here.

My idea was making in a future refactor of the parser a unified "tuple"-like type. Also it is odd for me that we have the syntax for couple of values but offset is the only one using 1x2. Also min and max are not really used, it's just a parsing artifact, because it is cast to a struct position. (for the offset)

For the height I wanted to add a min and max because dynamic heights was one of the low hanging fruit I am gonna add soon (aka one of the next prs)