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

Corners patch part 1 #1227

Merged
merged 5 commits into from
Jan 20, 2024
Merged

Corners patch part 1 #1227

merged 5 commits into from
Jan 20, 2024

Conversation

bynect
Copy link
Member

@bynect bynect commented Nov 7, 2023

I divided the patch in #1213 to simplify things a bit. This first part only deals with the rendering of the corners. The second part (which I will add soon) adds support for the customization option to change corner profile. Anyway, refer to #1213 for any information on the changes.

@bynect
Copy link
Member Author

bynect commented Nov 7, 2023

Should I close the other?

@fwsmit
Copy link
Member

fwsmit commented Nov 9, 2023

Should I close the other?

No that's not neccesary

@codecov-commenter
Copy link

Codecov Report

Merging #1227 (a697e8c) into master (19865c0) will decrease coverage by 0.72%.
Report is 22 commits behind head on master.
The diff coverage is 20.37%.

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

@@            Coverage Diff             @@
##           master    #1227      +/-   ##
==========================================
- Coverage   66.03%   65.31%   -0.72%     
==========================================
  Files          46       46              
  Lines        7595     7637      +42     
==========================================
- Hits         5015     4988      -27     
- Misses       2580     2649      +69     
Flag Coverage Δ
unittests 65.31% <20.37%> (-0.72%) ⬇️

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

Files Coverage Δ
src/x11/x.c 0.00% <ø> (ø)
test/draw.c 98.87% <100.00%> (+0.01%) ⬆️
src/draw.c 42.48% <15.68%> (-8.88%) ⬇️

... and 2 files with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

@fwsmit
Copy link
Member

fwsmit commented Dec 31, 2023

Sorry that I've taken so long to review this. This PR still contains more than just the fix for the progress bar rendering, but I'll try to roll with it. Could you explain how the bug happened in the old progress bar implementation? Good testing is nice, but it's also good to know what went wrong.

@fwsmit
Copy link
Member

fwsmit commented Dec 31, 2023

The code does look good, but I'll have to take a closer later.

@bynect
Copy link
Member Author

bynect commented Jan 1, 2024

Sorry that I've taken so long to review this. This PR still contains more than just the fix for the progress bar rendering, but I'll try to roll with it. Could you explain how the bug happened in the old progress bar implementation? Good testing is nice, but it's also good to know what went wrong.

To be fair, the only thing that is not about corners is the bar adjustment, which is minimal and I already commented in the original pr. (It just deals with either a pango antialiasing/number rounding issue that makes the lines slightly skewed)

When you merge this I can make the part 2 of the pr for costumizing corners

@bynect
Copy link
Member Author

bynect commented Jan 18, 2024

did you manage to see it?

The code does look good, but I'll have to take a closer later.

@fwsmit
Copy link
Member

fwsmit commented Jan 20, 2024

Thanks for the poke. The code looks good, I haven't found any issues with it. Thanks for your patience and working on this fix. I will go ahead and merge this. I look forward to the V2 patch!

@fwsmit fwsmit merged commit 2fcea84 into dunst-project:master Jan 20, 2024
18 checks passed
@bynect
Copy link
Member Author

bynect commented Jan 20, 2024

Thanks. I'll prepare the patch as soon as I have a little spare time since the code is already there. Sometime next week at most

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.

3 participants