Use class syntax for AIConfig page and move it to Gamesetup/Pages subfolder
ClosedPublic
Actions

Authored by elexis on Jan 17 2020, 8:54 PM.

Details

Reviewers: None
Commits: rP23419: Rewrite Gamesetup AIConfig page to use class syntax, decouple settings and move…
Trac Tickets: #5387
#5322

Summary

The purpose of this diff is to
(1) move from procedural to object oriented code for the AIconfig page (#5387)
(2) update the AIconfig page without closing it and reopening it on the same frame
(3) run it in the same page context as the GameSetup page in order to
(3A) reuse the Gamesetup controller classes instead of reimplementing them, and
(3A) provide an example for future gamesetup subpages that will more clearly benefit from running in the same context.
(4) Change AIConfig page to have changes take effect immediately after changing the dropdowns.

This is necessary for the user choice to not be lost in case there are multiple gamesetup controllers (i.e. preparation for #3806).

(5) Hide AIDifficulty, AIBehavior if no AI is assigned and hide AIBehavior for Sandbox AI.

Test Plan

To evaluate the sanity of the approach, consider the following:

It is possible to implement a C++ GUI diff that allows one GUI page to send data to another GUI page. This however may come with its own catches, possibly dangling references, certainly a reimplementation of the onGameAttributesChange and updateGameAttributes code and hardcoding of references from one GUI page to another (misleading devs that they can code in that one page without considering other pages). The network dialog in D1746 also needed to be updated from the session. For that feature I implemented it using this C++ method:

diff --git a/source/gui/scripting/JSInterface_GUIPage.cpp b/source/gui/scripting/JSInterface_GUIPage.cpp
new file mode 100644
index 0000000000..ed39ac195e
--- /dev/null
+++ b/source/gui/scripting/JSInterface_GUIPage.cpp
@@ -0,0 +1,99 @@
+/* Copyright (C) 2018 Wildfire Games.
+ * This file is part of 0 A.D.
+ *
+ * 0 A.D. is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 0 A.D. is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with 0 A.D.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "precompiled.h"
+
+#include "JSInterface_GUIPage.h"
+
+#include "gui/CGUI.h"
+#include "gui/GUIManager.h"
+#include "scriptinterface/ScriptInterface.h"
+
+JSClass JSI_GUIPage::JSI_class = {
+	"GUIPage", JSCLASS_HAS_PRIVATE,
+	nullptr, nullptr,
+	JSI_GUIPage::GetProperty, JSI_GUIPage::SetProperty,
+	nullptr, nullptr, nullptr, nullptr,
+	nullptr, nullptr, nullptr, nullptr
+};
+
+void JSI_GUIPage::RegisterScriptClass(ScriptInterface& scriptInterface)
+{
+	scriptInterface.DefineCustomObjectType(&JSI_class, nullptr, 1, nullptr, nullptr, nullptr, nullptr);
+}
+
+bool JSI_GUIPage::GetProperty(JSContext* cxSource, JS::HandleObject obj, JS::HandleId id, JS::MutableHandleValue vp)
+{
+	JSAutoRequest rqSource(cxSource);
+
+	JS::RootedValue idval(cxSource);
+	if (!JS_IdToValue(cxSource, id, &idval))
+		return false;
+
+	std::string functionName;
+	if (!ScriptInterface::FromJSVal(cxSource, idval, functionName))
+		return false;
+
+	JS::RootedObject parent(cxSource);
+	JS::Rooted<JSFunction*> function(cxSource, JS_NewFunction(cxSource, JSI_GUIPage::CallFunction, 1, 0, obj, functionName.c_str()));
+	vp.setObject(*JS_GetFunctionObject(function));
+	return true;
+}
+
+bool JSI_GUIPage::SetProperty(JSContext* cx, JS::HandleObject UNUSED(obj), JS::HandleId UNUSED(id), bool UNUSED(strict), JS::MutableHandleValue UNUSED(vp))
+{
+	JS_ReportError(cx, "Page settings are immutable.");
+	return true;
+}
+
+bool JSI_GUIPage::CallFunction(JSContext *cxSource, unsigned argc, JS::Value *vp)
+{
+	// Determine function name
+	JSAutoRequest rqSource(cxSource);
+	JS::CallArgs args = JS::CallArgsFromVp(argc, vp);
+	JS::RootedValue functionNameValue(cxSource, JS::StringValue(JS_GetFunctionId(JS_ValueToFunction(cxSource, args.calleev()))));
+	std::string functionNameString;
+	ScriptInterface::FromJSVal(cxSource, functionNameValue, functionNameString);
+
+	// Determine target GUI page from the private data
+	JS::RootedObject thisObj(cxSource, &args.thisv().toObject());
+	CGUI* guiPage = static_cast<CGUI*>(JS_GetInstancePrivate(cxSource, thisObj, &JSI_GUIPage::JSI_class, nullptr));
+
+	if (!g_GUI->IsPageOpen(guiPage))
+	{
+		JS_ReportError(cxSource, "GUIPage is not open.");
+		return true;
+	}
+
+	{
+		ScriptInterface* scriptInterfaceSource = ScriptInterface::GetScriptInterfaceAndCBData(cxSource)->pScriptInterface;
+		JSContext* cxDestination = guiPage->GetScriptInterface()->GetContext();
+		JSAutoRequest rqDestination(cxDestination);
+		JS::RootedValue global(cxDestination, guiPage->GetGlobalObject());
+		JS::RootedValue argument(cxDestination, argc > 0 ? guiPage->GetScriptInterface()->CloneValueFromOtherContext(*scriptInterfaceSource, args[0]) : JS::UndefinedValue());
+		JS::RootedValue returnValueDestination(cxDestination);
+
+		// Call the function of the determined name in the context of the other page
+		guiPage->GetScriptInterface()->CallFunction(global, functionNameString.c_str(), &returnValueDestination, argument);
+
+		// Clone return value
+		JS::RootedValue returnValueSource(cxSource, scriptInterfaceSource->CloneValueFromOtherContext(*guiPage->GetScriptInterface(), returnValueDestination));
+		JS::CallReceiverFromVp(vp).rval().set(returnValueSource);
+	}
+
+	return true;
+}

Most importantly running it in the same GUI context allows to reuse all handlers. This may not be relevant for the AIConfig page, but for the future planned Gamesetup subpages it is relevant: The MapBrowser for example can reuse the same MapCache and thus avoid having to read all XML files (where some of them are 10mb) when opening the page. The map files could be cached in C++ too, but that would make it harder to distinguish when they can be cleaned and when not (the cache would have to be wiped explicitly instead of implicitly upon page close).

A second gamesetup subpage example is the the dialog to enter a handicap per player (StartingResources, StartingTechs, Pop limit). If that was a separate GUI page as well, there would be yet another onGameAttributes interface instead of reusing the GameSettingsController.

A third gamesetup subpage example is the dialog to select the regicide hero (or which ever other features that civ selection dialog that s0600204 implemented that that Delenda Est used implemented, I didn't get to that yet.)

Another advantage of this patch (unrelated to running the subpage in the same Gamesetup page) is the furthering of separation of concerns and decoupling of settings: The AISelection, AIDifficulty and AIBehavior are currently tightly coupled. For example:

-		// Enforce map specified AI
-		if (this.fixedAI &&
-			(pData.AI !== this.fixedAI.AI ||
-			 pData.AIDiff !== this.fixedAI.AIDiff ||
-			 pData.AIBehavior !== this.fixedAI.AIBehavior))
-		{
-			pData.AI = this.fixedAI.AI;
-			pData.AIDiff = this.fixedAI.AIDiff;
-			pData.AIBehavior = this.fixedAI.AIBehavior;
-			this.gameSettingsControl.updateGameAttributes();
-		}

This may be in the order of magnitude of who cares with settingcount=3, but it's a clear antipattern that was just removed in D2483/rP23374.
This may actually result in more code, but logically separated code.
An AIgamesetting class may be deleted without affecting other AIGamesettings that don't depend on it.
More importantly it also allows mods to insert new AIGamesettings without having to overwrite the AIConfigButton which currently handles all three properties.
(This is similar to the selectMap code prior to the gamesetup class rewrite which hardcoded which settings had to be deleted when changing the map, thus making it impossible for mods to insert a new setting without overwriting selectMap (which in turn made it impossible to run two mods at the same time that each inserted a new setting).
Thus it is pivotal to fix this antipattern.

Clear and not so insignificant disadvantages of this rewrite are:

Some more lines of code.
Running it in the same GUI page, and that gamesetup GUI page having GUI objects which hardcode some Z indices means that the the dialog may not be rendered on top and some random GUI objects of the gamesetup page are shown on top of the AIconfig page.
Needs to move a bunch of files to subfolders. Sorry for not having anticipated that.

Diff Detail

Repository

rP 0 A.D. Public Repository

Branch

/ps/trunk

Lint

Lint OK

Unit

No Unit Test Coverage

Build Status

Buildable 10855
Build 18977: Vulcan Build	Jenkins
Build 18976: Vulcan Build (macOS)	Jenkins
Build 18975: Vulcan Build (Windows)	Jenkins
Build 18974: arc lint + arc unit

Event Timeline

elexis created this revision.Jan 17 2020, 8:54 PM

elexis updated the Trac tickets for this revision.Jan 17 2020, 8:55 PM

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

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

Harbormaster failed remote builds in B10855: Diff 11077!Jan 17 2020, 8:55 PM

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/1074/display/redirect

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

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/170/display/redirect

elexis edited the test plan for this revision. (Show Details)Jan 17 2020, 9:06 PM

asterix awarded a token.Jan 18 2020, 11:33 AM

elexis mentioned this in D2581: Refactor gamesetup page to support subpages.Jan 18 2020, 3:23 PM

elexis mentioned this in rP23413: Gamesetup subpages support..Jan 18 2020, 8:43 PM

elexis retitled this revision from Use class syntax for AIConfig page and move it to GamesetupPage to Use class syntax for AIConfig page and move it to Gamesetup/Pages subfolder.Jan 18 2020, 9:13 PM

Rebase following rP23413 and remove more hardcodings.

As mentioned in D2581, it is necessary to run the AIConfig page in the same script context as the GameSetupPage if one wants exclusively that page to be in charge of the AI attributes in g_GameAttributes.settings.PlayerData, since the page can't run code if its closed, but it can if its only hidden.
Not doing it that way would mean there would be a class in the AIConfig page manifesting the AI setting logic and another class in the Gamesetup page coding the same logic.
Second reason to perform this change is as mentioned the existing coupling between the Gamesetup page and subpages, which is a pattern, and thus refactoring this page to become a subpage means the pattern becomes an example of how to implement Gamesetup subpages, not a counterexample, with the new subpages being empowered and encouraged to reuse the existing gamesetup controlers (gameSettingsControl, playerAssignmentsControl, netMessages, mapCache, mapFilters, ...).

This revision was not accepted when it landed; it landed in state Needs Review.Jan 20 2020, 10:30 AM

This revision was landed with ongoing or failed builds.

Closed by commit rP23419: Rewrite Gamesetup AIConfig page to use class syntax, decouple settings and move… (authored by elexis). · Explain Why

This revision was automatically updated to reflect the committed changes.

elexis added a commit: rP23419: Rewrite Gamesetup AIConfig page to use class syntax, decouple settings and move….

Owners added a subscriber: Restricted Owners Package.Jan 20 2020, 10:30 AM

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/Pages/AIConfigPage/Controls/AIBehavior.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/Pages/AIConfigPage/Controls/AIBehavior.js
|  84|  84| 		this.gameSettingsControl.updateGameAttributes();
|  85|  85| 		this.gameSettingsControl.setNetworkGameAttributes();
|  86|  86| 	}
|  87|    |-}
|    |  87|+};
|  88|  88| 
|  89|  89| AIGameSettingControls.AIBehavior.prototype.ConfigBehavior =
|  90|  90| 	"gui.gamesetup.aibehavior";

binaries/data/mods/public/gui/gamesetup/Pages/AIConfigPage/Controls/AIBehavior.js
|  87| }
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.

binaries/data/mods/public/gui/gamesetup/Pages/AIConfigPage/AIDescription.js
|  31| »   »   let·AI·=·g_Settings.AIDescriptions.find(AI·=>·AI.id·==·pData.AI);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'AI' is already declared in the upper scope.
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/Pages/GameSetupPage/GameSetupPage.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/Pages/GameSetupPage/GameSetupPage.js
|  46|  46| 
|  47|  47| 		Engine.ProfileStop();
|  48|  48| 	}
|  49|    |-}
|    |  49|+};

binaries/data/mods/public/gui/gamesetup/Pages/GameSetupPage/GameSetupPage.js
|  49| }
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/Pages/LoadingPage/LoadingPage.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/Pages/LoadingPage/LoadingPage.js
|  22|  22| 		loadingPage.hidden = true;
|  23|  23| 		Engine.GetGUIObjectByName("setupWindow").hidden = false;
|  24|  24| 	}
|  25|    |-}
|    |  25|+};

binaries/data/mods/public/gui/gamesetup/Pages/LoadingPage/LoadingPage.js
|  25| }
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/Pages/AIConfigPage/AIConfigPage.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/Pages/AIConfigPage/AIConfigPage.js
|  60|  60| 	{
|  61|  61| 		this.aiConfigPage.hidden = true;
|  62|  62| 	}
|  63|    |-}
|    |  63|+};
|  64|  64| 
|  65|  65| SetupWindowPages.AIConfigPage.prototype.AIGameSettingControlOrder = [
|  66|  66| 	"AISelection",

binaries/data/mods/public/gui/gamesetup/Pages/AIConfigPage/AIConfigPage.js
|  63| }
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/Pages/GameSetupPage/Panels/GameSettingWarning.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/Pages/GameSetupPage/Panels/GameSettingWarning.js
|  16|  16| 		let caption =
|  17|  17| 			g_GameAttributes.settings.CheatsEnabled ?
|  18|  18| 				this.CheatsEnabled :
|  19|    |-			g_GameAttributes.settings.RatingEnabled ?
|    |  19|+				g_GameAttributes.settings.RatingEnabled ?
|  20|  20| 				this.RatingEnabled :
|  21|  21| 				"";
|  22|  22| 
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 5 tabs but found 4.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/Pages/GameSetupPage/Panels/GameSettingWarning.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/Pages/GameSetupPage/Panels/GameSettingWarning.js
|  17|  17| 			g_GameAttributes.settings.CheatsEnabled ?
|  18|  18| 				this.CheatsEnabled :
|  19|  19| 			g_GameAttributes.settings.RatingEnabled ?
|  20|    |-				this.RatingEnabled :
|    |  20|+					this.RatingEnabled :
|  21|  21| 				"";
|  22|  22| 
|  23|  23| 		this.gameSettingWarning.caption = caption;
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 5 tabs but found 4.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/Pages/GameSetupPage/Panels/GameSettingWarning.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/Pages/GameSetupPage/Panels/GameSettingWarning.js
|  18|  18| 				this.CheatsEnabled :
|  19|  19| 			g_GameAttributes.settings.RatingEnabled ?
|  20|  20| 				this.RatingEnabled :
|  21|    |-				"";
|    |  21|+					"";
|  22|  22| 
|  23|  23| 		this.gameSettingWarning.caption = caption;
|  24|  24| 		this.gameSettingWarning.hidden = !caption;
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/Pages/AIConfigPage/Controls/AISelection.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/Pages/AIConfigPage/Controls/AISelection.js
|  80|  80| 		this.gameSettingsControl.updateGameAttributes();
|  81|  81| 		this.gameSettingsControl.setNetworkGameAttributes();
|  82|  82| 	}
|  83|    |-}
|    |  83|+};
|  84|  84| 
|  85|  85| AIGameSettingControls.AISelection.prototype.NoAI = {
|  86|  86| 	"Title": translateWithContext("ai", "None"),

binaries/data/mods/public/gui/gamesetup/Pages/AIConfigPage/Controls/AISelection.js
|  83| }
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.
Executing section cli...

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

Harbormaster completed remote builds in B10888: Diff 11115.Jan 20 2020, 10:32 AM