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:

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 Stan, 5 years ago

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.

comment:2 by elexis, 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 Imarok, 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 Itms, 5 years ago

Component: UI & SimulationUI – Miscellaneous

comment:5 by wraitii, 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 Krinkle, 5 years ago

Cc: Krinkle added

comment:7 by elexis, 5 years ago

In 22534:

Delete JSClass JSI_GUIColor / GUIColor from rP666, refs #5387, D1699.

  • JSI_GUIColor::construct and JSI_IGUIObject::setProperty hunk are duplicates of FromJSVal, getProperty of ToJSVal, following rP7259.
  • The constructor, it's fallback magenta and toString have never been used by JS.
  • A JS color class providing some methods (such as found in color.js, refs #5387, and the unused toString redundant with rgbToGuiColor, see also rP21527) is more inviting to changes and maintenance if defined in JS and can still be instantiated in C++, like the Vector2D (see also rP22487).
  • PredefinedColors (such as "red" or "green") can be obtained from the prototype without defining the class in C++.
  • Minimize ugliness by removing macrosity, refs rP725.

Rename alpha to a in guiToRgbColor from rP21527 for consistency with the C++ conversions (opaqueness had been skipped in the only callers of this JS function yet).
Delete unused GUISTDTYPE Mouse in header forgotton in rP22531.

comment:8 by elexis, 5 years ago

In 22663:

Delete wrongful proxy CGUIManager::GetPreDefinedColor from rP7214.

It is wrong because the predefined colors should be loaded from the GUI page that requests to have the color parsed, which may be different from the topmost page.
Similar to FindObjectByName removed in rP22200.

Achieve this by implementing the CGUISetting<CGUIColor>::FromJSVal specialization, moved from ScriptInterface::FromJSVal<CGUIColor>, instead of adding a CGUI pointer to CGUIColor.
Mark ScriptInterface::FromJSVal<GUIColor> and inherited CColor::ParseString explicitly as deleted, so that people get a compile error if they forget to check for predefined colors when parsing a color.

Refs #5387, D1746 > D1684 > this > r22200, rP22534, rP22558, rP22587, r22604.

Differential Revision: https://code.wildfiregames.com/D2108
Tested on: clang 8, VS2015

comment:9 by elexis, 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:10 by elexis, 5 years ago

In 22851:

Support assigning functions to hotkeys from the JS GUI without involving a GUI object.

Allows developers and modders to register and unregister hotkeys without having to specify, change or overwrite XML files.
Also allows implementing the main menu GUI page with only GUI objects for one submenu, refs #5387 / D2240.

Differential Revision: https://code.wildfiregames.com/D2260
Idea by nani in D2257.

comment:11 by elexis, 5 years ago

In 22854:

Rewrite Main Menu.

Specify menu and submenu buttons title, tooltip, hotkey and actions comfortably in an extensible JS object and derive the GUI object settings based on that rather than relying on complex markup alternating with JS in the same file.
Automatically compute the position and number of submenu items instead of letting the author manually compute and keep that in sync in various places of the XML file.

Use object oriented programming, so that there is a strong separation of concerns, allowing the readers and authors to only involve themselves with the component relevant to the feature they work on, refs #5387.
Use class keyword instead of prototype keyword for the JS classes, because that enforces a policy where no globals are inserted between class members and informs the reader of that as soon as reading the classes first line, anticipating fragmentation.

Group project information and community links in a more clearly arranged JS file.
Keep and document splashscreen onTick hack from rP13597, #2042, #5578.
String change: Inform instead of warn (deter) about the state of the game in the main menu.
Depends on Engine.SetGlobalHotkey from D2260.

Differential Revision: https://code.wildfiregames.com/D2240
Class syntax comments by: Krinkle, bb, Chakakhan, smiley, nani, fpre
Main menu comments by: Krinkle, nani, Stan (and Imarok in D820)

comment:12 by elexis, 5 years ago

The XmppClient and NetClient GUI message handlers can be called from C++ rather than by pulling in onTick: #5585.

comment:13 by elexis, 5 years ago

In 22923:

Unify loadgame and savegame dialog following rP13579, refs #2030.

This way the save dialog gains the sortable list and details section feature from rP19351 / D246 by removing a folder of code, refs #4413.
Fixes a style warning about the old dialog using a style for an 'olist' type for a 'list' type, refs rP22792 / D2232.

Subsequently move the gui/common/ helper file to the page code, meaning it's not loaded for every GUI page needlessly anymore.
Fix map preview aspect ratio.
Use object-oriented code for the moved savegame code, refs #5387, but conserve the rest of the code to limit the changes for auditability.

Differential Revision: https://code.wildfiregames.com/D2290
Tested on: Jenkins

comment:14 by elexis, 5 years ago

In 22985:

Rewrite Savegame page to use class syntax, enabled by rP22923 / D2290, refs #5387.

Uses simple event subscription to make the classes independent of each other.
Removes all globals except two variables and one function (that may be avoided in the future too).
Allows the reader to focus on the relevant aspect of the implementation (encapsulation, information hiding) and makes implementation more modifiable and extensible.

Differential Revision: https://code.wildfiregames.com/D2310

comment:15 by elexis, 4 years ago

In 23062:

Rewrite session chat to use hierarchical object oriented design using class syntax, refs #5387.

Set global hotkeys in JS instead of empty XML objects from rP22851/D2260.

Differential Revision: https://code.wildfiregames.com/D2355

comment:16 by elexis, 4 years ago

In 23065:

Refactor diplomacy dialog to use object orientation paradigm using the class keyword, refs #5387.

Includes dilomacy colors, tribute buttons, spy request buttons, attack request buttons, minimap panel, idle worker button, ceasefire counter.
Differential Revision: https://code.wildfiregames.com/D2365

Use hotkey release event from rP19028 to remove input.js state and evil global nested closure hackery from rP13201, refs #1839, #3194.
Fixes masstribute selecitonchange issue subject of D1191.

comment:17 by elexis, 4 years ago

In 23072:

Refactor trade dialog and barter panel buttons to use object orientation, refs #23, #2311, #4366, #5387, rP10588, rP14417, rP17553, rP19354.

Differential Revision: https://code.wildfiregames.com/D2369

comment:18 by elexis, 4 years ago

In 23076:

Implement session event subscription system and rewrite TopPanel, PlayerViewControl, GameSpeed, Pausing, ObjectivesDialog to use object orientation, refs #5387.

New controller classes: PlayerViewControl, PauseControl, GameSpeedControl
New viewer classes: ObjectivesDialog, PauseOverlay, FollowPlayer, TopPanel (BuildLabel, CivIcon, CounterManager, CounterPopulation, CounterResource refs rP21071/D1113, GameSpeedButton, ObjectivesDialogButton)

New events: SimulationUpdate, EntitySelectionChange, ViewedPlayerChange, PreViewedPlayerChangeHandler, PlayerIDChange, PlayersInit, PlayersFinished, Pause, DiplomacyColorsChange, HotkeyChange, refs #2604
Improves GUI onSimuationUpdate performance without selected entities by allegedly 30%.

Delete misleading dead code resign command from leaveGame and rename to endGame. The command is not sent via network (see rP14732) nor processed in simulation, because the Game instance is deleted immediately thereafter, introduced in rP8234, refs rP14772, rP17548, rP18441.
Remove explicitResume 0 value from rP17423 and rP20644 which should have been a false if defined, and is equivalent to the default.
Restore fast forwarding option from rP20577/D595 for developers changing the perspective to observer or player following rP21149.
Add pausing for the delete dialog missing following rP10194.

Differential Revision: https://code.wildfiregames.com/D2378

comment:19 by elexis, 4 years ago

In 23080:

Rewrite session menu code to use object orientation, finish off menu.js, refs #5387, rP22854 / D2240.

Add tooltips for buttons that can be triggered with hotkeys.
Unassign menu animation onTick function after the animation finished, refs rP10146.
Cleans unused openMenu following rP17549.

Differential Revision: https://code.wildfiregames.com/D2380

comment:20 by elexis, 4 years ago

In 23081:

Rewrite developer overlay to use class syntax, one class per checkbox, a class for the EntityState overlay and TimeWarp debug feature, refs #5387.

Using 22 classes instead of 1 class (refs rP22370/D1928) leverages more benefit of the paradigm.
In particular it means the checkboxes can own the EntityState and TimeWarp helpers and the DeveloperOverlay itself can remain independent.
Improve performance by 200 microseconds per turn by unsubscribing from onSimulationUpdate when the developer overlay is not opened, refs rP23076 / D2378.
Move TimeWarp from input.js from rP8803 to independent class using hotkey release event from rP19028, refs #3194.

Differential Revision: https://code.wildfiregames.com/D2383

comment:21 by elexis, 4 years ago

In 23086:

Remove hardcoding and quadruplication of the StatisticsTracker unit and building classes following rP14703, refs #686.

Amongst other issues encountered in the lobby ranking session statistics reporting, refs #5387, D2385, rP14098.
Correct test case from rP21250/D1305.

Differential Revision: https://code.wildfiregames.com/D2384
Comments By: Freagarach

comment:22 by elexis, 4 years ago

In 23087:

Refactor session lobby bot client code to use object orientation, refs #5387.

Adapt rating score for exploration score and add comment to keep calculateExplorationScore and calculateScoreTotal in sync following rP14752 (following the formula in rP12914 and rP14098 being otherwise in sync with the summary screen measures), refs #686.
Adapt rating score for captured entities and add comment to keep calculateMilitaryScore in sync following rP18395, refs #3216, rP16550.
Adapt rating score for trade and vegetarian food and add comment to keep calculateEconomyScore in sync following rP19584/D494 and rP20543/D1052, refs #3948.
Remove ceasefire sending from the client from rP16624 since the bot doesn't record it and since ceasefire time is expected to be 0 after a 1v1 was decided, refs #2749.
Resolve column value computation fragmentation and remove unneeded intermediary object encoding following rP14703, refs #686.
Remove hardcoding/duplication of unit and structure classes and send domesticUnitsTrained too, refs rP23086/D2384.
Move session code including the two globals to a separate folder so as to maximize separation of and ease distribution without lobby code.
Migrate useless ugly trailing commas from rP14098, to be removed in a patch modifying the bot code.

Differential Revision: https://code.wildfiregames.com/D2385

comment:23 by elexis, 4 years ago

In 23088:

Move researched tech progress overlay to a separate file, use class notation, refs #5387.

Improves performance if no techs are researched from 200 to 50 microseconds per turn (from 500 to 300 microseconds for one tech etc).

Differential Revision: https://code.wildfiregames.com/D2386

comment:24 by elexis, 4 years ago

In 23089:

Decouple panel entities code from session code and use class notation, refs #5387, #3000, #1902, #1802, rP18361.

Change the logic to only insert/delete buttonhandlers on ownershipchange and update only the entitystate dependent part on simulation update.

Differential Revision: https://code.wildfiregames.com/D2387

comment:25 by elexis, 4 years ago

In 23091:

Make options page agnostic of session page and implement RangeOverlayManager GUI class, refs #5387, #4747.

Avoids further session commits that missed to keep options.json session code in sync,
enables use of anonymous or object owned config callback handlers and
encourages session developers to make the GUI more reactive to changing options.

Update RangeOverlays when reseting to default, and update
RangeOverlays of selected entities upon closing the page too (not only hotkey).

Differential Revision: https://code.wildfiregames.com/D2389

comment:26 by elexis, 4 years ago

In 23093:

Small mainmenu style cleaup following rP22854/D2240, refs #5387.

Add class MainMenuPage, BackgroundLayer, remove duplication of XML size values in the MainMenuItemHandler,
remove onTick assignment from XML, replace references to globals with local references.

Differential Revision: https://code.wildfiregames.com/D2390
Comments By: Stan

comment:27 by elexis, 4 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 using ClassName.StaticFunction. So a mod (like nanis autociv mod) would have to replace the function of ClassName 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 class ClassName didnt need a this instance). Since every static function can trivially be implemented as a non-static function, and the non-static ones have tangible benefits, the pattern to never use static (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 a get function without a set function, one can do the same more commonly with an explicit function. If one wants to implement a get and set 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. So get+set pairs don't have an advantage over getFoo + 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).
Last edited 4 years ago by elexis (previous) (diff)

comment:28 by elexis, 4 years ago

In 23096:

Rewrite FPS/Realtime/Gametime/Ceasefire counters to use object semantics using class notation, refs #5387.

Rebuild the counters every 250ms instead of every frame and minimize object creation.

Differential Revision: https://code.wildfiregames.com/D2391
Comments By: Stan

comment:29 by elexis, 4 years ago

In 23103:

Support deleting GUI Object event handlers following rP666, refs #5387.

Differential Revision: https://code.wildfiregames.com/D2395
Reported by: nani
Tested on: gcc 9.2.0, Jenkins/vs2015

comment:30 by elexis, 4 years ago

In 23114:

SessionMessageBox class, refs #5387.

Decouples events from event handling, removes implicit-pause duplication per messagebox and allows mods to modify message box values.

Differential Revision: https://code.wildfiregames.com/D2398
Comments By: Stan, Freagarach

comment:31 by elexis, 4 years ago

In 23115:

Move session network status overlay to a separate class, refs #5387 and fix pause hotkey disabling for non-hosting observers following rP23076/D2378.

comment:32 by elexis, 4 years ago

In 23116:

Cheats GUI container class, refs #5387.

comment:33 by elexis, 4 years ago

In 23117:

TimeNotificationOverlay and Ambient class, refs #5387.

Notice TimeNotificationOverlay.xml must be included after PauseOverlay.xml, otherwise there will be a Z/transparency bug, see D148.

comment:34 by elexis, 4 years ago

In 23150:

Rewrite loading screen to use class semantics, refs #5387.

Add config option to display the loading screen stage description.
Add tip functions for future use, refs D1730.
Harmonize coordinates.

Differential Revision: https://code.wildfiregames.com/D2418

comment:35 by elexis, 4 years ago

In 23172:

Rewrite lobby page to use class semantics, add more gamedetails labels, improve performance using batch processing and caching and gain possibility for game creation/player-join/leave events, refs #5387.

Game selection details features:

  • Display victory conditions following their sending but missing display following rP14098, refs rP19642.
  • Display the host of the match and the game name in the selected game details following rP19703, refs D1666.
  • Display mods if the mods differ (without having to attempt to join the game prior) following rP21301.

Performance features:

  • Implement batch message processing in the XmppClient to rebuild GUI objects only once when receiving backlog or returning from a match.
  • Implement Game class to cache gamelist, filter and sorting values, as they rarely change but are accessed often.
  • Cache sprintf objects.

Security fixes:

  • Add escapeText in lobby/ to avoid players breaking the lobby for every participant, supersedes D720, comments by bb.
  • Do not hide broadcasted unrecognized chat commands that mods used as leaking private channels, fixes #5615.

Defect fixes:

  • Fix XmppClient.cpp storing unused historic message types resulting in memory waste and unintentional replay of for instance disconnect/announcements messages following both rP20070/D819 and rP22855/D2265, refs #3306.
  • Fix XmppClient.cpp victoryCondition -> victoryConditions gamesetup.js change from rP21474/D1240.
  • Fix leaderboard/profile page cancel hotkey closing the lobby dialog as well and removes cancel hotkey note from lobby_panels.xml from rP20886/D817 since the described issue was fixed by rP22200/D1701.
  • Fix lobby playing menu sound in a running game after having closed the lobby dialog following introduction in rP20886/D817.
  • Fix GUI on nick change by updating g_Username.
  • Update profile panel only with data matching the player requested.

Hack erasure:

  • Object semantics make it cheap to add state and cache values, storing literals in properties while removing globals, adding events while decoupling components and gaining moddability.
  • Erase comments and translation comments stating that this would be IRC!!, supersedes D1136.
  • Introduce Status chat message type to supersede "/special" chat command + "isSpecial" property from rP14098 (formerly g_specialKey rP17360) deluxe hack.
  • Introduce System chat message type to supersede system errors disguising as chat from a mock user called "system".

Code cleanups:

  • Move code from XML to JS.
  • Move size values from JS to XML, especially following rP20886/D817 and rP21003/D1051.
  • Rename "user" to "player".
  • Fix lobby/ eslint warnings, refs D2261.
  • Remove message.nick emptiness check from rP20064/D835, since XEP-0045 dictates that it is non-empty.
  • Add translated string for deleted subjects.
  • Add TODOs for some evident COList issues, refs #5638.

Differential Revision: https://code.wildfiregames.com/D2412

comment:36 by elexis, 4 years ago

In 23374:

Gamesetup class rewrite, fixes #5322, refs #5387.

  • Decouples settings logically which in turn allows fixing many problems arising from previous coupling.
  • Fixes the persist-match-settings feature, refs #2963, refs #3049.
  • Improves performance of the matchsetup by rebuilding GUI objects only when necessary.

Provides groundwork for:

  • UI to control per-player handicap, such as StartingResources, PopulationCap, StartingTechnologies, DisabledTechnologies, DisabledTemplates, ..., refs #812.
  • Map specific settings (onMapChange event), refs #4838.
  • Chat notifications announcing which settings changed, refs D1195,
  • Multiple controllers setting up the game (since setting types can check for permissions in onUpdateGameAttributes without the need for a new data model or a second gamesetup data network message type), refs #3806, subsequently dedicated server, refs #3556.
  • MapBrowser (MapCache, MapTypes, onUpdateGameAttributes interface), refs D1703 and D1777,
  • Multiplayer saved games (decoupling and setting dependent unique logic), refs #1088.

Refs https://wildfiregames.com/forum/index.php?/topic/20787-paid-development-2016/ https://wildfiregames.com/forum/index.php?/topic/20789-paid-development-2016/

Enable maps to restrict setting values:

  • If a map specifies an AI or Civs for a playerslot, the controller can't assign a player/other AI or Civ to that slot, refs #3049, #3013.

Fix per player StartingResources, PopulationCap, StartingTechnologies, DisabledTechnologies, DisabledTemplates following rP12756, refs #812, fixes #4504. Use this for DisabledTechnologies on Polar Sea.

Persist user settings for Skirmish maps:

  • All user chosen settings are persisted when changing the selected map or maptype, except where the selected map overwrites the setting value and except for Scenario maps which still use the default value where the map doesn't specify the setting value.
  • Tickets relating to that Skirmish mapchange user setting persistance:
    • Selecting a map doesn't change the selected civilizations, fixes #3120 (together with r23279 removing map specified Civs).
    • Selecting a map type doesn't reset the selected settings, fixes #5372.
    • Selecting a map doesn't change the selected victory conditions, unless the map specifies those, refs #4661, #3209. (Atlas still writes VictoryConditions to every map.)
    • Consume the player color palette from Skirmish maps, refs rP17040 / #1580. Preserve the selected playercolors when switching the Skirmish/Random map by chosing the most similar colors if the map comes with a different palette.

Rated games:

  • Hide and disable Rated game setting unless there are exactly two players, fixes #3950, supersedes D2117.
  • Display conspicuous warning if the game is rated, so players are perfectly aware.

Autostarted games:

  • Allow using the gamesetup page to autostart matches with arbitrary maps, not only this one tutorial, as reported in D194 and rP19599, refs D11.

Networking:

  • Keep gamesetup page open after disconnect, allowing players to read chat messages indicating why the host stopped the server, fixes #4114.
  • The message subscription system allows new and mod settings to run custom logic on arbitrary setting changes (most importantly on map change). This removes hardcoded logic restrictions from the gamesetup option unification rewrite in rP19504/D322, refs #3994, such as the hardcoding of setting references in selectMap to biomes from rP20115/D852 and the difficulty from rP20760/D1189, RelicDuration, WonderDuration, LastManStanding, RegicideGarrison, TriggerScripts, CircularMap, Garrison, DisabledTemplates.

Checkboxes:

  • Display values of disabled checkboxes with Yes/No labels, fixes D2349, reviewed by nani.

Clean g_GameAttributes of invalid values and gamesetup GUI temporaries, refs #3049, #3883:

MapCache:

  • Refactor to MapCache class, store maps of all types and use it in the replaymenu, lobby and session as well.

SettingTabsPanel:

  • Remove hardcodings and coupling of the SettingTabsPanel with biomes/difficulties/chat UI from D1027/rP20945.

GamesetupPage.xml:

  • Restructure the page to use hierarchical object organization (topPanel, centerPanel, centerLeftPanel, bottomPanel, centerCenterPanel, centerRightPanel, bottomLeftPanel, bottomRightPanel), allowing to deduplicate object position margins and size math and ease navigation.

New defaults:

  • Check LockedTeams default in multiplayer (not only rated games).
  • Persist the rated game setting instead of defaulting to true when restarting a match, which often lead to unintentional rated games when rehosting.
  • 60 FPS in menus since they are animated

Autocomplete sorting fixed (playernames should be completed first).
Refactoring encompasses the one proposed in Polakrity and bb D1651.

Differential Revision: https://code.wildfiregames.com/D2483
Tested by: nani
Discussed with:

Emojis by: asterix, Imarok, fpre, nani, Krinkle, Stan, Angen, Freagarach

comment:37 by elexis, 4 years ago

In 23419:

Rewrite Gamesetup AIConfig page to use class syntax, decouple settings and move it to the Gamesetup subfolder context, refs #5322, #5387.

This establishes a code architecture where only one class and only one page is responsible per AI setting,
removes the hardcodings from the XML and JS file,
allowing future patches and mods to insert, remove and edit independent AI settings without having to overwrite the existing code.

Differential Revision: https://code.wildfiregames.com/D2577
See also rP23413/D2581 and comments linked there.

comment:38 by elexis, 4 years ago

In 23452:

Support GUI script includes in included XML files to further decoupling of GUI page contents, refs #5387.

Differential Revision: https://code.wildfiregames.com/D2599
Comment By: Stan, Vladislav

comment:39 by s0600204, 3 years ago

In 24289:

Rewrite the civinfo page to use OOP principles

Tested by: Nescio, Freagarach
Refs: #5387
Differential Revision: https://code.wildfiregames.com/D2822

Note: See TracTickets for help on using tickets.