Skip to content

Conversation

@bynect
Copy link
Contributor

@bynect bynect commented Apr 9, 2024

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
Copy link

codecov-commenter commented Apr 9, 2024

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 ⚠️
src/option_parser.c 85.00% 3 Missing ⚠️

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

Additional details and impacted files
@@            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 Coverage Δ
unittests 65.95% <78.00%> (+0.23%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fwsmit
Copy link
Member

fwsmit commented Apr 10, 2024

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

@bynect bynect requested review from fwsmit and zappolowski and removed request for fwsmit April 11, 2024 18:00
@bynect bynect requested a review from fwsmit April 19, 2024 09:20
@bynect
Copy link
Contributor Author

bynect commented Apr 19, 2024

Can I go ahead?

@zappolowski
Copy link
Member

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
Copy link
Contributor Author

bynect commented Apr 19, 2024

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)

@bynect bynect merged commit a6e1d4e into dunst-project:master Apr 20, 2024
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.

4 participants