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:
```
lines=15
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.