Add support for "top" option to icon_position rule and hide_text rule

[m-bartlett]

EDIT: Features for other pull requests

A list of possible features recommended as other PRs in below conversation to keep track:

• Make height a notification rule
• Progress bar alignment (made centered within this PR)
• Add explicit vertical_padding setting, make padding act as a default for both horizontal_padding and vertical_padding if either is unset
  Add support for "top" option to icon_position rule and hide_text rule #985 (comment)

---

This adds a setting center_icon_when_no_text which centers the notification icon if the notification's text content is empty or only space characters.

The motivation for this feature is to support simple graphical notifications using the progress bar for volume, brightness, or any other such notification where the icon is the notification.

I had attempted to achieve this sort of notification using symbolic fonts (I am using a nerd-font) however dunst seems to position font characters pixel-wise which causes font faces (despite being monospaced) to not be placed consistently. Notice in this animation of volume notifications using purely symbolic font characters that the "icon" moves slightly by a few pixels along the x-axis when comparing the "low-volume" symbol to the rest of the symbols:

Admittedly the difference is slight but I believe having more freedom over the icons is a benefit, as the icon images can enforce being a consistent dimension.

Here is an animation of the same volume notification redone using my proposed code and centered icons instead of symbolic characters from fonts:

notice the base of the speaker icon does not move between each frame

So long as the body text is exclusively space characters, longer strings of spaces can be used to increase the width of the notification if the user prefers:

And as the setting name implies, if there is non-whitespace notification text content, the icon proceeds to respect the user's icon_position preference:

For reference in my dunstrc I am using the following rule to achieve this notification:

[volume]
    category="volume"   # Match criteria
    format = %b
    alignment = center
    history_ignore = yes
    fullscreen = show

and this sample notify-send command to create an "empty" notification body with an icon:

notify-send \
  -c volume \
  -h string:synchronous:volume \
  -h int:value:57 \
  -i /usr/share/icons/Papirus/48x48/status/notification-audio-volume-high.svg \
  'ignored-summary' '<span></span>'

---

I tried searching through this repository but I didn't find any code style guidelines or contribution guidelines. If those exist and I've overlooked them, please feel free to refer me to them. Otherwise please let me know if there is anything I should change about this PR to improve consistency.

[codecov-commenter]

Codecov Report

Merging #985 (b20544a) into master (da12102) will increase coverage by 0.01%.
The diff coverage is 24.00%.

@@            Coverage Diff             @@
##           master     #985      +/-   ##
==========================================
+ Coverage   61.07%   61.09%   +0.01%     
==========================================
  Files          44       45       +1     
  Lines        6836     6965     +129     
==========================================
+ Hits         4175     4255      +80     
- Misses       2661     2710      +49     

Flag	Coverage Δ
unittests	61.09% <24.00%> (+0.01%)	⬆️

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

Impacted Files	Coverage Δ
src/draw.c	11.92% <1.72%> (-0.79%)	⬇️
src/notification.c	58.51% <100.00%> (+0.19%)	⬆️
src/rules.c	77.94% <100.00%> (+2.94%)	⬆️
test/draw.c	100.00% <100.00%> (ø)
test/notification.c	100.00% <100.00%> (ø)
src/icon.c	67.22% <0.00%> (-7.31%)	⬇️
src/dbus.c	52.17% <0.00%> (ø)
test/rules.c	100.00% <0.00%> (ø)
test/test.c	93.93% <0.00%> (+0.18%)	⬆️
... and 1 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 da12102...b20544a. Read the comment docs.

[fwsmit]

Since you're making a rule for that notification anyways, it may be better to add an option center to icon_position instead. In the future this could also be expanded to more positions, as was requested in #972.

[m-bartlett]

For clarity, do you imagine that icon_position: center might yield something like this given the notification content from above (assuming the notification text is configured to be center-justified)?

I don't think this would be too hard to implement, although I personally prefer separating the feature and having the icons with text remain to the left or right. A compromise is potentially having this behavior captured in other extra option values like icon_position: center-left or icon_position: center-right, i.e the icon is centered with no text or left/right with text. They're not intuitive option names to me, but they could be described in detail in the man page, or could be made to be more verbose (e.g. icon_position: right-centered-when-no-text). What's your opinion between these approaches?

[fwsmit]

For clarity, do you imagine that icon_position: center might yield something like this given the notification content from above (assuming the notification text is configured to be center-justified)?

Yes, I was thinking something like that. Now that you show that, it occurs to me that the name top might be more fitting than center.

