Page MenuHomeWildfire Games

Remove ancient GUI ScriptEvent lowercase struggle

Authored by elexis on Nov 26 2019, 6:33 PM.




  1. Delete useless lowercase for ScriptEventNames to reduce code complexity and improve performance.
  2. Create EventName CStr strings only once per program launch instead of once per call.
  3. Send mouse coordinates and mouse buttons object only to mouse events, instead of all events that don't provide custom data.

GUI ScriptEvents such as the famous onTick event are converted to lowercase format, resulting in a number of case inconsistencies in various JS and C++ code and XML markup.
For example mouseleftdoubleclickitem in replaymenu but and MouseLeftDoubleClickItem elsewhere.
These inconsistencies had triggered bugs (as some parts needed to apply lowercasing to become consistent), then TODO comments, and more bugfixes adding comments explaining that this all makes sense and that there is nothing to see there.
Additionally the needless lowercasing performs string copies that would not be necessary and would save some performance.
Originally the lowercasing was introduced as a feature, but it appears to have been a flawed concept.


Jul 8 2004

  • rP665 Philip introduces Xeromyces XMB with the lowercasing advertized as a feature, intended to make it more fail safe by ignoring spelling mistakes:
* Case-sensitive (but converts all element/attribute names in
  the XML file to lowercase, so you only have to be careful in
  the code)

Jul 11 2004

  • rP706 first bugfix, by Phiip:
-			object->RegisterScriptHandler(action, code, this);
+			object->RegisterScriptHandler(action.LowerCase(), code, this);

Jul 22 2005

  • rP2511 second bugfix, by gee:
// TODO only works if lower-case, shouldn't it be made case sensitive instead?

Mar 3 2006

  • rP3586 third bugfix, by janwas:
void CGUI::SendEventToAll(const CStr& EventName)
	// janwas 2006-03-03: spoke with Ykkrosh about EventName case.
	// when registering, case is converted to lower - this avoids surprise
	// if someone were to get the case wrong and then not notice their
	// handler is never called. however, until now, the other end
	// (sending events here) wasn't converting to lower case,
	// leading to a similar problem.
	// now fixed; case is irrelevant since all are converted to lower.

I didn't find this conversation mentioned.

Mar 11 2006:

  • From 2006-03-11-QuakeNet-#wfg-Meeting-0126.log:
16:43 <Matei> OK, well I don't really have anything to discuss; the only thing that comes to mind is the Xeromyces issue (we should make it case-sensitive at some point)
16:46 <Ykkrosh> I can't think of why it's case insensitive at the moment, except possibly so programmers can assume the names are all lowercase and don't have to check in the XML file to see what case it actually uses. But that doesn't seem like a great advantage, since you have to check how things are spelt anyway, and any errors would be immediately obvious.
16:47 <Ykkrosh> and so if you have a reason to want to preserve the case, I'd agree with changing it to do that
16:48 <Ykkrosh> and I think it should be reasonably straightforward - it'll just need all the Xeromyces-using code to start using the same case as the XML files
16:48 <Ykkrosh> so, er, all we need is to actually do the work :-P

Jun 23 2006

  • matei created #127 to remove the lowercase feature

Jun 24 2006

  • From 2006-06-24-QuakeNet-#wfg-Meeting-0139.log
17:38 <Matei> I remembered one thing, which is to make Xeromyces not convert elements to lower-case by default, which would be nice as we finalize the entity properties and do more entity coding; I put up a ticket on so we don't forget

Aug 01 2006

  • matei removes the lowercase from Xeromyces but not the GUI lowercase code
  • rP4185 XML / JavaScript changes for new case-aware XMB format
  • rP4186 Modified Xeromyces to no longer automatically convert element and attribute names to lowercase

Sep 07 2006

  • matei closes #127 as fixed

So it seems everyone who came into contact with the lowercase code explicitly stated that they didn't see the point for having the lowercase format or added more lowercasing code merely to account for a bug arising from the lowercasing being applied inconsistently.

