Page MenuHomeWildfire Games

Support deleting GUIObject event handlers
ClosedPublic

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

Details

Summary

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

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
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: https://jenkins.wildfiregames.com/job/vs2015-differential/492/display/redirect

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

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1007/display/redirect

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
source/gui/ObjectBases/IGUIObject.cpp
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.

source/gui/Scripting/JSInterface_IGUIObject.cpp
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