I don't think this would be too hard to implement, although I personally prefer separating the feature and having the icons with text remain to the left or right. A compromise is potentially having this behavior captured in other extra option values like icon_position: center-left or icon_position: center-right, i.e the icon is centered with no text or left/right with text. They're not intuitive option names to me, but they could be described in detail in the man page, or could be made to be more verbose (e.g. icon_position: right-centered-when-no-text). What's your opinion between these approaches?

Maybe your use case wasn't entirely clear to me, but I thought you wanted all your volume notifications to have no text and other kinds of notifications with text need to have their icon on the left. If that is the case you can achieve this the following way:

dunstrc

[other]
   icon_position = left
   format = "%s\n%b"

[volume]
    category="volume"   # Match criteria
    format = %b
    icon_position = top
    history_ignore = yes
    fullscreen = show

Rules are flexible enough that we don't need the added functionality of center_icon_when_no_text.

When you want to exactly mimic the behaviour of center_icon_when_no_text, e.g. automatically center the icon when no text is present in the notification you can do this by using a script like this instead of dunstify:

# didn't test this, but something like this might work
if [ -z $1]
then
   shift 1
   dunstify "empty-summary" $@
else
  dunstify $@
fi

dunstrc

[other]
   icon_position = left
   format = "%s\n%b"

[empty]
    summary = "empty-summary"   # Match criteria
    format = %b
    icon_position = top
    history_ignore = yes
    fullscreen = show

Does either solutions satisfy your use case?

[m-bartlett]

The config you shared and what you suggest with setting icon_position for each notification rule would certainly satisfy my desired use case much more cleanly with the top option implemented.

However I am not observing that icon_position can be overridden in the rule for a specific notification match, I'm testing with the latest commit on master. It doesn't seem to be listed as one of the properties that can be overridden in the comment in the sample rc. I have icon_position = left set globally (perhaps that is the issue?) but trying to modify it to right or off in the rule still results in the notification showing the icon to the left.

i.e. my config is

[global]
...
    icon_position = left
...
[volume]
    category="volume"
    format = %b
    alignment = center
    history_ignore = yes
    fullscreen = show
    icon_position = right  # <---

After reloading dunst, testing with:

notify-send -c volume -h string:synchronous:volume 'test summary' 'test body' -i /usr/share/icons/Vibrancy-Colors/status/96/audio-volume-high-panel.png

yields:

Do I have this configured incorrectly? Or am I misunderstanding anything? Or if that feature also needs to be implemented, I could potentially provide that as a commit here as well as the top icon placement.

[fwsmit]

The config you shared and what you suggest with setting icon_position for each notification rule would certainly satisfy my desired use case much more cleanly with the top option implemented.

Great, let's do that then.

However I am not observing that icon_position can be overridden in the rule for a specific notification match, I'm testing with the latest commit on master. It doesn't seem to be listed as one of the properties that can be overridden in the comment in the sample rc. I have icon_position = left set globally (perhaps that is the issue?) but trying to modify it to right or off in the rule still results in the notification showing the icon to the left.

i.e. my config is

[global]
...
    icon_position = left
...
[volume]
    category="volume"
    format = %b
    alignment = center
    history_ignore = yes
    fullscreen = show
    icon_position = right  # <---

After reloading dunst, testing with:

notify-send -c volume -h string:synchronous:volume 'test summary' 'test body' -i /usr/share/icons/Vibrancy-Colors/status/96/audio-volume-high-panel.png

yields:

Do I have this configured incorrectly? Or am I misunderstanding anything? Or if that feature also needs to be implemented, I could potentially provide that as a commit here as well as the top icon placement.

I didn't notice this before, but icon_position currently is only allowed as a global setting as you can see right here. So first icon_position needs to be made a rule. That isn't too hard, but the right steps need to be taken for that. I've made a checklist for myself that I'll share with you:

How to add/change a rule

• Add variable to struct rules in rules.h (make sure to read the comment at the top)
• Add variable to to struct notification in notification.h
• Apply the rule in rule_apply in rules.c
• Change the listing in settings_data.h (make sure to move it to the other rule listings for clarity)
• Add the default rule value in settings_data.h (usually -1 or NULL)
• Set default value in notification.c (notification_create). This is where the
  real default is set.
• Free the variable in notification.c if dynamically allocated.
• Free the variable in rules.c if dynamically allocated.
• Remove the setting from the global settings struct in settings.h.
• Actually use the new setting.
• Update the documentation
• Test that it works

An example of making something a rule can be found in edc6f5a

[m-bartlett]

