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

Changing urgency by a rule does not affect color of message #484

Closed
ffwng opened this issue Jan 15, 2018 · 6 comments
Closed

Changing urgency by a rule does not affect color of message #484

ffwng opened this issue Jan 15, 2018 · 6 comments
Labels

Comments

@ffwng
Copy link

ffwng commented Jan 15, 2018

Issue description

Use the default config and append the following rule:

[notify-send]
    appname = notify-send
    urgency = critical

Execute notify-send Test.

The message is shown but with a blue color (like a normal message) instead of the red color for critical messages.

dunst -print outputs

{
	appname: 'notify-send'
	summary: 'Test'
	body: ''
	icon: 'dialog-information'
	raw_icon set: false
	category: 
	timeout: 10000
	urgency: CRITICAL
	transient: 0
	formatted: '<b>Test</b>'
	fg: #ffffff
	bg: #285577
	frame: #aaaaaa
	id: 2
	script: (null)
}

So the urgency is applied, but the colors are not changed accordingly.

This used to work with correctly with version 1.2. The commit which breaks this behavior seems to be 4bfae81.

Installation info

  • Version: 1.3.0 (2018-01-05)
  • Install type: package
  • Distro and version: Arch Linux
@ffwng ffwng changed the title Changing urgency by a rule does not affect color of message Changing urgency with a rule does not affect color of message Jan 15, 2018
@ffwng ffwng changed the title Changing urgency with a rule does not affect color of message Changing urgency by a rule does not affect color of message Jan 15, 2018
@bebehei
Copy link
Member

bebehei commented Jan 15, 2018

Oh, err. Something went south in #428. Thanks for pointing this out.

This used to work with correctly with version 1.2. The commit which breaks this behavior seems to be 4bfae81.

Thank you! You're awesome! More people should know how to git bisect!

When looking at the commit, I see I've flipped the rule application and the default color assignment. This is not so nice. But I'm reluctant to fix it in favor of #328. We want to drop these urgency_* configuration parts and actually make them what they are: rules.

Could you please use a workaround?

[notify-send]
    appname = notify-send
    urgency = critical

[defaults_critical]
    msg_urgency = critical
    background....

@tsipinakis I'd label this as wontfix. Do you have objections?

@tsipinakis
Copy link
Member

@tsipinakis I'd label this as wontfix. Do you have objections?

As I mentioned in #328 initially I only intend to change the handling of the sections internally to be technically rules but also work in the same way in the config file, we can talk about deprecation later.

Until then, this is definitely worth fixing but it'll probably have to wait a bit since this is a tough problem to solve and will probably required a rework of the rule system.


A quick outline of the problem: Dunst currently parses rules in the order they are defined and doesn't "go back" to check something if an attribute is updated so what happens internally is this

  1. New notification comes through priority normal
  2. Applies all attributes for a normal notification
  3. Applies rule that sets urgency to critical
  4. Displays notification

What's missing is a step between 3-4 where it goes back and applies all attributes for critical urgency. It's simple to implement now when the urgency_ sections support a grand total of 3 or 4 settings but with #328 intending to change that to be an implicit rule things start getting more complex and I think it's worth discussing if we want to keep the rule system in a dead simple top-down way as it is now or we want to complicate it a bit and start re-matching all the rules when an attribute that could affect previous rules changes.

I think I'm beginning to realise why the urgency_ sections weren't rules to begin with.

PS: We really need more tests, this is a prime example of an issue that could've been caught by a proper test suite

@ffwng
Copy link
Author

ffwng commented Jan 16, 2018

I see your problem. Personally, I am in favor of threating the urgency_ sections as rules, they just have to be moved to the bottom of the config file. I would keep the top-down approach, as it is easy to understand and predictable.

For the moment, the proposed workaround (using another rule to apply the colors again) works for me.

@tsipinakis tsipinakis added the Bug label Mar 6, 2018
@fwsmit
Copy link
Member

fwsmit commented Mar 13, 2021

I see your problem. Personally, I am in favor of threating the urgency_ sections as rules, they just have to be moved to the bottom of the config file. I would keep the top-down approach, as it is easy to understand and predictable.

For the moment, the proposed workaround (using another rule to apply the colors again) works for me.

This has been done in #803, so this issue can be closed when it's merged

@viktordanov
Copy link

#803 has been merged though I'm experiencing the same problem as @fwsmit:
I match the appname of a test process (e.g. notify-send) and change it's urgency to CRITICAL. I of course have different styles for CRITICAL and have tested it with notify-send -u critical "Lorem ipsum". The issue remains that even though the rule is applied the styles are not recalculated based on the urgency (in spite of the urgency rules being on the bottom of the config file).

@fwsmit
Copy link
Member

fwsmit commented Nov 2, 2021

#803 has been merged though I'm experiencing the same problem as @fwsmit: I match the appname of a test process (e.g. notify-send) and change it's urgency to CRITICAL. I of course have different styles for CRITICAL and have tested it with notify-send -u critical "Lorem ipsum". The issue remains that even though the rule is applied the styles are not recalculated based on the urgency (in spite of the urgency rules being on the bottom of the config file).

@viktordanov I'm not having the problem, I just happened to implement the proposed solution in #803.
I tried to to reproduce what you said, but for me it just works. Make sure you're on the latest version of dunst, v1.7.1. v1.7.0 should work as well. If you're still having problems, please reopen this issue and send your dunstrc. For the moment I consider this one closed.

@fwsmit fwsmit closed this as completed Nov 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants