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.
Details
- Reviewers
wraitii - Commits
- rP22200: Remove workaround in GetGUIObjectByName
- Trac Tickets
- #5369
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
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Event Timeline
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.
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/829/
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.
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/830/
Great change and going forward might resolve some of the issues I had upgrading to SM60
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.