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

[RFC] config reload mechanism #1350

Merged
merged 18 commits into from
Jul 15, 2024
Merged

[RFC] config reload mechanism #1350

merged 18 commits into from
Jul 15, 2024

Conversation

bynect
Copy link
Member

@bynect bynect commented May 2, 2024

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 -conf options (without any acknowledgment of the fact). So I changed the behavior to accept a list of files.


Summary

  • Add dbus method ConfigReload
  • Refactor cmdline parsing to account for multiple -conf values
  • Cleanup wayland output on deinit
  • Update tests and documentation
  • Add dunstctl reload and completions
  • Keep original notification state for reload

@bynect
Copy link
Member Author

bynect commented May 2, 2024

@zappolowski it seems like the ci is having a problem with arch

maybe it's https://bbs.archlinux.org/viewtopic.php?id=276422?

@bynect bynect requested review from fwsmit and zappolowski May 2, 2024 17:54
@codecov-commenter
Copy link

codecov-commenter commented May 2, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 41.28440% with 128 lines in your changes missing coverage. Please review.

Project coverage is 65.37%. Comparing base (20033b8) to head (703bc5a).
Report is 6 commits behind head on master.

Files Patch % Lines
src/rules.c 35.71% 63 Missing ⚠️
src/dunst.c 0.00% 24 Missing ⚠️
src/wayland/wl.c 0.00% 14 Missing ⚠️
src/dbus.c 47.36% 10 Missing ⚠️
src/queues.c 0.00% 9 Missing ⚠️
src/option_parser.c 83.33% 3 Missing ⚠️
src/settings.c 66.66% 3 Missing ⚠️
src/draw.c 0.00% 1 Missing ⚠️
test/icon-lookup.c 0.00% 1 Missing ⚠️

❗ 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.37%   -0.72%     
==========================================
  Files          50       50              
  Lines        8247     8326      +79     
  Branches      958     1000      +42     
