fix(naughty): ensure default timeout applies without ruled.notification#4081
fix(naughty): ensure default timeout applies without ruled.notification#4081JimmyCozza wants to merge 4 commits intoawesomeWM:masterfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4081 +/- ##
==========================================
+ Coverage 90.45% 90.47% +0.02%
==========================================
Files 941 941
Lines 60385 60422 +37
Branches 1145 1145
==========================================
+ Hits 54621 54669 +48
+ Misses 5254 5246 -8
+ Partials 510 507 -3
🚀 New features to boost your workflow:
|
…on (awesomeWM#3239) - Remove constructor guard that skipped set_timeout() when no explicit timeout was provided - Fix fallback config path: cst.config.timeout → cst.config.defaults.timeout - Stop and nil old timer when setting timeout=0 (prevents stale timer leak) - Add test covering both default-timeout and timeout=0 timer cleanup
Avoids deprecation warnings in test output. The constructor maps `text` → `message`, so both work, but new tests should use the current API.
JimmyCozza
force-pushed
the
fix/naughty-default-timeout
branch
from
524bc22 to
ae04548
Compare
|
merge conflict fixed |
| -- set_timeout(0) must stop the old timer and nil out self.timer so that | ||
| -- reset_timeout() doesn't accidentally restart a stale reference. | ||
| assert(n_timeout_zero.timer == nil, | ||
| "timer should be nil after setting timeout=0") |
There was a problem hiding this comment.
hm yeah ok. I guess that's an alternative way to prevent the 5s wait from #3626 (comment) but makes the test aware of the internals.
That's not the best, but I guess I'm ok with it 🤷
There was a problem hiding this comment.
Let me think about this one and see if I can find an alternative approach.
There was a problem hiding this comment.
What do you think about a more "black-box" approach? No .timer or ._private checks at all.
For timeout=0: create with timeout=1, set to 0, wait ~1.5s, confirm the notification survives via the destroyed signal. Same pattern for reset_timeout().
For default timeout: call ruled.notification._clear() first so implicit_timeout doesn't mask the bug, then create without explicit timeout and wait for it to actually expire. (I found the previous version passed on master because ruled.notification was setting the timeout via implicit_timeout anyway.)
Tradeoff is a few extra seconds of runtime for real timer waits, and wait_per_step bumped to 8.
- use public n.timeout getter instead of n._private.timeout - use %d format for numeric fields - shorten assert messages to regression labels - drop redundant destroy guards and no-op kill_clients option
4 participants
Fixes #3239 — notifications created without an explicit
timeoutand withoutruled.notificationnever expire.Three bugs in
set_timeout()/create():The constructor guard
if n._private.timeout thenskippedset_timeout()entirely when no timeout was provided, so no timer was ever created and the notification lived forever.The fallback used
cst.config.timeoutwhich resolves tonil— the correct path iscst.config.defaults.timeout.The timer-stop logic was inside the
if timeout > 0branch, so switching totimeout=0left the old timer referenced byself.timer.reset_timeout()could then accidentally restart it.Includes a regression test (
tests/test-naughty-timeout-init.lua) covering default timeout creation, timer cleanup ontimeout=0, andreset_timeout()after clearing.