Opened 7 months ago

Last modified 4 days 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 (8)

comment:1 Changed 7 months ago by stanislas69

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 Changed 7 months ago by elexis

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 Changed 7 months ago by Imarok

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 Changed 5 months ago by Itms

Component: UI & SimulationUI – Miscellaneous

comment:5 Changed 2 months ago by wraitii

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 Changed 2 months ago by Krinkle

Cc: Krinkle added

comment:7 Changed 4 weeks ago by elexis

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 Changed 4 days ago by elexis

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

Note: See TracTickets for help on using tickets.