So first icon_position needs to be made a rule. That isn't too hard, but the right steps need to be taken for that. I've made a checklist for myself that I'll share with you:

How to add/change a rule

• Add variable to struct rules in rules.h (make sure to read the comment at the top)
• Add variable to to struct notification in notification.h
• Apply the rule in rule_apply in rules.c
• Change the listing in settings_data.h (make sure to move it to the other rule listings for clarity)
• Add the default rule value in settings_data.h (usually -1 or NULL)
• Set default value in notification.c (notification_create). This is where the
  real default is set.
• Free the variable in notification.c if dynamically allocated.
• Free the variable in rules.c if dynamically allocated.
• Remove the setting from the global settings struct in settings.h.
• Actually use the new setting.
• Update the documentation
• Test that it works

An example of making something a rule can be found in edc6f5a

Thanks for the guidance, I'll work towards making icon_position a mutable rule property, as well as implementing the top option. This may take me a few weeks due to the upcoming holidays occupying much of my free time, but once I feel I have a candidate set of changes I will force push over the source branch used in this PR and @ you for a review.

[fwsmit]

Nice! There's also this issue about more icon positions (#972), if you want to.

[fwsmit]

Hey @m-bartlett have you had time to work on this?

[m-bartlett]

Not quite yet but it has been on my todo list, just putting some finishing touches on a different project

[m-bartlett]

Made a little bit of headway, I've added icon_position as a notification rule, added "top" to the icon_position enum/docstrings, and I've got the basics for the layout changes needed to position the icon at the top. Currently working around modifying the notification height/width, and text dimensions/placement appropriately when the icon_position is set to "top". No pushes to this branch yet.

I am rolling with the assumption that the global setting "height" should still be respected absolutely even with the icon being on top generally adding a lot more height. If you use a relatively large icon it causes the notification to exhaust its height before ever putting the text. It might be hard to restrict the height of "normal" notifications while supporting a few custom rules that allow a top-icon since the height boundary will be filled in differently. Let me know if you think this should be different, i.e. height should only restrict the text's rendered height and ignore the icon.

An example of what I mean:

With height 100 icon: left

With height 100 icon: top (text placement will obviously need to be fixed)

[m-bartlett]

@fwsmit I've got the core of these features implemented as far as my testing has proven. Still no pushes to this PR's branch yet as I want to run everything by you first before pushing

Things needing your decision:

• Progress bars have a min/max width setting but they are aligned left if the notification is wider than the progress bar. I changed this behavior to center the progress bar within the notification window (see my screenshot below). As far as I'm able to tell, there isn't a configuration option for this behavior (yet). This could probably be added as a configurable setting but let's save that for a different PR if necessary. I can easily undo this change if you think that's best.

• I'm finding with the vertical stacked layout implied by having the icon on "top", that the notification text (i.e. summary+body) always has a height, even if these fields are "empty" (i.e. just whitespace characters, as dbus doesn't seem to support a truly empty notification). In practice I find this to cause a noticeably large empty gap in a "textless" notification which is the use-case I'm trying to support. Consider this screenshot from my working branch:

  

  My ideal in the top case with "no text" is that this padding would not be added to the layout.
  I'm aware from the man pages that format="" is reserved for silencing certain matched notifications so that's not an option for configuring a notification with no text. However, what are you thoughts on adding another rule that would indicate not to render any text? Maybe something like like textless=yes or no_text=yes.
  I propose this because there is no straightforward way of checking if a notification is "empty" as A) the notification cannot actually really be empty and B) Pango will always render these not-actually-empty strings with a height. Alternatively I could add similar logic from my original PR (i.e. if the body is only space characters then it is empty) but I'm skeptical that that's a good approach.

[fwsmit]

I am rolling with the assumption that the global setting "height" should still be respected absolutely even with the icon being on top generally adding a lot more height.

Yes that seems right.

If you use a relatively large icon it causes the notification to exhaust its height before ever putting the text. It might be hard to restrict the height of "normal" notifications while supporting a few custom rules that allow a top-icon since the height boundary will be filled in differently. Let me know if you think this should be different, i.e. height should only restrict the text's rendered height and ignore the icon.

The default max height could probably be a bit higher, but for these cases you can always set the max height higher with a rule.

I've got the core of these features implemented as far as my testing has proven. Still no pushes to this PR's branch yet as I want to run everything by you first before pushing

Feel free to push unfinished code, you can always force-push later.

Things needing your decision:

