Opened 5 years ago
Last modified 3 years ago
#5387 new defect
Rewrite the JS GUI to use object-orientation
Reported by: | elexis | Owned by: | |
---|---|---|---|
Priority: | Should Have | Milestone: | Backlog |
Component: | UI – Miscellaneous | Keywords: | |
Cc: | Krinkle | Patch: |
Description
Object-oriented programming advantages, Example simulation components: The pyrogenesis simulation system is very solid and clarly arranged, because it uses object-oriented programming. Every property an entiy can have is stored in a JS prototype instance. This allows a reader or writer to the codebase to understand a single component without having to understand all the other components (few dependent components only).
In contrast, procedural programming has been used in most of the JS GUI part.
This means all functions are global, most states are global. So they are represented as peers when in fact they might be entirely unrelated. This is confusing to the one who reads the code for the first time, because he has to infer where the logically related parts are located. But much worse is the fact that the developers who write to the codebase for the first time, even the ones who refactored the code already a hundred times have trouble identifying logical boundaries for the entire codebase (spaghetti code). So when writing to the codebase, it is likely the code becomes even more chaotic (code degradation). So the advantages mentioned for the simulation system only benefit the JS GUI once it's written to use OOP.
Patterns / Anti-patterns:
- https://en.wikipedia.org/wiki/Object-oriented_programming
- https://en.wikipedia.org/wiki/Separation_of_concerns
- https://en.wikipedia.org/wiki/Information_hiding
- https://en.wikipedia.org/wiki/Software_rot
Example GUI session/
:
For example consider Object.properties(global)
of the session/
folder as of alpha 23:
acceptTerms ACTION_GARRISON ACTION_GUARD ACTION_NONE ACTION_PATROL ACTION_REPAIR addChatMessage addNetworkWarning addResearchToQueue addTrainingByPosition addTrainingToQueue appendSessionCounters armorLevelToPercentageString attackRateDetails AuraTemplates autoCompleteNick backToWork barterOpenCommon barterUpdateCommon basename bicubicInterpolation bodyFont BUTTON_HEIGHT calculateCarriedResources cancelOnLoadGameError cancelPackUnit cancelUpgradeEntity canMoveSelectionIntoFormation changeGameSpeed changePrimarySelectionGroup chatMenuButton checkTerms clampColorValue clearChatMessages clearSelection clearTimeout clickedEntity clone closeChat closeDiplomacy closeMenu closeObjectives closeOpenDialogs closePageHack closeTrade colorDistance coloredText colorFade_attackUnit colorizeAutocompleteHotkey colorizeHotkey colorizePlayernameByGUID colorizePlayernameByID colorizePlayernameHelper colorizePlayernameParameters commaFont comparedModsString compatibilityColor confirmExit controlsPlayer createPanelEntityTooltip cubicInterpolation currIdleClassIndex DamageTypes damageTypesToText deepfreeze deleteGame deleteTooltip DeriveModificationsFromTech DeriveModificationsFromTechnologies DeriveTechnologyRequirements determineAction diplomacyFormatAttackRequestButton diplomacyFormatSpyRequestButton diplomacyFormatStanceButtons diplomacyFormatTributeButtons diplomacyMenuButton diplomacySetupTexts dirname displayGamestateNotifications displayMultiple displayPanelEntities displaySingle doAction DoesModificationApply doublePressTime doublePressTimer DrawTargetMarker END_MENU_POSITION endOfAlert Engine EntityGroups EntityGroupsContainer EntitySelection error escapeText executeCheat executeNetworkCommand exitMenuButton fadeColorTick filesizeToObj filesizeToString findGatherType findGuidForPlayerID findIdleUnit flushTrainingBatch formatAttackMessage formatBarterMessage formatBatchTrainingString formatChatCommand formatDefeatVictoryMessage formatDiplomacyMessage formatLimitString formatPhaseMessage formatPlayerInfo formatTributeMessage formatTributeTooltip g_AdditionalHighlight g_allBuildableEntities g_allTrainableEntities g_AllyEntityCommands g_AlwaysDisplayFriendlyFire g_Ambient g_AttackTypes g_AvailableFormations g_BarterActions g_BarterMultiplier g_BarterResourceSellQuantity g_BarterSell g_BatchSize g_BatchTrainingEntities g_BatchTrainingEntityAllowedCount g_BatchTrainingType g_BiomesDirectory g_BribeButtonsWaiting g_Buddies g_BuddyListDelimiter g_BuddySymbol g_canMoveIntoFormation g_Ceasefire g_ChatAddresseeContext g_ChatCommands g_ChatHistory g_ChatHistoryFilters g_ChatLines g_ChatMessages g_ChatTimeout g_ChatTimers g_Cheats g_CivData g_CivInfo g_ColorFade g_ConfirmExit g_DamageTypes g_DefaultLobbyRating g_DefaultPopulationColor g_DescriptionHighlight g_DevSettings g_DiplomacyColorsToggle g_DiplomacyMessages g_Disconnected g_DisplayedPlayerColors g_DragStart generateSavegameDateString generateSavegameLabel g_EntityCommands g_EntityStates getActionInfo getAllBuildableEntities getAllBuildableEntitiesFromSelection getAllTrainableEntities getAllTrainableEntitiesFromSelection getAllyStatTooltip getArmorTooltip getAttackTooltip getAurasTooltip GetBaseTemplateDataValue getBatchTrainingSize getBiomePreview getBuildingsWhichCanTrainEntity getBuildString getBuildTimeTooltip getCheatsData getCostTypes getCurrentHealthTooltip getDefaultBatchTrainingSize getDescriptionTooltip getDisconnectReason getEntityCostComponentsTooltipString getEntityCostTooltip getEntityLimitAndCount getEntityNames getEntityNamesFormatted getEntityOrHolder GetEntityState getEntityTooltip getGameDescription getGameSpeedChoices getGarrisonTooltip getGatherTooltip getHealerTooltip getHealthTooltip getHistoryTooltip getHotloadData GetIdentityClasses getIdleLandTradersText getIdleShipTradersText getInitColorFadeRGB getLocalizedResourceAmounts getLootTooltip getMapDescriptionAndPreview GetModifiedTemplateDataValue GetMultipleEntityStates getNeededResourcesTooltip getNetworkWarnings getNumberOfRightPanelButtons getPlayerHighlightColor getPopulationBonusTooltip getPreferredEntities getProjectilesTooltip getRepairTimeTooltip getReplayMetadata getRequiredTechnologyTooltip getResourceTrickleTooltip getResourceTypeDisplayName getSavedGameData getSecondsString GetSimState getSpeedTooltip getSplashDamageTooltip getStanceDisplayName getStanceTooltip GetTechModifiedProperty GetTechnologyBasicDataHelper GetTechnologyData GetTechnologyDataHelper GetTemplateData GetTemplateDataHelper getTermsHash getTradingTooltip getTrainingStatus getUsernameList getValidPort getVisibleEntityClassesFormatted GetVisibleIdentityClasses getWallPieceTooltip g_FadeAttackUnit g_FlushTributing g_FollowPlayer g_FormatChatMessage g_FormationsInfo g_FreehandSelection_InputLine g_FreehandSelection_MinLengthOfLine g_FreehandSelection_MinNumberOfUnits g_FreehandSelection_ResolutionInputLineSquared g_GameAttributes g_GameSpeeds g_Groups g_HasRejoined g_HighlightedAlpha g_HotkeyTags g_IdleTraderTextColor g_IsChatAddressee g_IsController g_IsDiplomacyOpen g_IsMenuOpen g_IsNetworked g_IsNetworkedActive g_IsObjectivesOpen g_IsObserver g_IsReplay g_IsTradeOpen g_IsTrainingBlocked g_JumpCameraLast g_JumpCameraPositions g_LastChatAddressee g_LastTickTime g_LoadDescription g_MapPreviewPath g_MapSizes g_MapTypes g_MaxDragDelta g_MaxPlayers g_MaxSelectionSize g_MaxTeams g_MessageBoxBtnFunctions g_MessageBoxCallbackArgs g_MilitaryTypes g_NetMessageTypes g_NetworkCommands g_NetworkWarnings g_NetworkWarningTexts g_NetworkWarningTimeout g_NotificationsTypes g_NumberOfBatches g_OnSelectTab g_PanelEntities g_PanelEntityOrder g_PanelsOrder g_PatrolTargets g_Paused g_PausingClients g_PlayerAssignments g_Players g_PlayerStateMessages g_PluralTranslations g_PluralTranslationsWithContext g_PopulationAlertColor g_PopulationCapacities g_Progress g_RangeTooltipString g_ReplaySelectionData g_ResearchListTop g_ResourceData g_ResourceTitleFont g_Selection g_SelectionPanels g_Settings g_SettingsDirectory g_ShowAllStatusBars g_ShowGuarded g_ShowGuarding g_ShowGUI g_SimState g_SoundNotifications g_SplashDamageTypes g_StartingResources g_StatusBarUpdate g_StatusMessageTypes g_SummarySelectedData g_TabCategoryCount g_TabCategorySelected g_TargetMarker g_TechnologyData g_TemplateData g_Terms g_Time g_TimerID g_Timers g_TooltipTextFormats g_Translations g_TranslationsWithContext g_TutorialMessages g_TutorialNewMessageTags guiToRgbColor g_UnitActions g_unitPanelButtons g_ValidPorts g_VictoryConditions g_VictoryDurations g_ViewedPlayer g_WorkerTypes handleClientsLoadingMessage handleInputAfterGui handleInputBeforeGui handleMinimapEvent handleNetMessages handleNetStatusMessage handleNotifications handlePlayerAssignmentsMessage hasClass hasSameEngineVersion hasSameMods hasSameRestrictionCategory headerFont heapsPermute hideRemaining hideUnitCommands horizontallySpaceObjects hslToRgb init initBarterButtons initChatWindow initGUIObjects initializeMusic initMenu initMusic initPanelEntities initTerms initViewedPlayerDropdown INPUT_BANDBOXING INPUT_BATCHTRAINING INPUT_BUILDING_CLICK INPUT_BUILDING_DRAG INPUT_BUILDING_PLACEMENT INPUT_BUILDING_WALL_CLICK INPUT_BUILDING_WALL_PATHING INPUT_MASSTRIBUTING INPUT_NORMAL INPUT_PRESELECTEDACTION INPUT_SELECTING inputState INPUT_UNIT_POSITION INPUT_UNIT_POSITION_START InterpretTechRequirements INVALID_ENTITY isColorFadeRunning isCompatibleSavegame isPlayerObserver isTranslatableString isUndeletable jumpCamera kickError kickObservers kickPlayer lastIdleClasses lastIdleUnit layoutSelectionMultiple layoutSelectionSingle leaveGame listFiles loadAIBehaviors loadAIDescriptions loadAIDifficulties loadBiomes loadCeasefire loadCivData loadCivFiles loadMapTypes LoadModificationTemplates loadPlayerDefaults loadPopulationCapacities loadSettingsValues loadSettingValuesFile loadTermsAcceptance loadVictoryConditions loadVictoryDuration lobbyDialogButton lockGate log MARGIN markForPluralTranslation markForTranslation markForTranslationWithContext MatchesClassList matchUsername MENU_BOTTOM MENU_SPEED MENU_TOP messageBox messageBoxCallbackFunction ModificationTemplates modsToString mouseIsOverObject mouseX mouseY multiplayerName multiplyEntityCosts music Music NetworkDialogManager NUM_BUTTONS onClientJoin onClientLeave onNetworkOutOfSync onReplayFinished onReplayOutOfSync onSelectionChange onSimulationUpdate onTick onToggleChatWindowExtended OnTrainMouseWheel onWindowResized openChat openDeleteDialog openDialog openDiplomacy openGameSummary openManual openMenu openObjectives openOptions openSave openStrucTree openTerms openTrade openURL optionsMenuButton optionsPageClosed packUnit parseChatAddressee pauseGame pauseMenuButton performAllyCommand performCommand performFormation performGroup performStance pickRandom placementSupport PlacementSupport placeTabButtons playAmbient playerCheck playerDataToStringifiedTeamList playersFinished _playSound positionUnitsFreehandSelectionMouseMove positionUnitsFreehandSelectionMouseUp prepareForDropdown preSelectedAction prevHotkey print raiseAlert randBool randFloat randIntExclusive randIntInclusive randomAngle randomNormal2D reallyDeleteGame recalculateStatusBarDisplay removeDupes removeFiltersFromTemplateName removeFromProductionQueue removeGuard removeOldChatMessage reportDisconnect reportGame resetIdleUnit resignGame resignMenuButton resignQuestion resizeChatWindow resizeDiplomacyDialog resizeTradeDialog resourceIcon resourceNameFirstWord resourceNameWithinSentence Resources resourcesToAlphaMask restartColorFade restoreSavedGameData resumeGame resumeGameAndSaveSummarySelectedData rgbToGuiColor rgbToHsl sameColor saveSettingAndWriteToUserConfig SDL_BUTTON_LEFT SDL_BUTTON_MIDDLE SDL_BUTTON_RIGHT SDLK_LALT SDLK_LCTRL SDLK_LEFTBRACKET SDLK_LSHIFT SDLK_RALT SDLK_RCTRL SDLK_RIGHTBRACKET SDLK_RSHIFT selectAndMoveTo selectNextTab selectPanel selectViewPlayer sendDialogAnswer sendLobbyPlayerlistUpdate setCameraFollow setClientPauseState _setHighlight setJumpCamera setMapPreviewImage _setMotionOverlay setNewTimerFunction setOutcomeIcon setPanelObjectPosition _setStatusBars setStringTags setTimeout setupUnitPanel showTemplateDetails showTemplateViewerOnClickTooltip showTemplateViewerOnRightClickTooltip showTimeWarpMessageBox shuffleArray singleplayerName smoothColorFadeRestart_attackUnit someCanPatrol someGuarding someRallyPoints someUnitAI sortDecreasingDate sortGUIDsByPlayerID sortNameIgnoreCase soundNotification splitRatingFromNick sprintf startBuildingPlacement startColorFade STEP stopColorFade stopUnits storeCivInfoPage stringifiedTeamListToPlayerData submitChatDirectly submitChatInput TechnologyTemplates timeToString toggleChangePerspective toggleConfigBool toggleDeveloperOverlay toggleDiplomacy toggleGameSpeed toggleGUI toggleMenu toggleObjectives togglePause toggleRangeOverlay toggleTrade toggleTutorial TradeGain TradeGainNormalization tradingGainString translate translateAIBehavior translateAIDifficulty translateAIName translateAISettings translateMapSize translateMapTitle translateMapType translateMessageObject translateObjectKeys translatePlural translatePluralWithContext translatePopulationCapacity translateVictoryCondition translateWithContext tryAutoComplete tryPlaceBuilding tryPlaceWall unescapeText unitFilters unitFont unloadAll unloadSelection unloadTemplate UnravelPhases updateAdditionalHighlight updateBandbox updateBarterButtons updateBuildingPlacementPreview updateChatAddressees updateChatHistory updateCinemaPath updateCounters updateCursorAndTooltip updateDebug updateDefaultBatchSize updateDiplomacy updateDiplomacyColorsButton updateDisplayedPlayerColors updateEnabledRangeOverlayTypes updateGameSpeedControl updateGarrisonHealthBar updateGroups updateGUIObjects updateGUIStatusBar updateHotkeyTooltips updateIdleWorkerButton updateMenuPosition updatePanelEntities updatePauseOverlay updatePlayerData updatePlayerDisplay updateResearchDisplay updateSelectionDetails updateTimeNotifications updateTimers updateTopPanel updateTraderTexts updateTutorial updateUnitCommands updateViewedPlayerDropdown upgradeEntity UPGRADING_CHOSEN_OTHER UPGRADING_NOT_STARTED Vector2D Vector2Dprototype Vector3D Vector3Dprototype vsprintf warn
These are 682 globals! The reader now has to either use circumstances (functions being located near a file) to identify relations (which is often unreliable and easy to do sub-ideal); or the reader has to read all callstacks of the code to figure out how that works. To get the entire callstack with this spaghetti procedural code, he soon will turn out to have to read half the code base to get what's going on (depending on the most recent state of refactoring and the most recent state of newly introduced spaghetti).
To get a visual indication of the code organization issue, one can also draw the graph where each vertex is a JS object and each edge indicates that the object is a property of the other object. This yields a tree / hierarchy (as the codebase avoids circles as far as I know).
In the above example the tree would have the global
root node that owns 682 nodes (or similar).
But, hypothetically, the code could be refactored to use about 40 prototypes that each have about 15 properties (states and functions).
This way said tree would be much more harmonic, the reader can cut out much larger chunks of understanding of the codebase to understand the area of code relevant to his concern.
Even more importantly, the writers to the code base will stop adding procedural code when the entire other codebase uses OOP.
This way the spaghetti problem will not only be fixed for the existing codebase but also change the behavior of future developers by giving better example codes that they can orient at.
Precedents rmgen / gamesetup:
In #5322 the same issue was reported for the gamesetup specifically, which in 2015 was the best demonstration of code that works correctly to the user but prevents the readers from understanding it and developers from adding features that are wished since decades.
In #5042 the same issue was reported for random maps. The rmgen core was already OOP (matei, historic_bruno...), but many new library functions were added in procedural style later (spahbod, FeXoR, elexis, ...).
Proof of concept / examples:
- The change in Phab:D322 is a good example of how the code looked before and how it looks afterwards (albeit not using prototypes) and it marked a milestone, allowing for iteration over gamesettings that enabled the addition of many more features and made the code easy enough so that new developers had only one hunk of 20 lines of code to read to understand one gamesetting, rather than 10 functions spread throughout the file.
- Phab:D1746 demonstrates how new JS GUI code can be implemented using prototypes.
Implementation time consumption and review: The rmgen cleanup in alpha 23 (nuking duplication #4805, relocating #4804, vector algebra #4845, #4992, OOP #5042) consumed about 8 git refactoring intermediary branches and from September 2017 to April 2018 with no or almost no review and many hours of daily work. The gamesetup cleanups in late 2015 (50 relocation / duplication revmoal commits) and 2016 (Phab:D322 OOP, duplication removal) consumed maybe half a year of working time without reviews. I believe there should be a review of the plan, so that we're all in the same boat and sail towards the same goal. But given that the gamesetup is only one of maybe 20 GUI pages and that the gamesetup refactoring already consumed half a year of unreviewed work, given that the rmgen cleanup consumed about 8 months without review, and given that a mandatory upload or review rule would at the very best introduce a 24 hour delay per commit, not rarely an indefinite delay, this ticket will not be possible to realize with a mandatory review policy, and consume maybe an additional year of lifetime with a mandatory patch upload before commit policy to complete. (Not to mention hundreds of differential revision proposals that only contain the same qualitative change, just for many lines, i.e. would yield a bad signal/noise ratio for Phabricator and distract people from reading revision proposals where people actually need help with non-trivial changes.)
So despite unpermitted renames of two variables changes were considered extremely terrible in at least one instance (r20552), I find myself in a situation where I can't satisfy both a review / upload rule and complete this ticket in the remaining time that is provided to me. So I can either offer to comply with any review or upload rule or offer to work on this ticket, possibly offering to upload specific patches before a commit, but not all of them. See the internal discussion about a mirror of this problem https://wildfiregames.com/forum/index.php?/topic/25169-post-a23-commit-frenzy/ which so far concluded in guidelines not being rules.
Change History (39)
comment:1 by , 5 years ago
comment:2 by , 5 years ago
Model/view/controller pattern and alike will become apparent once things are better encapsulated. Just by splitting / grouping into separate objects, the patterns will emerge already (for example many prototypes that only display things but get their data (model) from elsewhere). Once it's laid out, we can consider fancy rewiring on top of it. Most of the global GUI variables will become properties of the objects, so no use in renaming them in advance, but thanks for the offer.
comment:3 by , 5 years ago
Sounds like a good idea in general. Also as you said, the last gamesetup rewrites already went in that direction.
But of course if the changes are good or bad depends on the specific object model/order that is chosen for each GUI page.
comment:4 by , 5 years ago
Component: | UI & Simulation → UI – Miscellaneous |
---|
comment:5 by , 5 years ago
I think, as I've said on some diffs (for example D1837 or D1927) that we should move to a more reactive GUI.
I think ideally GUI objects would be decorated with objects that handle their layout, instead of doing that imperatively elsewhere, and the "imperative" code just sets the state for these objects and the objects re-render (cleverly, or not, as needed/implemented).
This isn't very different from what you mean above, since we'll automatically split stuff into their own objects then.
comment:6 by , 5 years ago
Cc: | added |
---|
comment:9 by , 5 years ago
In r22370 by Vladislav:
Refactors Developer Overlay using an object instead of global functions.
Reviewed By: wraitii
Commented By: elexis
Differential Revision: https://code.wildfiregames.com/D1928
comment:12 by , 5 years ago
The XmppClient and NetClient GUI message handlers can be called from C++ rather than by pulling in onTick
: #5585.
comment:27 by , 5 years ago
Benefitial object oriented programming patterns:
Disclaimer: These are neither rules nor guidelines, but reoccurring patterns that present themselves following the leveraging of their merits. They occur naturally, this text only helps observing and recognizing them.
- A well sorted GUI page is one that has not more than one global variable and function, the rest being class definitions.
- Decouple as much as possible, abstract hardcoded config strings, identfiiers, references by using an event subscription system.
- A well sorted GUI class manager is defined in one file and can be deleted without requiring more than one or two reference removals.
- A variety of classes implementing the same interface (set of expected function signatures) can be implemented without using global variable declarations and without using global function declarations while still keeping them grouped in one container.
- Hardcoded numbers, strings, and boolean values can become prototype properties, thus remain easily extensible by the 0ad mod and other mods.
- A good GUIObject size property modification is one that leaves all numbers in XML and sets the size using
let size = this.object1.size; size.top = this.object2.size.bottom; this.object1.size = size;
pattern instead of using fixed numbers. - A JS class that does not sacrifice moddability for performance doesn't specify any numbers, strings or boolean literal in the code but uses references to prototype or instance properties accessed using
this
. - If the JS class can be split into two logically independent parts, it usually makes it easier to comprehend and more extensible to split them into two classes.
- A constructor defines all properties that it will ever be able to own during its lifetime, thus providing the reader the information at the time of reading the constructor what the following code will operate on.
- A class that has more than 20 lines of code is defined in a separate file.
- The purpose of the class is described by the comment using JSdoc syntax preceeding it.
- The purpose of wrapping code in objects to shield variables from global scope is second to the objective of object oriented code to maximize decoupling. The difference may be the introduction of one or 20 classes to solve the same problem [rP23081].
- Function calls with many arguments may be wrapped by using one argument per line. Objects define at most one property per line to form a table and to allow the reader to find a specific keys without having to read the values.
- Constructor arguments that are used by the constructor and stored as member properties are referred to using this access to make the constructor code more similar to the other member function code.
- Engine.GetGUIObjectByName only in constructors.
- A function does not define a local function, but stores it in a class, possibly a new one.
- A temporary variable inside a function is limited to the scope of its use by introducing another scope, thus showing the reader that the variable is not being referred to anymore in the rest of the function scope.
- There is a space between an operator and a value.
- Whitespace only exists before the last non-whitespace character of a line.
- There is no JS code in XML.
- Literal values that are prototype properties start with a capital letter, while prototype functions and instance properties start with a lowercase letter.
- Class properties that are GUIObject references have the same name as the GUI object, allowing the reader to find references to the GUIObject as easily as possible.
- Classes don't have references to globals (except classnames) but obtain object references in the constructor.
- The filename is the name of the class it defines.
- Functions performing work onTick are to cache as much information as possible in their class instance.
- Event handling functions are called externally, hence require special attention and should appear prior to the main substance of the class.
static
functions don't gain the benefit of being able to use member variables and don't gain the benefit of being able to use prototype inheritance and object instance changes from other files (which is especially useful for modders). Example:static
functions have to be called usingClassName.StaticFunction
. So a mod (like nanis autociv mod) would have to replace the function ofClassName
to modify that function. But if the function was not written to be a static one, then the mod can extend the JS object that is an instance of the class without changing the class; and the mod can introduce JS object properties to that instance too (even if the original classClassName
didnt need athis
instance). Since everystatic
function can trivially be implemented as a non-static function, and the non-static ones have tangible benefits, the pattern to never usestatic
(unless in some strongly reasoned exception) established in my commits.get
may be treated as an anti-pattern, because it hides to the caller which property calls are function calls. But the distinction between a function and a JS property fetch should be clear to the caller. If one only wants to implement aget
function without aset
function, one can do the same more commonly with an explicit function. If one wants to implement aget
andset
pair, then it modifies most likely a local state variable, and that local state variable can't be hidden / set to private with current JS standard. Soget
+set
pairs don't have an advantage overgetFoo
+setFoo
pairs or.foo
+setFoo
pairs while having the disadvantage of hiding the fact that calling the thing is a function call. Hiding function calls is bad because they can be expensive for performance, they can have side-effects such as statechanges. So the reader should be informed when they run JS statements rather than being led to believe that there is only a JS property lookup (that does not perform any unrelated state changes).
From my POV using OOP everywhere would be nice. Would it be just making everything objects or do you plan to use MVVM or MVC and any subsequent patterns such as factories and decorators ?
I agree that this is one of the case where review doesn't make sense when the general direction has been agreed upon. Would a rename of all those globals to g_Something be useful beforehand? I could submit a patch for that.