2. const EventNames:
Unless the code optimizers are smart enough, the CStr string names are created once per event call.
At least the onTick call is done once per frame, so it should be optimized.
By making these strings static const members, we enforce that they are not recreated, without having to check what some of the compilers do.

Secondly having the EventNames stored as named objects allows refering to these names in multiple places of the code in a way that triggers compile errors if there is a spelling mistake.

A planned future user of these string constants is an error message that is to be triggered in case someone does a spelling mistake in JS or XML for an EventName.
That error is currently absent, it would be useful for authors to see dead code due to a typo (something I ran into at least once and only found by accident).
And that error might precisely cover the use case of the lowercasing feature from rP665.
But this should be done independently to avoid mixing two discussions about two different implementations.

3. MouseEvent

  • For many events that don't pass a custom JS object there is a JS object with mouse coordinates and mouse buttons created. But if it isn't an event relating to the mouse directly, it is a useless surprise that costs performance, rather than a useful argument.

(In the future one might consider obtaining this object only via a JS Interface function or GUI Object property access call, rather than passing it in advance regardless of it being used or not.)

Test Plan
  1. Read the summary. See the history of suffering and become bothered too each time you read the lowercase() for no reason.
  1. Should the event name constants use the m_ prefix?

The CodingConventions page speaks of member variables using that, member functions using FooBar case. Strictly speaking, static const members are not member variables, and there are essential differences between member variables and const static members.

  • (1), (2) Variables are once per class instance and are variable, static const members are invariable, created only once per program launch
  • (3) (more importantly) member variables are assigned with m_Foo = bar and m_Foo(bar), but static const members are initialized with const T FooClass::m_Foo = bar.

