Support event priority in the WML event pump

[Slayer95]

Describe the desired feature

ac717f7 added event priority support to Lua, which reached Stable in 1.14.

Fast-forward today, and customization of WML events is still very precarious ([fire_event][data] was just introduced in 1.17.6). Support for priority attribute in [event] tags would be very helpful and open many possibilities:

• Addons that add high-priority event handlers that use [return] in order to cancel events.
• Support for reliably refactoring routines from monolithic blocks to modularized customizable events.
• Independence of [event] execution timing from file/line placements, in case there are several listeners to the same event name.

[CelticMinstrel]

I don't believe [return] supports cancelling events at all, nor should it. Perhaps [cancel_action] fits that use-case though.

The event handling system does not have a concept of priority as of right now. The commit you linked is a separate Lua-only event handling system that was sort of a hack to work around a lack of Lua API to the actual event handling system. We should definitely support priority in the real event handling system though.

[Slayer95]

I don't believe [return] supports cancelling events at all, nor should it.

I might have used the wrong wording, but this is what I mean:

[event]
   name=moveto
   [fire_event]
      name=return_please
   [/fire_event]
   [message]
     message="Made it back"
   [/message]
[/event]
[event]
   name=return_please
   [return][/return]
[/event]

The idea is not allowing the Made it back message to be displayed. Practical example in inferno8/wesnoth-To_Lands_Unknown@d50081c

The event handling system does not have a concept of priority as of right now. The commit you linked is a separate Lua-only event handling system

Oh! That's an unfortunate state of affairs.

[gfgtdf]

I strongly support adding such a key to events, In particular to give independent addons/modules a way to coordinate their events, when they register handlers to the same event.
In a similar case the priority key from on_event has been proven useful for example when backporting the fixes for WC2 here: df9bbd5#diff-2eab570b800677592a19e1b3550f6acb2d6bc229fd0cf3541bd3a35858cb1b0bR117

I might have used the wrong wording, but this is what I mean:

I think in your case the [return] has no effect

The idea is not allowing the Made it back message to be displayed

I don't think that'd be expected bahviour by most wml developers, and not something that should be changed,

[Slayer95]

I think in your case the [return] has no effect

Well, that's how it works in Stable.

I don't think that'd be expected bahviour by most wml developers, and not something that should be changed,

The current behavior has been documented since 2017. https://wiki.wesnoth.org/index.php?title=InternalActionsWML&diff=58736&oldid=58389

Althought it's apparent that exiting just one event handler is a missing feature.

[CelticMinstrel]

Althought it's apparent that exiting just one event handler is a missing feature.

It might be covered by [break] but I'm not sure. Though that would only be if you used it outside of a loop or switch; maybe it's better to have a separate tag for that which cuts through loops.

I might have used the wrong wording, but this is what I mean:
The idea is not allowing the Made it back message to be displayed. Practical example in inferno8/wesnoth-To_Lands_Unknown@d50081c

While the name may be slightly unfortunate, given that it's already documented and working that way, we probably can't change it.

My interpretation of what you said was more like this:

[event]
   name=moveto
   [return][/return]
[/event]
[event]
   name=moveto
   [message]
     message="Made it back"
   [/message]
[/event]

Where, just like in your example, "Made it back" is never reached. That's why I mentioned [cancel_action], which seems like it might have that kind of effect.

[Slayer95]

My interpretation of what you said was more like this:

I see. In a web context, that would be the difference between Event.preventDefault() (my case) and Event.stopImmediatePropagation() (yours). Both are legitimate features of an event system.

[CelticMinstrel]

I think the real reason [return] behaves the way it does is because the primary use-case for it in my mind was exiting the main event, and the case of nested events just ended up how it ended up.

[gfgtdf]

[cancel_action] doesn't interact with the event queue at all, it basically stops the move (in a move action), There was also the idea to make it also stop attacks, (or recruits when if we get an event that is fired before the unit is placed) but i don't think that was implemented.

The current behavior has been documented since 2017

Hmm okay, i mean i probably won't care that much since i write most of my events in lua these days anyways

[Slayer95]

While the name may be slightly unfortunate, given that it's already documented and working that way, we probably can't change it.

Well, [return] could get a nested: int|'root' attribute.

IMO, the current behavior of aborting the entire event stack (nested=root) is not very useful.

nested=1, would end the current event handler as well as its parent (that's the main use case imo).

As for nested=0, I guess that's the most intuitive meaning of return: just end the current event handler.

nested≥2 would handle more niche cases, and it'd (possibly) not introduce more cost that supporting each of 0, 1 and root.

Anyway... Any more thoughts about priority?

[Pentarctagon]

To be honest I'm confused what's being proposed. It sounds like there's really two features:

• Adding event priority
• Adding a way to exit multiple levels of nested events at once? Or a way to prevent lower priority handlers for an event from executing after a higher priority event does a [cancel_action]?

[Slayer95]

Adding event priority

That's the subject of the issue.

Adding a way to exit multiple levels of nested events at once? Or a way to prevent lower priority handlers for an event from executing after a higher priority event does a [cancel_action]?

Those are tangentially-related use cases, which kinda hijacked this thread.

[CelticMinstrel]

Since on_event supports priority, the feature is also required in order to complete the deprecation of on_event. That means that it should be the third (optional) argument to wesnoth.game_events.add_repeating (which is intended to be mostly a drop-in replacement for on_event) and be an available option in wesnoth.game_events.add.

So, I'm marking this higher priority.

[gfgtdf]

That means that it should be the third (optional) argument to wesnoth.game_events.add_repeating

It is the second argument to on_event thoughso if the main purpose is to be drop in replacement for on_event it should probably be also the second parameter there.

[CelticMinstrel]

So the final signature will be wesnoth.game_events.add_repeating(name, [priority], actions, [on_undo]).

[soliton-]

Just for the record I really dislike our tendency to make difficult to understand lua interfaces by putting optional arguments before required ones or allowing locations as single or multiple arguments. Great if that is theoretically unambiguously possible. Still a bad idea in terms of clarity in my book.

[gfgtdf]

difficult to understand lua interfaces by putting optional arguments before required ones

This is pretty normal in Lua, even the most often used standard function table.insert does it.

[Pentarctagon]

It still makes it harder to understand, whether it's common in lua or not.

[Slayer95]

Might as well deprecate on_event and make it call add_repeating with the most correct signature?

[Pentarctagon]

Deprecation is awaiting #7566

[Slayer95]

OK. Point is that newer APIs should be designed to supercede old APIs, not to mimick them. If the new API would require a bad signature in order to be a drop-in replacement to on_event, then it just shouldn't become such.

Instead, the old API is kept and routed through the new API, and the old API is eventually deprecated, and removed.

[CelticMinstrel]

Might as well deprecate on_event and make it call add_repeating with the most correct signature?

Yes, if we prefer to reorder the arguments, on_event can be built from a closure that does that for you, rather than just assigning the one function to the other.

So, basically, you're correct that we don't need to preserve the same order that on_event used. I don't personally have an issue with doing so, but I'm not particularly attached to it either.

[gfgtdf]

I don't think that a second optional argument is harder to understand. I however think that code like

add_repeating("moveto", function()

.. code






end, 9)

is quite hard to read,

[CelticMinstrel]

That does actually make sense to me as an argument for putting the priority before the function…