BUG: gears.timer {call_now=true} doesn't pass self (ret)

[aarondill]

Output of awesome --version:

Tested in both 4.3 and HEAD

> awesome --version
awesome v4.3 (Too long)
 • Compiled against Lua 5.3.6 (running with Lua 5.3)
 • D-Bus support: ✔
 • execinfo support: ✔
 • xcb-randr version: 1.6
 • LGI version: 0.9.2
> ./build/awesome --version
awesome v4.3-1647-ge6f5c798-dirty (Too long)
 • Compiled against Lua 5.4.6 (running with 0.9.2)
 • API level: 4
 • D-Bus support: yes
 • xcb-errors support: no
 • execinfo support: yes
 • xcb-randr version: 1.6
 • LGI version: /usr/share/lua/5.4/lgi/version.lua
 • Transparency enabled: yes
 • Custom search paths: no

How to reproduce the issue:

local gtimer = require("gears.timer")
local naughty = require("naughty")
local function notify(val) --- A simple shim around naughty to support the V5 or V4 apis
  if naughty.notification then return naughty.notification({ message = tostring(val) }) end
  return naughty.notify({ text = tostring(val) })
end

gtimer.new({
  call_now = true, -- This introduces the broken behavior
  autostart = true, -- Call :start() for me, required to show the correct behavior (through :emit_signal('timeout'))
  timeout = 1, -- A short timeout to easily reproduce the issue
  single_shot = true, -- Only run this once, not needed, but simplifies cleanup after the demonstration
  callback = notify, -- Call notify on timeout, just notifies the first value
})

Actual result:

A notification is received nil from the call_now
A second notification is received 1 second later (due to timeout) table: <address> (the timer)

Expected result:

call_now passes the timer to the callback like :emit_signal('timeout') does, resulting in two identical notifications both displaying the timer's memory address.

Possible implementation:
The code currently looks like this:

    if args.callback then
        if args.call_now then
            args.callback()
        end
        ret:connect_signal("timeout", args.callback)
    end

    if args.single_shot then
        ret:connect_signal("timeout", function() ret:stop() end)
    end

It would be simpler and avoid this sort of issue in the future to do this:

    if args.callback then
        ret:connect_signal("timeout", args.callback)
    end
    if args.call_now then
            ret:emit_signal("timeout")
    end
    if args.single_shot then
        ret:connect_signal("timeout", function() ret:stop() end)
    end

This makes call_now call :emit_signal("timeout") which would pass the timer to args.callback (the only connected listener)

This would also be more future-proof to potential changes to :emit_signal(), as it would make the actual timer behavior (in :start()) and the .call_now behavior match exactly.

alternatively, keep the current code and just call args.callback(ret)

[aarondill]

The current hack to avoid this is:

-gtimer.new({
-  call_now = true,
+local timer = gtimer.new({
+  call_now = false,
  autostart = true,
  timeout = 1,
  single_shot = true,
  callback = notify,
})
+notify(timer)

which adds an additional variable declaration and error-prone call to callback, which call_now is intended to avoid.