==========================================
- Hits         5450     5443       -7     
- Misses       2797     2883      +86     
Flag Coverage Δ
unittests 65.37% <41.28%> (-0.72%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bynect bynect marked this pull request as ready for review May 3, 2024 08:02
@bynect
Copy link
Member Author

bynect commented May 3, 2024

mostly done. I'll do some more checks and maybe add a functional test

main.c Outdated Show resolved Hide resolved
src/dunst.c Outdated Show resolved Hide resolved
src/queues.h Show resolved Hide resolved
src/notification.c Outdated Show resolved Hide resolved
@fwsmit
Copy link
Member

fwsmit commented May 6, 2024

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

src/dunst.c Show resolved Hide resolved
@bynect
Copy link
Member Author

bynect commented May 7, 2024

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

This was referenced May 22, 2024
@bynect bynect requested a review from fwsmit May 28, 2024 23:24
@bynect
Copy link
Member Author

bynect commented May 28, 2024

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.
it should work but I haven't tested much (it is kind of involved)

@fwsmit does this solve the problem you said in the comments?

@bynect
Copy link
Member Author

bynect commented May 29, 2024

arch ci is not working ...

dunstctl Show resolved Hide resolved
completions/dunstctl.fishcomp Outdated Show resolved Hide resolved
src/queues.c Outdated Show resolved Hide resolved
src/rules.c Outdated Show resolved Hide resolved
src/settings.c Outdated Show resolved Hide resolved
src/wayland/wl.c Show resolved Hide resolved
@bynect
Copy link
Member Author

bynect commented Jul 1, 2024

@zappolowski I made the changes you suggested. Did you try hot reloading and found any problem? I wanted to merge this soon

docs/dunstctl.pod Outdated Show resolved Hide resolved
src/wayland/wl.c Show resolved Hide resolved
@bynect
Copy link
Member Author

bynect commented Jul 10, 2024

@fwsmit @zappolowski I finished. Also updated the docs. Can I merge this? wanted to release a version in this month what do you think?

@fwsmit
Copy link
Member

fwsmit commented Jul 14, 2024

Looks good to me!

@bynect
Copy link
Member Author

bynect commented Jul 15, 2024

I'll merge then

@bynect bynect merged commit 844167a into dunst-project:master Jul 15, 2024
21 checks passed
@Fxzzi
Copy link

Fxzzi commented Sep 25, 2024

seems like running dunstctl reload whilst a notification on screen causes dunst to stop working.

@bynect
Copy link
Member Author

bynect commented Sep 26, 2024

seems like running dunstctl reload whilst a notification on screen causes dunst to stop working.

How so? It should just reapply the config to the notification

@Dreuzz
Copy link

Dreuzz commented Sep 28, 2024

dunstctl reload /home/dreuzzz/.cache/wal/dunst

Error org.freedesktop.DBus.Error.UnknownMethod: No such method “ConfigReload”
Failed to communicate with dunst, is it running? Or maybe the version is outdated. You can try 'dunstctl debug' as a next debugging step.

@bynect
Copy link
Member Author

bynect commented Sep 29, 2024

dunstctl reload /home/dreuzzz/.cache/wal/dunst

Error org.freedesktop.DBus.Error.UnknownMethod: No such method “ConfigReload” Failed to communicate with dunst, is it running? Or maybe the version is outdated. You can try 'dunstctl debug' as a next debugging step.

this means that the running version of dunst is not updated and doesn't have the reload patch.

@Dreuzz
Copy link

Dreuzz commented Sep 29, 2024

dunst -v

Dunst - A customizable and lightweight notification-daemon v1.11.0-76-gc1cc6d1
Compiled on 2024-09-29 with the following options:
X11 support: enabled
Wayland support: enabled
SYSCONFDIR set to: /usr/local/etc/xdg
Compiler flags: -g -std=gnu11 -pedantic -Wall -Wno-overlength-strings -Os -pthread -MMD -MP
Linker flags: -lm -lrt -lgio-2.0 -lgdk_pixbuf-2.0 -lglib-2.0 -lpangocairo-1.0 -lpango-1.0 -lgobject-2.0 -lharfbuzz -lcairo -lwayland-client -lwayland-cursor -lX11 -lXinerama -lXext -lXrandr -lXss

@bynect
Copy link
Member Author

bynect commented Sep 29, 2024

dunst -v

Dunst - A customizable and lightweight notification-daemon v1.11.0-76-gc1cc6d1 Compiled on 2024-09-29 with the following options: X11 support: enabled Wayland support: enabled SYSCONFDIR set to: /usr/local/etc/xdg Compiler flags: -g -std=gnu11 -pedantic -Wall -Wno-overlength-strings -Os -pthread -MMD -MP Linker flags: -lm -lrt -lgio-2.0 -lgdk_pixbuf-2.0 -lglib-2.0 -lpangocairo-1.0 -lpango-1.0 -lgobject-2.0 -lharfbuzz -lcairo -lwayland-client -lwayland-cursor -lX11 -lXinerama -lXext -lXrandr -lXss

you are using master branch right? You have to make sure that the program actually running is this one. sometimes the build systems/package managers start a different binary than the one in path

@B1ack3ye
Copy link

B1ack3ye commented Oct 4, 2024

On my Arch installation, dunstctl reload is not working:

[b1ack3ye@Black-PC ~]$ dunst -v
Dunst - A customizable and lightweight notification-daemon 1.11.0 (2024-04-15)
[b1ack3ye@Black-PC ~]$ dunstctl reload
dunstctl: unrecognized command 'reload'. Please consult the usage.
[b1ack3ye@Black-PC ~]$ dunstify "Test"
[b1ack3ye@Black-PC ~]$ dunstctl reload
dunstctl: unrecognized command 'reload'. Please consult the usage.

@zappolowski
Copy link
Member

@B1ack3ye this feature is not yet released. If you want to it you either have to build it yourself or use dunst-git from AUR.

@Fxzzi
Copy link

Fxzzi commented Oct 5, 2024

seems like running dunstctl reload whilst a notification on screen causes dunst to stop working.

How so? It should just reapply the config to the notification

yeah, which is what I'm thinking, however afterwards no notifications show up. For now I just pkill dunst and it starts itself up again next time a notification is sent.

@bynect
Copy link
Member Author

bynect commented Oct 6, 2024

seems like running dunstctl reload whilst a notification on screen causes dunst to stop working.

How so? It should just reapply the config to the notification

yeah, which is what I'm thinking, however afterwards no notifications show up. For now I just pkill dunst and it starts itself up again next time a notification is sent.

do you mean when reloading all the notifications are closed? or when new notifications come nothing show up? it would be very helpful to provide a debug log (dunst -verbosity debug)

@Fxzzi
Copy link

Fxzzi commented Oct 6, 2024

do you mean when reloading all the notifications are closed? or when new notifications come nothing show up?

Both. current notifications close, and no new notifications show up.

it would be very helpful to provide a debug log (dunst -verbosity debug)

sure, here u go. https://paste.rs/xSpVi.txt

in this log, i simply launched dunst with verbosity, triggered my volume notification, and then did a dunstctl reload

@bynect
Copy link
Member Author

bynect commented Oct 6, 2024

do you mean when reloading all the notifications are closed? or when new notifications come nothing show up?

Both. current notifications close, and no new notifications show up.

it would be very helpful to provide a debug log (dunst -verbosity debug)

sure, here u go. https://paste.rs/xSpVi.txt

in this log, i simply launched dunst with verbosity, triggered my volume notification, and then did a dunstctl reload

Are you using wayland right? Since I can only test on xorg maybe this is a wayland only bug. could you try using dunst in Xwayland mode to see if this works there?

@Fxzzi
Copy link

Fxzzi commented Oct 6, 2024

Are you using wayland right? Since I can only test on xorg maybe this is a wayland only bug. could you try using dunst in Xwayland mode to see if this works there?

yep, I'm running on wayland.

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.

7 participants