Adding a config variable that adds extra space between the notification icon and text #810
Conversation
Codecov Report
@@ Coverage Diff @@
## master #810 +/- ##
==========================================
+ Coverage 59.48% 59.50% +0.01%
==========================================
Files 36 36
Lines 5798 5798
==========================================
+ Hits 3449 3450 +1
+ Misses 2349 2348 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
GuessWhatBBQ
changed the title
|
It would also be nice if the config file allowed more fine tuned control of padding in general. For example adding padding to text and icons separately and also specify the direction of the padding (top/bottom/left/right) |
|
The PR looks good. I haven't tested it yet, though. Could you add some documentation in
As currently worded in dunstrc, it seems like the
It's worth thinking about, but we should be careful making the config too long or complicated. |
|
I see what you mean about the wording. But just so we're clear, lets say this is the behavior without any 0 = Icon According to you it should be like this Compared to what my PR does right now: The think an implementation like the first one would require a bit more work, but I'm concerned about whether this is truly the desired effect. |
|
I don't know what's most intuitive to users, but |
|
I think that should do it |
dunstrc
Outdated
| # Padding between text and icon. | ||
| text_icon_padding = 0 | ||
|
|
||
|
|
src/draw.c
Outdated
| width -= cairo_image_surface_get_width(cl->icon) + settings.text_icon_padding; | ||
| } | ||
| else { | ||
| width -= cairo_image_surface_get_width(cl->icon) + settings.h_padding; |
There was a problem hiding this comment.
You could simplify this by adding a function get_text_icon_padding() which returns either h_padding or text_icon_padding. By adding that, you don't need if's here.
There was a problem hiding this comment.
I've thought about that. Honestly the entire file could use a lot more modularization
There was a problem hiding this comment.
Yeah, the drawing code is pretty rough (and untested)
There was a problem hiding this comment.
That modularization could also allow the fine tuning controls I mentioned before
There was a problem hiding this comment.
Yeah it would be good to do that. It's probably best to first modularize it, then add tests and lastly add some fine tuning controls.
|
Thank you for taking the time to change it. After these comments, it should be good. |
tsipinakis
left a comment
tsipinakis
left a comment
There was a problem hiding this comment.
Some code style issues, lets keep things consistent. Code looks good but I haven't tested it yet, I'll do that tomorrow.
There's also a binary file that made its way in there src/settings.h.gch, please remove it :).
|
LGTM, thank you! |
tsipinakis
left a comment
tsipinakis
left a comment
There was a problem hiding this comment.
LGTM. However, from the quick tests I did the looks of the increased padding looked weird to me. I'm curious about the usecase here (perhaps it just doesn't suit my config?).
That issue has 5 upvotes so I'll merge this either way.
|
I set my h_padding to 0 because I dont like the icon getting smaller. This has the added side effect of making the icon and text almost touch and that looks weird to me. So that's why the added gap is required like this.
|
|
Makes sense. I'll merge it so I can fix more conflicts in #799. |
4 participants
This solves/satisfies the feature request in #543 by creating another config variable called
text_icon_paddingwhich defaults to 0, but when set with a pixel value adds that to the regular h_padding to create the desired effect.Testing is required to see if it breaks any of the regular padding behavior.