Another random thought about using aliases for Event handlers: beware of the return value (because it controls propagation).

(Not an issue here given the type of events in question ;)).

Reviewed 2 of 4 files at r1, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @poire-z and @zwim)

frontend/ui/uimanager.lua line 1105 at r3 (raw file):

        if self._window_stack[i].widget.covers_fullscreen then
            start_idx = i
            if dbg.is_on and i > 1 then

Forgot this one ;).

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @NiLuJe and @poire-z)

frontend/ui/uimanager.lua line 1105 at r3 (raw file):

Logic suggests it should be a touch faster that way than having logger.dbg decide the same thing, assuming that's backed up by profiling.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Frenzie and @poire-z)

frontend/ui/uimanager.lua line 1105 at r3 (raw file):

Possibly, but if nothing else than for consistency's sake, those concerns are left to logger unless the gains are obvious (c.f., my earlier comment about the verbose input event decodes) ;).

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @NiLuJe and @poire-z)

frontend/ui/uimanager.lua line 1105 at r3 (raw file):

Yeah, it would be faster with ad dbg.is_on check, but not really noticeable.
I would say we just should keep in mind that for all logger.xxx calls the arguments are always evaluated, even if the function itself is a noop.

For this special case I have no strong opinion to keep if dbg.is_on or drop it. We have a similar construct in Line 202 (but there the logger call before the if costs more so hmm, I would drop it.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @NiLuJe and @poire-z)

frontend/ui/uimanager.lua line 1105 at r3 (raw file):

I'm not entirely sure what you mean by special case. If it makes no appreciable performance difference it's not a special case to me. ;-)

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @NiLuJe and @poire-z)

frontend/ui/uimanager.lua line 1105 at r3 (raw file):

"This special case" was just a saying and meant nothing special (huh). Exactly spoken I meant in this "this context".

Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @NiLuJe and @poire-z)

frontend/ui/uimanager.lua line 1105 at r3 (raw file):

Specific? ^_^

Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @NiLuJe and @poire-z)

frontend/ui/uimanager.lua line 1105 at r3 (raw file):

I don't want to fool you. But I think we have a communication problem ;-)
I wanted to say:
1.) I would drop it and
2.) don't forget that arguments of functions are always evaluated.

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Frenzie and @poire-z)

frontend/ui/uimanager.lua line 1105 at r3 (raw file):

Yeah, that's kind of my reasoning for dropping it ;).

Namely: the argument.. argument ;p doesn't hold here as they're relatively trivial; the main issue is the fact that it's the sole call behind a dedicated branch...
And that we indeed have two similar constructs, so handling only one doesn't make sense ;).

(Not that I'm advocating for handling both, although I do understand the motivation. I'm mostly against it so as not to set a precedent when it's for such trivial branching).

Reviewed 3 of 3 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Frenzie and @poire-z)