* Progress bars have a min/max width setting but they are aligned left if the notification is wider than the progress bar. I changed this behavior to center the progress bar within the notification window (see my screenshot below). As far as I'm able to tell, there isn't a configuration option for this behavior (yet). This could probably be added as a configurable setting but let's save that for a different PR if necessary. I can easily undo this change if you think that's best.

A new progress bar position setting best for another PR. But for now, the centering behaviour seems fine.

  My ideal in the top case with "no text" is that this padding would not be added to the layout.
  I'm aware from the man pages that `format=""` is reserved for silencing certain matched notifications so that's not an option for configuring a notification with no text. However, what are you thoughts on adding another rule that would indicate not to render any text? Maybe something like like `textless=yes` or `no_text=yes`.

format="" does exactly what you want. You can match the desired notifications and remove the text from there. What would a textless=yes option add?

[fwsmit]

This feature cannot really be tested via automated testing, but it would be nice to easily test it in the future, so could you add a few tests to test/functional-tests/test.sh?

[m-bartlett]

The default max height could probably be a bit higher, but for these cases you can always set the max height higher with a rule.

This would be sensible, however neither height or width are supported as rules currently, just global settings. Should I go ahead and make these rules?

---

format="" does exactly what you want. You can match the desired notifications and remove the text from there. What would a textless=yes option add?

format="" suppresses the notification, i.e. it doesn't show the notification at all. This is the behavior I observe, plus this behavior is (somewhat confusingly) documented—for example in man page for dunstrc:

always_run_script (values: [true/false] default: [true]
           Always run rule-defined scripts, even if the
           notification is suppressed with format = "". See
           SCRIPTING.

However later on in the same man page under the markup section:

If the format is set to an empty string, the notification will not be suppressed.

I'm suggesting the no_text rule as a means to inform the notification rending to skip rendering text, but still show the rest of the notification (like the icon and progress bar).

For example, in this notification the red box shows where whitespace text is still being rendered:

Whereas with a rule like no_text=yes the text rendering would be skipped and would not occupy height:

I am not set on this rule, just suggesting it as an idea. There is the rule skip_display = true which makes the intent to not show the rule explicit, so given this I support making format="" still show the notification, just without any text. What are your thoughts on this? It may introduce some unexpected new behavior in end-user dunstrc's.

---

This feature cannot really be tested via automated testing, but it would be nice to easily test it in the future, so could you add a few tests to test/functional-tests/test.sh?

Yep I will add some test cases there

[fwsmit]

This would be sensible, however neither height or width are supported as rules currently, just global settings. Should I go ahead and make these rules?

format="" suppresses the notification, i.e. it doesn't show the notification at all. This feature is somewhat confusingly documented, but for example in man page for dunstrc:

I was wrong on both of these, thanks for correcting. I it would be sensible to add this option, but I would call it hide_text.

As for the height, yes, this should definitely be made a rule, but I would prefer if you did that in another PR. The width cannot really be made a rule, since we are limited to a single notification window. Until that's fixed, I think it's best to stick to one width.

always_run_script (values: [true/false] default: [true]
           Always run rule-defined scripts, even if the
           notification is suppressed with format = "". See
           SCRIPTING.

However later on in the same man page under the markup section:

If the format is set to an empty string, the notification will not be suppressed.

This documentation should definitely be improved. Also, I think that letting format="" be a way of skipping is a bit confusing, but for backward compatibility we should let it in for a bit.

[m-bartlett]

Okay, so for next actionables:

• Make height (i.e. max height) a rule (separate PR)
• Add hide_text as a rule
• Don't modify the behavior of format

Is this correct?

[fwsmit]

Okay, so for next actionables:

* Make `height` (i.e. max height) a rule (separate PR)

* Add `hide_text` as a rule

* Don't modify the behavior of `format`

Is this correct?

Yep, but I would do the height changes in a separate PR.

[m-bartlett]

Okay I will work on adding the hide_text rule as well as some test cases. Once it appears to be working as intended I will push to this branch.

[m-bartlett]

@fwsmit I believe this is ready for your review, let me know if there's anything I should change

[fwsmit]

Overall the changes look good. I'll take a closer look at the changes in draw.c later, but I don't expect to find any big problems. I've left a few comments here and there.

[m-bartlett]

@fwsmit I believe I've made all requested changes, let me know what you think.

I consolidated the icon_position tests into one batch of notifications, and then added iterating different padding configurations for the icon_position test.

Here is each batch of icon_position tests rendered with the 4 different padding configurations now found in the icon_position test as rendered on my screen:

[fwsmit]

Thank you, looks perfect. The functional tests are also very nice.