[RFC] config reload mechanism #1350
Conversation
|
@zappolowski it seems like the ci is having a problem with arch maybe it's https://bbs.archlinux.org/viewtopic.php?id=276422? |
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #1350 +/- ##
==========================================
- Coverage 66.08% 65.75% -0.34%
==========================================
Files 50 50
Lines 8247 8293 +46
Branches 958 963 +5
==========================================
+ Hits 5450 5453 +3
- Misses 2797 2840 +43
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
mostly done. I'll do some more checks and maybe add a functional test |
| @@ -179,6 +179,10 @@ gint64 queues_get_next_datachange(gint64 time); | |||
| */ | |||
| struct notification* queues_get_by_id(int id); | |||
|
|
|||
| /** | |||
| * Reapply all rules to the queue (used when reloading configs) | |||
There was a problem hiding this comment.
What if a property of a notification is previously changed by a rule? Then reapplying the rule will apply to the changed notification, not the original. This might result in weird behavior. To make it work correctly you have to store the original notification's contents
There was a problem hiding this comment.
That's true, but for now I think this is the correct behavior. I also added it to the documentation. Maybe we could add an option keep_original or something to maintain a copy of the original notification if the user so wants. Doing so by default would add to the memory footprint I think
There was a problem hiding this comment.
I would definitely not make an option for this. The behavior will probably be acceptable in most cases, but the more correct thing to do is store the original notification and apply the rule to that.
Say a rule matches a notification with a timeout of 10 seconds with match_dbus_timeout and sets the timeout to 20 seconds and the color to yellow. Let's say you change your config to change the color to green instead and reload. You expect the notification to become green, but it stays yellow, because it's timeout has been changed to 20, making it not match the rule anymore.
Of course this is not the end of the world, but it is not the correct behavior. I don't see many rules that would make it a very common occurrence, so it would be fine by me to keep it like this. But it would be good to document and make an issue about it, so it is not forgotten.
There was a problem hiding this comment.
I think I already wrote it somewhere in the docs. But I can just as easily save the original state if you so want
There was a problem hiding this comment.
@fwsmit I would prefer doing that in another pr if possible since this is already quite big. How are the current changes looking?
|
Thanks for picking this up! I never got it to work properly when I tried it. But when the code is structured well, it should not be too hard to implement |
I made some updates. The only thing I am a bit unsure is the reapply of rules |
|
Basically I added a rule struct inside the notification that is allocated and filled when we try to apply a rule to change a value. Then when we reload we reapply that rule to "revert" the original state. @fwsmit does this solve the problem you said in the comments? |
|
arch ci is not working ... |
I wanted to add a way to reload the config files. part of the code is taken from #968.
In respect to #968 I added the ability to pass a list of config files to use to the dbus method. If none are passed it will use the old config list.
I noticed that the cmdline parser ignores multiple
-confoptions (without any acknowledgment of the fact). So I changed the behavior to accept a list of files.Summary
ConfigReload-confvaluesdunstctl reloadand completions