Page MenuHomeWildfire Games

GUIManager m_CurrentPage workaround removal
AcceptedPublic

Authored by elexis on Dec 21 2018, 5:35 PM.

Details

Reviewers
wraitii
Trac Tickets
#5369
Summary

rP7214 added a problem: Engine.GetGuiObjectByName is unaware of the caller GUI page.
So GUI pages in the background that still run the onTick and other event code tried to look on the topmost GUI page, rather than their own GUI page.
rP7769 added a workaround that has to be copied to any place that can call JS code.
If developers don't know about the reason for this workaround and add a new place that can call JS code (#5369), they won't be able to implement anything.
Philip added the code comment "kind of ugly". Yet the ScriptInterface SetCallbackData function is used by the GUIManager at least since rP7259 and the same author committed it, so he ought to have
known better.

Test Plan

Verify that the pointers are always set to the intended value, never stale, or otherwise set to null and tested for that.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 6516
Build 10778: Vulcan BuildJenkins
Build 10777: arc lint + arc unit

Event Timeline

elexis created this revision.Dec 21 2018, 5:35 PM

This first revision of the patch was a promising workaround.
But the hope of being careful enough to not run into weird edge cases was broken by the StartGame button in multiplayer gamesetup;
the button calls onTick -> handleNetMessages -> handleGamestartMessage -> SwitchPage. But the code doesn't end after SwitchPage command.
Instead the gamesetup page is removed from the GUIManagers page stack, but the gamesetup page itself is still active and operated on by the onTick function, which now goes on to handle other netmessages, just that it fails for this specific GetGUIObjectByName function.

Vulcan added a subscriber: Vulcan.Dec 21 2018, 5:45 PM

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

Link to build: https://jenkins.wildfiregames.com/job/differential/829/

elexis updated this revision to Diff 7058.Dec 21 2018, 5:47 PM

From that issue and the knowledge that JS variables stored in C++ may have one private data pointer from #5369,
it became clear that the CGUI instance must be saved in some private data field.
Since at least rP7259 there is a property of the ScriptInterface where private data can be stored.
All callers of that function use it in exactly this way, so it should be the most correct solution as far as I see.

elexis updated the Trac tickets for this revision.Dec 21 2018, 5:49 PM

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

Link to build: https://jenkins.wildfiregames.com/job/differential/830/

wraitii accepted this revision.Dec 31 2018, 9:44 AM
wraitii added a subscriber: wraitii.

Great change and going forward might resolve some of the issues I had upgrading to SM60

This revision is now accepted and ready to land.Dec 31 2018, 9:44 AM

If a reviewer doesn't post why the code is correct and complete, one would have to rely on trust that it was actually checked for that, and somehow I don't.
Are there some caveats? Alternative implementations?
Perhaps a place in the code forgotton, or a place in the code that now operates under a different GUI page than it should?
Yes, the patch is still quite simple, but why do we have reviews at all if they aren't reviews.

I've looked at the code and found it sensible, I agree with your reasoning, it fits with what I know of SM, and it compiles. That's already a lot more information than we had before. I'm tired of waiting on 100% complete reviews, feel free to feel differently but I maintain that this patch can imo be merged because I trust in your abilities to write correct patches.

Are there some caveats? Alternative implementations?

The original implementation is an alternative implementation. And it's crap.

Perhaps a place in the code forgotton, or a place in the code that now operates under a different GUI page than it should?

Well then we have a bug and we'll notice and fix it.

Yes, the patch is still quite simple, but why do we have reviews at all if they aren't reviews.

I don't intend to test all cases of GetGUIObjectByName manually to make sure this patch is complete.

elexis retitled this revision from Gratis GUIManager m_CurrentPage workaround removal to GUIManager m_CurrentPage workaround removal.Jan 1 2019, 1:46 PM