Member functions (those that aren't std::function) are created only once per program launch too and are invariable afaik (ignoring class inheritance).
So const T FooClass::BarMember = value; more resembles the member function syntax and semantics than const T FooClass::m_BarMember = value;.

  1. Perhaps check with clang optimization report if the static const string EventNames are already created by clang.

I used CC="clang -Rpass=inline" CXX="clang++ -Rpass=.*" make -j3 and got this for the one line I tested:

../../../source/ps/GameSetup/GameSetup.cpp:199:2: remark: _ZNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEED2Ev inlined into _Z23GUI_DisplayLoadProgressiPKw with (cost=10, threshold=250) [-Rpass=inline]
        g_GUI->SendEventToAll(/*g_EventNameGameLoadProgress*/"GameLoadProgress", paramData);
../../../source/ps/GameSetup/GameSetup.cpp:199:55: remark: _ZN5CStr8C2EPKc inlined into _Z23GUI_DisplayLoadProgressiPKw with (cost=140, threshold=250) [-Rpass=inline]
        g_GUI->SendEventToAll(/*g_EventNameGameLoadProgress*/"GameLoadProgress", paramData);
../../../source/ps/GameSetup/GameSetup.cpp:199:75: remark: _ZN2JS16HandleValueArrayC2ERKNS_16AutoVectorRooterINS_5ValueEEE inlined into _Z23GUI_DisplayLoadProgressiPKw with (cost=-40, threshold=375) [-Rpass=inline]
        g_GUI->SendEventToAll(/*g_EventNameGameLoadProgress*/"GameLoadProgress", paramData);
../../../source/ps/GameSetup/GameSetup.cpp:199:2: remark: _ZNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEED2Ev inlined into _Z23GUI_DisplayLoadProgressiPKw with (cost=10, threshold=250) [-Rpass=inline]
        g_GUI->SendEventToAll(/*g_EventNameGameLoadProgress*/"GameLoadProgress", paramData);

But even if it does optimize it, writing the code this way will enforce this even if the optimizer is stupid or the compiler old, and it
will allow us to reuse these static const members in other contexts (getting compile errors when adding typos).

  1. Consider whether char* would be preferable over CStr.

What speaks for CStr is that
4.1. CodingConventions recommend using CStr over std::string, since it provides more functions, for example UTF8 conversion or lowercase.
4.2. IGUIObject::RecurseObject parameter pack currently complains about not receiving a refernce.
4.3. If one uses char* then it means one will have to create a CStr the moment one will need a CStr, whereas using CCtr as the const static member type means both CStr& and char* arguments can be provided without copying the string.
4.4. CStrIntern may be another candidate. However it does not appear to be as attractive as advertizes. When creating a CStrIntern it performs hashing on the string, which is the same as an std::map or std::unordered::map find with string keys, so basically the same.

  1. To test completeness of the JS/XML changes:
  2. Search in XML GUI files for on=" (mind the space at the beginning)
  3. Search in JS GUI files for .on*=

and see that there is TitleCase used exclusively

Diff Detail

rP 0 A.D. Public Repository
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

elexis created this revision.Nov 26 2019, 6:33 PM

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

Link to build:

nani awarded a token.Nov 26 2019, 6:43 PM
elexis edited the summary of this revision. (Show Details)Nov 26 2019, 6:45 PM
elexis edited the summary of this revision. (Show Details)

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

Linter detected issues:
Executing section Source...

|  24| class·CSlider·:·public·IGUIObject
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCSlider:' is invalid C code. Use --std or --language to configure the language.

|  34| class·CInput·:·public·IGUIObject,·public·IGUIScrollBarOwner
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCInput:' is invalid C code. Use --std or --language to configure the language.

|  53| class·IGUIObject
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classIGUIObject{' is invalid C code. Use --std or --language to configure the language.

|  51| class·CGUI
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCGUI{' is invalid C code. Use --std or --language to configure the language.

|  58| class·CTurnManager
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCTurnManager{' is invalid C code. Use --std or --language to configure the language.

|  42| class·CGUIManager
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCGUIManager{' is invalid C code. Use --std or --language to configure the language.

|  50| class·COList·:·public·CList
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCOList:' is invalid C code. Use --std or --language to configure the language.

|  37| class·CList·:·public·IGUIObject,·public·IGUIScrollBarOwner,·public·IGUITextOwner
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCList:' is invalid C code. Use --std or --language to configure the language.

|  29| class·CMiniMap·:·public·IGUIObject
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCMiniMap:' is invalid C code. Use --std or --language to configure the language.

|  38| class·IGUIButtonBehavior
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classIGUIButtonBehavior{' is invalid C code. Use --std or --language to configure the language.

| 108| class·XMBFile
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classXMBFile{' is invalid C code. Use --std or --language to configure the language.

|  41| class·CGame
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCGame{' is invalid C code. Use --std or --language to configure the language.

|   1| /*·Copyright·(C)·2017·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2017"

|  58| The line belonging to the following result cannot be printed because it refers to a line that doesn't seem to exist in the given file.
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCTurnManager{' is invalid C code. Use --std or --language to configure the language.
Executing section JS...

| 686| »   »   button.onPress·=·(function(i)·{·return·function()·{·performGroup((Engine.HotkeyIsPressed("selection.add")·?·"add"·:·"select"),·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

| 687| »   »   button.onDoublePress·=·(function(i)·{·return·function()·{·performGroup("snap",·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

| 688| »   »   button.onPressRight·=·(function(i)·{·return·function()·{·performGroup("breakUp",·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.
Executing section cli...

Link to build:

vladislavbelov added inline comments.
190 ↗(On Diff #10426)

Why do we need it (and all other similar cases) in header? Why not to have the constant in implementation only.

elexis added inline comments.Nov 27 2019, 12:05 PM
190 ↗(On Diff #10426)

It seems cleaner to me as it is an attribute of that class, so it's (1) in the namespace of that class (i.e. if theres another global included from elsewhere its not triggering a naming conflict), (2) in case there are multiple classes, it becomes clear which constant is owned by which class, (3) the constant is ready to be reused, for example for inheritance or a friend class.
Those are something like 20-30 globals or members in this commit, imagine there are 10x more of them, then it becomes more noticeable that it would appear cleaner with everything being part of a class.

Consider another example, member functions are also often added as private members but could also become cpp globals if they don't access many members, but theyre made private members since it's well sorted and consistent with the other function definitions that are protected or public members, right?

Current naming cases are heterogenous.
It seems most of the UNDERSCORE_UPPERCASE seems to come from C developers.
We notice that at least in source/gui/ there are no UNDERSCORE_UPPERCASE constants, so the folder is selfconsistent.

maths/Brush.h:	static const size_t NO_VERTEX = ~0u;
maths/MD5.h:	static const size_t DIGESTSIZE = 16;
maths/BoundingBoxOriented.h:	static const CBoundingBoxOriented EMPTY;
maths/Fixed.h:	static const bool is_specialized = true;
maths/BoundingBoxAligned.h:	static const CBoundingBoxAligned EMPTY;
maths/Plane.h:		static const float m_EPS;

tools/atlas/GameInterface/View.cpp:		static const char digits[] = "0123456789abcdef";
tools/atlas/AtlasUI/CustomControls/EditableListCtrl/EditableListCtrl.h:	// just use static constant strings)
tools/atlas/AtlasUI/CustomControls/MapDialog/MapDialog.cpp:static const wxString scenarioPath(L"maps/scenarios/");
tools/atlas/AtlasUI/CustomControls/MapDialog/MapDialog.cpp:static const wxString skirmishPath(L"maps/skirmishes/");
tools/atlas/AtlasUI/CustomControls/MapDialog/MapDialog.cpp:static const wxString tutorialPath(L"maps/tutorials/");
tools/atlas/AtlasUI/CustomControls/VirtualDirTreeCtrl/folder.xpm:static const char *xpm_folder[] = {
tools/atlas/AtlasUI/CustomControls/VirtualDirTreeCtrl/file.xpm:static const char *xpm_file[] = {
tools/atlas/AtlasUI/CustomControls/VirtualDirTreeCtrl/root.xpm:static const char *xpm_root[] = {
tools/atlas/AtlasUI/ScenarioEditor/Sections/Terrain/Terrain.cpp:	static const int imageWidth = 120;
tools/atlas/AtlasUI/ScenarioEditor/Sections/Terrain/Terrain.cpp:	static const int imageHeight = 40;
tools/atlas/AtlasUI/ScenarioEditor/Sections/Terrain/Terrain.cpp:	static const int imageWidth = 120;
tools/atlas/AtlasUI/ScenarioEditor/Sections/Terrain/Terrain.cpp:	static const int imageHeight = 40;
tools/atlas/AtlasUI/ScenarioEditor/Sections/Environment/Environment.cpp:	static const int range = 1024;
tools/atlas/AtlasUI/ScenarioEditor/Sections/Player/Player.cpp:	static const size_t MAX_NUM_PLAYERS = 8;
tools/atlas/AtlasUI/ScenarioEditor/Tools/Common/Brushes.h:	static const float STRENGTH_MULTIPLIER;
tools/atlas/AtlasUI/ScenarioEditor/Tools/TransformObject.cpp:	static const wxKeyCode SELECTION_ADD_HOTKEY = WXK_SHIFT;
tools/atlas/AtlasUI/ScenarioEditor/Tools/TransformObject.cpp:	static const wxKeyCode SELECTION_REMOVE_HOTKEY = WXK_CONTROL;	// COMMAND on Macs
tools/atlas/AtlasUI/ScenarioEditor/Tools/TransformObject.cpp:	static const wxKeyCode SELECTION_ACTORS_HOTKEY = WXK_ALT;
tools/atlas/AtlasUI/ScenarioEditor/Tools/PaintTerrain.cpp:	static const wxKeyCode EYEDROPPER_HOTKEY = WXK_SHIFT;

graphics/LOSTexture.cpp:static const size_t g_BlurSize = 7;
graphics/LOSTexture.cpp:static const size_t g_SubTextureAlignment = 4;
graphics/ShaderProgramFFP.cpp:		static const float GreyscaleDotColor[4] = {
graphics/ShaderProgramFFP.cpp:		static const float GreyscaleInterpColor0[4] = { 1.0f, 1.0f, 1.0f, 1.0f };
graphics/ShaderProgramFFP.cpp:		static const float GreyscaleInterpColor1[4] = { 0.5f, 0.5f, 0.5f, 1.0f };
graphics/CameraController.cpp:static const float CAMERA_EDGE_MARGIN = 2.0f * TERRAIN_TILE_SIZE;
graphics/HFTracer.cpp:static const int MARGIN_SIZE = 64;

renderer/SilhouetteRenderer.cpp:static const bool g_DisablePreciseIntersections = false;
renderer/SilhouetteRenderer.cpp:static const u16 g_MaxCoord = 1 << 14;
renderer/SilhouetteRenderer.cpp:static const u16 g_HalfMaxCoord = g_MaxCoord / 2;
renderer/TerrainRenderer.cpp:	static const float one[4] = { 1.f, 1.f, 1.f, 1.f };
renderer/PatchRData.h:	static const ssize_t water_cell_size = 1;
renderer/SkyManager.cpp:	static const CStrW images[NUMBER_OF_TEXTURES + 1] = {
renderer/SkyManager.cpp:	static const int types[] = {
renderer/OverlayRenderer.h:	static const float OVERLAY_VOFFSET;
renderer/OverlayRenderer.cpp:	static const size_t MAX_QUAD_OVERLAYS = 1024;
renderer/WaterManager.cpp:	static const int around[8][2] = { { -1,-1 }, { -1,0 }, { -1,1 }, { 0,1 }, { 1,1 }, { 1,0 }, { 1,-1 }, { 0,-1 } };
renderer/WaterManager.cpp:				static const float perpT1[9] = { 6.0f, 6.05f, 6.1f, 6.2f, 6.3f, 6.4f, 6.5f, 6.6f, 9.7f };
renderer/WaterManager.cpp:				static const float perpT2[9] = { 2.0f, 2.1f,  2.2f, 2.3f, 2.4f, 3.0f, 3.3f, 3.6f, 9.5f };
renderer/WaterManager.cpp:				static const float perpT3[9] = { 1.1f, 0.7f, -0.2f, 0.0f, 0.6f, 1.3f, 2.2f, 3.6f, 9.0f };
renderer/WaterManager.cpp:				static const float perpT4[9] = { 2.0f, 2.1f,  1.2f, 1.5f, 1.7f, 1.9f, 2.7f, 3.8f, 9.0f };
renderer/WaterManager.cpp:				static const float heightT1[9] = { 0.0f, 0.2f, 0.5f, 0.8f, 0.9f, 0.85f, 0.6f, 0.2f, 0.0 };
renderer/WaterManager.cpp:				static const float heightT2[9] = { -0.8f, -0.4f, 0.0f, 0.1f, 0.1f, 0.03f, 0.0f, 0.0f, 0.0 };
renderer/WaterManager.cpp:				static const float heightT3[9] = { 0.0f, 0.0f, 0.0f, 0.0f, 0.0f, 0.0f, 0.0f, 0.0f, 0.0 };

lobby/StanzaExtensions.cpp:	static const glooxwrapper::string filter = "/iq/report[@xmlns='" XMLNS_GAMEREPORT "']";
lobby/StanzaExtensions.cpp:	static const glooxwrapper::string filter = "/iq/query[@xmlns='" XMLNS_BOARDLIST "']";
lobby/StanzaExtensions.cpp:	static const glooxwrapper::string filter = "/iq/query[@xmlns='" XMLNS_GAMELIST "']";
lobby/StanzaExtensions.cpp:	static const glooxwrapper::string filter = "/iq/query[@xmlns='" XMLNS_PROFILE "']";
lobby/StanzaExtensions.cpp:	static const glooxwrapper::string filter = "/iq/auth[@xmlns='" XMLNS_LOBBYAUTH "']";
lobby/scripting/JSInterface_Lobby.cpp:	static const unsigned char salt_base[DIGESTSIZE] = {
lobby/glooxwrapper/glooxwrapper.cpp:static const std::string XMLNS = "xmlns";
lobby/glooxwrapper/glooxwrapper.cpp:static const std::string XMLNS_JINGLE_0AD_GAME = "urn:xmpp:jingle:apps:0ad-game:1";

collada/PMDConvert.cpp:		static const VertexBlend noBlend = { { 0xFF, 0xFF, 0xFF, 0xFF }, { 0, 0, 0, 0 } };
collada/DLL.cpp:	static const unsigned int bufferSize = 4096;

gui/SettingTypes/CGUIString.cpp:static const int NUM_WORD_DELIMITERS = 4*2;
gui/SettingTypes/CGUIString.cpp:static const u16 WordDelimiters[NUM_WORD_DELIMITERS] = {
gui/SettingTypes/CGUIString.h:		static const int Left = 0;
gui/SettingTypes/CGUIString.h:		static const int Right = 1;

simulation2/helpers/Spatial.h:	static const int SUBDIVISION_SIZE = 20; // bigger than most buildings and entities
simulation2/helpers/Pathfinding.h:static const int PASS_CLASS_BITS = 16;
simulation2/helpers/MapEdgeTiles.h:static const ssize_t MAP_EDGE_TILES = 3;
simulation2/helpers/HierarchicalPathfinder.h:	static const u8 CHUNK_SIZE = 96; // number of navcells per side
simulation2/helpers/Player.h:static const player_id_t INVALID_PLAYER = -1;
simulation2/helpers/VertexPathfinder.cpp:static const u8 QUADRANT_NONE = 0;
simulation2/helpers/VertexPathfinder.cpp:static const u8 QUADRANT_BL = 1;
simulation2/helpers/VertexPathfinder.cpp:static const u8 QUADRANT_TR = 2;
simulation2/helpers/VertexPathfinder.cpp:static const u8 QUADRANT_TL = 4;
simulation2/helpers/VertexPathfinder.cpp:static const u8 QUADRANT_BR = 8;
simulation2/helpers/VertexPathfinder.cpp:static const u8 QUADRANT_BLTR = QUADRANT_BL|QUADRANT_TR;
simulation2/helpers/VertexPathfinder.cpp:static const u8 QUADRANT_TLBR = QUADRANT_TL|QUADRANT_BR;
simulation2/helpers/VertexPathfinder.cpp:static const u8 QUADRANT_ALL = QUADRANT_BLTR|QUADRANT_TLBR;
simulation2/helpers/VertexPathfinder.cpp:static const entity_pos_t EDGE_EXPAND_DELTA = entity_pos_t::FromInt(1)/16;
simulation2/MessageTypes.h:	static const std::array<const char*, UpdateType::LENGTH> UpdateTypeStr;
simulation2/components/ICmpTerritoryManager.h:	static const int NAVCELLS_PER_TERRITORY_TILE = 8;
simulation2/components/ICmpTerritoryManager.h:	static const int TERRITORY_PLAYER_MASK = 0x1F;
simulation2/components/ICmpTerritoryManager.h:	static const int TERRITORY_CONNECTED_MASK = 0x20;
simulation2/components/ICmpTerritoryManager.h:	static const int TERRITORY_BLINKING_MASK = 0x40;
simulation2/components/ICmpTerritoryManager.h:	static const int TERRITORY_PROCESSED_MASK = 0x80; //< For internal use; marks a tile as processed.
simulation2/components/CCmpSelectable.cpp:static const float MIN_ALPHA_ALWAYS_VISIBLE = 0.65f;
simulation2/components/CCmpSelectable.cpp:static const float MIN_ALPHA_UNSELECTED = 0.0f;
simulation2/components/CCmpSelectable.cpp:static const float RGB_DESATURATION = 0.333333f;
simulation2/components/CCmpSelectable.cpp:	static const double FADE_DURATION;
simulation2/components/CCmpSelectable.cpp:	static const char* TEXTUREBASEPATH;
simulation2/components/CCmpUnitMotion.cpp:static const entity_pos_t SHORT_PATH_MIN_SEARCH_RANGE = entity_pos_t::FromInt(TERRAIN_TILE_SIZE*3)/2;
simulation2/components/CCmpUnitMotion.cpp:static const entity_pos_t SHORT_PATH_MAX_SEARCH_RANGE = entity_pos_t::FromInt(TERRAIN_TILE_SIZE*6);
simulation2/components/CCmpUnitMotion.cpp:static const entity_pos_t SHORT_PATH_SEARCH_RANGE_INCREMENT = entity_pos_t::FromInt(TERRAIN_TILE_SIZE*1);
simulation2/components/CCmpUnitMotion.cpp:static const entity_pos_t SHORT_PATH_LONG_WAYPOINT_RANGE = entity_pos_t::FromInt(TERRAIN_TILE_SIZE*1);
simulation2/components/CCmpUnitMotion.cpp:static const entity_pos_t LONG_PATH_MIN_DIST = entity_pos_t::FromInt(TERRAIN_TILE_SIZE*4);
simulation2/components/CCmpUnitMotion.cpp:static const entity_pos_t DIRECT_PATH_RANGE = entity_pos_t::FromInt(TERRAIN_TILE_SIZE*4);
simulation2/components/CCmpUnitMotion.cpp:static const entity_pos_t TARGET_UNCERTAINTY_MULTIPLIER = entity_pos_t::FromInt(TERRAIN_TILE_SIZE*2);
simulation2/components/CCmpUnitMotion.cpp:static const u8 MAX_FAILED_PATH_COMPUTATIONS = 15;
simulation2/components/CCmpUnitMotion.cpp:static const u8 MAX_FAILED_PATH_COMPUTATIONS_BEFORE_LONG_PATH = 3;
simulation2/components/CCmpUnitMotion.cpp:static const CColor OVERLAY_COLOR_LONG_PATH(1, 1, 1, 1);
simulation2/components/CCmpUnitMotion.cpp:static const CColor OVERLAY_COLOR_SHORT_PATH(1, 0, 0, 1);
simulation2/components/CCmpRangeManager.cpp:	static const player_id_t MAX_LOS_PLAYER_ID = 16;

network/NetFileTransfer.h:static const size_t DEFAULT_FILE_TRANSFER_PACKET_SIZE = 1024;
network/NetFileTransfer.h:static const size_t DEFAULT_FILE_TRANSFER_WINDOW_SIZE = 32;
network/NetFileTransfer.h:static const size_t MAX_FILE_TRANSFER_SIZE = 8*MiB;
network/NetHost.h:	static const int DEFAULT_CHANNEL = 0;
network/NetServer.cpp:static const int CHANNEL_COUNT = 1;
network/NetServer.cpp:static const int HOST_SERVICE_TIMEOUT = 50;
network/NetSession.cpp:static const int CHANNEL_COUNT = 1;

ps/KeyName.cpp:static const SKeycodeMapping keycodeMapping[] =
ps/Preprocessor.cpp:    static const char newlines [8] =
ps/CLogger.cpp:static const double RENDER_TIMEOUT = 10.0; // seconds before messages are deleted
ps/CLogger.cpp:static const double RENDER_TIMEOUT_RATE = 10.0; // number of timed-out messages deleted per second
ps/CLogger.cpp:static const size_t RENDER_LIMIT = 20; // maximum messages on screen at once
ps/scripting/JSInterface_ConfigDB.cpp:static const std::unordered_set<std::string> g_ProtectedConfigNames = {
ps/CConsole.cpp:		static const char newline = '\n';
ps/GameSetup/GameSetup.cpp:static const int SANE_TEX_QUALITY_DEFAULT = 5;	// keep in sync with code
ps/Replay.cpp:static const int PROFILE_TURN_INTERVAL = 20;
ps/UserReport.cpp:static const int REPORTER_VERSION = 1;
ps/UserReport.cpp:static const double TIMER_CHECK_INTERVAL = 10.0;
ps/UserReport.cpp:static const double RECONNECT_INVERVAL = 60.0;
ps/TemplateLoader.cpp:static const wchar_t TEMPLATE_ROOT[] = L"simulation/templates/";
ps/TemplateLoader.cpp:static const wchar_t ACTOR_ROOT[] = L"art/actors/";
ps/Profiler2.h:	static const size_t MAX_ATTRIBUTE_LENGTH; // includes null terminator, which isn't stored
ps/Profiler2.h:	static const u8 RESYNC_MAGIC[8];
ps/Profiler2.h:	static const size_t BUFFER_SIZE;
ps/Profiler2.h:	static const size_t HOLD_BUFFER_SIZE;
ps/TouchInput.h:	static const size_t MAX_MOUSE = 2;
ps/TouchInput.h:	static const size_t MAX_FINGERS = 2;
ps/ConfigDB.cpp:static const std::unordered_set<std::string> g_UnloggedEntries = {

The constants in this diff in fact must be members if derived classes shall be able to fire events defined in base classes, and that is something that should be possible by design (unless adding a wrapper/proxy function, which would be further increasing code complexity and perhaps even performance footprint).

Derived/base classes already are:

source/gui/ObjectBases/IGUIButtonBehavior.h (5 lines)	
M			source/gui/ObjectBases/IGUIButtonBehavior.cpp (13 lines)	
M			source/gui/ObjectBases/IGUIObject.h (29 lines)	
M			source/gui/ObjectBases/IGUIObject.cpp (79 lines)	
M			source/gui/ObjectTypes/CInput.h (4 lines)	
M			source/gui/ObjectTypes/CInput.cpp (26 lines)	
M			source/gui/ObjectTypes/CList.h (5 lines)	
M			source/gui/ObjectTypes/CList.cpp (16 lines)	
M			source/gui/ObjectTypes/CMiniMap.h (2 lines)	
M			source/gui/ObjectTypes/CMiniMap.cpp (4 lines)	
M			source/gui/ObjectTypes/COList.h (2 lines)	
M			source/gui/ObjectTypes/COList.cpp (4 lines)	
M			source/gui/ObjectTypes/CSlider.h (1 line)	
M			source/gui/ObjectTypes/CSlider.cpp (4 lines)

It is inviting to reuse these constants and it should be possible.
For example CButton or CCheckbox that implement IGUIButtonBehavior shall be able to fire the events in IGUIButtonBehavior,
COList shall be able to fire events from IGUIObject.
IGUIObject.cpp and derived might want to fire some events of CGUI.h
CRadioButton should be able to fire events from CCheckBox if it had some.

It's also not inconceivable that one of the TurnManagers may want to trigger an event defined in the base classes, especially if we add some in the future (for example networked replays).

elexis edited the test plan for this revision. (Show Details)Jan 15 2020, 4:58 PM
This revision was not accepted when it landed; it landed in state Needs Review.Jan 15 2020, 5:00 PM
This revision was automatically updated to reflect the committed changes.
Owners added subscribers: Restricted Owners Package, Restricted Owners Package.Jan 15 2020, 5:00 PM