Skip to content

Conversation

CelDaemon
Copy link

This PR fixes #832 , ensuring any applications listening to the pause state properties will always receive proper updates.

The introspection XML is also modified to advertise the changed signals for the *Length properties, which was missing before, and mentioned in the same issue.

Add the missing EmitsChangedSignal annotations to the *Length dbus
properties, which were given changed signals in dunst-project#1127 .
Fixes incorrect introspection mentioned in dunst-project#832 .
Make SIGUSR* signal handlers send a PropertiesChanged signal after
modifying the paused state. Fixes the remainder of dunst-project#832 .
@CelDaemon
Copy link
Author

Checks seem to have failed, I assume there's a problem in the CI?

@bynect
Copy link
Member

bynect commented Sep 7, 2025

Yes, its unrelated to the test suite.
I'll take a look

@bynect
Copy link
Member

bynect commented Sep 9, 2025

code looks good.
do you mind adding a test like these?

void dbus_signal_cb_propertieschanged(GDBusConnection *connection,

These tests ensure that PropertiesChanged signals are sent when the
pause and unpause signals are handled.

Signal handler functions are called directly instead of emitting real
signals, as doing this might not be safe inside of a testing
environment.
@CelDaemon
Copy link
Author

@bynect I've added two tests for the pause and unpause signals, I hope these will suffice.

One thing I have noticed is that tests which subscribe to DBus signals seem to lock up when failing. This seems to be a result of the assert functions exiting the function immediately without ever getting to the cleanup code, which in turn leaks memory and never unsubscribes from the DBus signal.

I haven't solved this issue in my new test though, as doing so likely means a rework in many of the existing tests as well.

@codecov-commenter
Copy link

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

Codecov Report

❌ Patch coverage is 98.14815% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 65.02%. Comparing base (ee63a72) to head (129b52d).

Files with missing lines Patch % Lines
src/dbus.c 92.85% 1 Missing ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1505      +/-   ##
==========================================
+ Coverage   64.72%   65.02%   +0.30%     
==========================================
  Files          51       51              
  Lines        9082     9129      +47     
  Branches     1068     1071       +3     
==========================================
+ Hits         5878     5936      +58     
+ Misses       3204     3193      -11     
Flag Coverage Δ
unittests 65.02% <98.14%> (+0.30%) ⬆️

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.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@bynect
Copy link
Member

bynect commented Sep 9, 2025

two are fine 👍🏻

One thing I have noticed is that tests which subscribe to DBus signals seem to lock up when failing. This seems to be a result of the assert functions exiting the function immediately without ever getting to the cleanup code, which in turn leaks memory and never unsubscribes from the DBus signal.

you mean they will deadlock?

i don't know if it is worth it reworking the code for all those test functions, after all if we detect a failure nothing guarantees the state of the program is correct

@CelDaemon
Copy link
Author

CelDaemon commented Sep 9, 2025

you mean they will deadlock?

Yes, which may be troublesome if the intention is to report on all tests even if some of them fail.
Just thought I'd mention it in case it wasn't known yet, even if fixing it might not be worth it.

Glad to see everything is passing and working now though!

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.

Subscription for some dbus properties doesn't work
3 participants