Main loop: rework timer logic

tobast commented 4 months ago

This logic was over-complicated and caused multiple bugs. This hopefully remediates to this for good. Fixes #1196.

I finally took the time to look at the timer logic of the run function, check what was actually needed, and came up with this simplification. I tried to document it in the function, feedback welcome :)

This passes make test correctly:

$ make test
[…]
Total: 193 tests (84019 ticks, 0.084 sec), 2529 assertions
Pass: 193, fail: 0, skip: 0.

Issue #1196 is also fixed: I tried manually the scenarios I could think of (including the one mentioned in the issue) and could not get a Source ID xx was not found error.

Note for the future: replacing g_timeout_add with g_timeout_add_once would enhance robustness, in case a bug causes two timers to be running concurrently — which would double the number of invocations of run unnecessarily, until no notifications are shown for a time, and could exponentially blow-up if the bug occurs within the bug. However, this function was introduced in GLib 2.74, which is too recent for eg. Debian Bullseye.

bynect commented 3 months ago

Looks good to me. We have to wait for the ci to work again though

fwsmit commented 3 months ago

You could maybe use an #if to use the new glib function where available

bynect commented 3 months ago

You could maybe use an #if to use the new glib function where available

Wouldn't that be just redundant, given that currently this does the same thing?

tobast commented 3 months ago

In my opinion, using g_timeout_add_once adds a slight bit of robustness, but adding conditionals on the version of glib would decrease robustness -- eventually one of the branches of the preprocessor would be changed without the other being updated, etc., causing more opportunities for bugs than it remedies to.

bynect commented 3 months ago

Could you please rebase to master for the ci?

codecov-commenter commented 3 months ago

Codecov Report

Attention: Patch coverage is 0% with 16 lines in your changes are missing coverage. Please review.

Project coverage is 65.64%. Comparing base (bd5b333) to head (5702095). Report is 3 commits behind head on master.

Files	Patch %	Lines
src/dunst.c	0.00%	16 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #1308 +/- ## ========================================== - Coverage 65.65% 65.64% -0.01% ========================================== Files 50 50 Lines 8051 8052 +1 Branches 928 925 -3 ========================================== Hits 5286 5286 - Misses 2765 2766 +1 ``` | [Flag](https://app.codecov.io/gh/dunst-project/dunst/pull/1308/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dunst-project) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/dunst-project/dunst/pull/1308/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dunst-project) | `65.64% <0.00%> (-0.01%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dunst-project#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

bynect commented 2 months ago

Thanks @tobast

tobast commented 2 months ago

Whoops, sorry for not merging the review! I've been pretty busy…

dunst-project / dunst

Main loop: rework timer logic #1308

Codecov Report