Page MenuHomeWildfire Games

Support deleting GUIObject event handlers

Authored by elexis on Oct 27 2019, 5:55 PM.



As reported by nani in the course of his autociv mod development to me on October 12th, it is currently not possible to delete JS handlers once they were assigned (since introduction in rP666).

This is most crucial for onTick handlers that are called every frame. rP23080/D2380 and rP23093/D2390 added TODOs for this aspect.
This diff fixes that.

Test Plan

Verify that the Tracer is present if and only if there are JS functions assigned.
Add a warn() to the onTick functions to see that they are subscribed if and only if they are relevant.
Notice that we dont want a warning or error reported if the deleted event handler doesn't exist, for consistency with deleting properties from JS objects that don't exist.
Notice that there is some duplication in the JSClass functions duplicated here again. Theres some experiment in D2142 to remove it, so if that is addressed, then there.

Diff Detail

rP 0 A.D. Public Repository
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

elexis created this revision.Oct 27 2019, 5:55 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build:

Build failure - The Moirai have given mortals hearts that can endure.

Link to build:

elexis edited the summary of this revision. (Show Details)Oct 27 2019, 6:10 PM
elexis added inline comments.Oct 28 2019, 12:28 PM
350 ↗(On Diff #10213)

This tracer is getting annoying, the idea was to transform the entire codebase to JS::Heap with SM60+, in which case this code in this function would be removed too.

197 ↗(On Diff #10213)

the LowerCase is somewhat dangerous, because every JSInterface function but then not the C++ functions have this check.
Then theres also the XML parsing code which doesn't have the check, but then the triggering code has it again:

void CGUI::SendEventToAll(const CStr& EventName)
	// janwas 2006-03-03: spoke with Ykkrosh about EventName case.
	// when registering, case is converted to lower - this avoids surprise
	// if someone were to get the case wrong and then not notice their
	// handler is never called. however, until now, the other end
	// (sending events here) wasn't converting to lower case,
	// leading to a similar problem.
	// now fixed; case is irrelevant since all are converted to lower.
	const CStr EventNameLower = EventName.LowerCase();
	m_BaseObject.RecurseObject(nullptr, &IGUIObject::ScriptEvent, EventNameLower);

On top of that one can assign event handlers to events that don't exist.

If every IGUIObject class would define the events that can be subscribed to, then one can check that their case is correct and throw errors if the name doesnt exist, rather than pretending that everything went according to the authors intention.
So not here, not now (but also not a forgotton issue and I dont need a ticket for that to remember.)

This revision was not accepted when it landed; it landed in state Needs Review.Oct 28 2019, 12:35 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Owners added a subscriber: Restricted Owners Package.Oct 28 2019, 12:35 PM