Details

Reviewers: None
Commits: rP23020: Remove virtual inheritance from the codebase, refs rP73, rP141 rP290.

Summary

Following recent discussions with Vladislav about virtual inheritance in the context of rP22596 / D2136,
a measurement that showed that dynamic_cast is 80 times slower than static_cast to determine the child class from the virtually inherited class, and following the Coding_Conventions recommending that too,
this patch removes the virtual inheritance by contiuing to inherit all base classes (but not virtually), passing the IGUIObject& *this reference to the base classes, and registering the GetTextSize function in the leaf class (CText).

Hence the patch removes the forced workaround for the dynamic_cast and allows future code like D1346 to be introduced without virtual inheritance.

Cheesy graphics:

Before the patch, virtual inheritance solved the "Diamond Problem" https://en.wikipedia.org/wiki/Multiple_inheritance#The_diamond_problem

With this patch applied the dependency graph looks like this (gray = pointer):

Test Plan

Verify that code behavior is not changed.

Notice that inheritance order matters. If the IGUIObject is not constructed first, the IButtonBehavior base class will call RegisterSetting on the uninitialized GUI object.
Notice that the JSInterface_IGUITextOwner removal is necessary, because the IGUIObject* stored using JS SetPrivate cannot be statically cast to IGUITextOwner* since they're not inherited anymore. (static_cast from 'IGUIObject *' to 'IGUITextOwner *', which are not related by inheritance, is not allowed)
Notice that we can cast the IGUIObject* to CText* and then operate from there, which is strictly more correct since the previous GetTextSize function only return the size of the first CGUIText, while it can store multiple. So every leaf class can decide more closely which values it wants to return.
Notice the IGUIObject::foo function calls may not be translated to m_pObject.foo calls, because that would not call the base class function but the overriding function of the leaf class. The most simple and failsafe alternative is to make it a responsibility of the leaf class to call the inherited functions when there are multiple of them. So this is the greatest behavior change in this diff and it can be verified by going through all classes of IGUITextOwner, IGUIScrollBarOwner, IGUIButtonBehavior and ensuring that they are called from the classes that inherit them. The classes that inherit them are identified by these classnames appearing in the class definition in the header.
Observe that the functions are ordered consistently and in a way that minimizes fragmentation of virtual function calls on different bases.
Notice that IGUIButtonBehavior would be much cleaner if it would inherit IGUIObject. But that would break the symmetry. The approach should be scalable, so that developers can chose from many more base classes to inherit from. For that to become possible, none of them should be inheriting IGUIObject, thus IGUIButtonBehavior neither.
Notice IGUIScrollBarOwner doesn't use the object yet, but it seems probable that it will be used soon, as it is in the inheritance chain. The benefit of not passing the IGUIObject& but the CGUI& is avoiding one GetGUI each draw call and reducing the number of lines in this diff. But passing IGUIObject& also means that the base classes are more symmetrical and consistent.
Notice that all function implementations in the three base classes are moved to the cpp file.
Ensure that there are no remanants of the removed in rP22596.
Ensure that all functions have appropriate yet consistent comments.

Diff Detail

Repository

rP 0 A.D. Public Repository

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

elexis created this revision.Sep 23 2019, 7:44 PM

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/308/display/redirect

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

Linter detected issues:
Executing section Source...

source/gui/CCheckBox.h
|  23| class·CCheckBox·:·public·IGUIObject
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCCheckBox:' is invalid C code. Use --std or --language to configure the language.

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

source/gui/CButton.h
|  25| class·CButton·:·public·IGUITextOwner
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCButton:' is invalid C code. Use --std or --language to configure the language.

source/gui/IGUIButtonBehavior.h
|  38| class·IGUIButtonBehavior
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classIGUIButtonBehavior{' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/823/display/redirect

Harbormaster completed remote builds in B9588: Diff 9949.Sep 23 2019, 7:53 PM

Rebase, remove DrawButton duplication.

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/844/display/redirect

Harbormaster failed remote builds in B9623: Diff 9980!Sep 27 2019, 4:59 PM

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/329/display/redirect

It seems this is not a sufficient proof of concept yet for being the right implementation.
I should start wit the IGUITextOwner.
The JS functions JSI_IGUITextOwner::GetTextSize only receive the IGUIObject pointer, but not every IGUIObject has the ButtonBehavior delegate thing.
So it seems one has to use inheritance regardless (just not virtual inheritance).

source/gui/IGUIButtonBehavior.cpp
136 ↗	(On Diff #9980)	`IsMouseOver()` is not trivially the same as `m_MouseHovering`. bool IGUIObject::IsMouseOver() const { return m_CachedActualSize.PointInside(m_pGUI.GetMousePos()); } So it's less performant even (4 conditions instead of 1 returned bool). So I wonder if `IsMouseOver()` shouldn't even return `m_MouseHovering`. But CDropdown and MiniMap override this. So there could be reasonably a ButtonBehavior user that also overrides that. So it must actually use `IsMouseOver()`, but that still doesnt mean that IGUIObject cant return the bool for the default implementation. Both the (formerly known as) `MouseOver` function and the member variable both come from rP74 and I still don't see a reason why the function should compute something that the private boolean already knows. As long as `UpdateMouseOver` is called, as long as `ResetStates()` isn't forgotton, the member should have the right value. If one of these calls was missed, then also the events would not be sent correctly in `UpdateMouseOver`. I'd prefer to do that in a commit elsewhere, so that if that was a wrong assumption, it doesnt invalidate this diff. Also `MouseOverIcon()` seems like something that shouldnt even be defined in `IGUIObject`, but should be in a derived class or delegate.
source/gui/IGUIObject.h
140 ↗	(On Diff #9980)	I'd prefer to make this public rather than making the base classes friend classes in this class, since that would hardcode which classes are allowed to use that function. Coders should be able to introduce new IGUIObject classes (including base classes and delegates) without having having to change IGUIObject.

elexis mentioned this in D2333: Remove redundant IGUITextOwner::AddText IGUIObject argument.Sep 28 2019, 1:39 PM

elexis retitled this revision from Remove IGUIButtonBehavior virtual inheritance to Remove virtual inheritance.Sep 28 2019, 2:20 PM

elexis edited the summary of this revision. (Show Details)

elexis edited the test plan for this revision. (Show Details)

Remove all virtual inheritance.

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/858/display/redirect

Harbormaster failed remote builds in B9638: Diff 9995!Sep 28 2019, 2:21 PM

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/342/display/redirect

elexis mentioned this in D1346: List multiselection.Sep 29 2019, 1:21 PM

elexis edited the test plan for this revision. (Show Details)

elexis edited the summary of this revision. (Show Details)Sep 30 2019, 11:04 AM

elexis edited the test plan for this revision. (Show Details)Sep 30 2019, 12:09 PM

Maybe this could be of interest https://stackoverflow.com/questions/4899657/performance-impact-of-virtual-inheritance

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

Linter detected issues:
Executing section Source...

source/gui/CChart.h
|  38| »   std::vector<CVector2D>·m_Points;
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'std::vector' is invalid C code. Use --std or --language to configure the language.

source/gui/CCheckBox.h
|  24| class·CCheckBox·:·public·IGUIObject,·public·IGUIButtonBehavior
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCCheckBox:' is invalid C code. Use --std or --language to configure the language.

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

source/gui/CList.h
|  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.

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

source/gui/CButton.h
|  27| class·CButton·:·public·IGUIObject,·public·IGUITextOwner,·public·IGUIButtonBehavior
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCButton:' is invalid C code. Use --std or --language to configure the language.

source/gui/CTooltip.h
|  28| class·CTooltip·:·public·IGUIObject,·public·IGUITextOwner
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCTooltip:' is invalid C code. Use --std or --language to configure the language.

source/gui/IGUIScrollBarOwner.h
|  32| class·IGUIScrollBarOwner
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classIGUIScrollBarOwner{' is invalid C code. Use --std or --language to configure the language.

source/gui/IGUITextOwner.h
|  44| class·IGUITextOwner
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classIGUITextOwner{' is invalid C code. Use --std or --language to configure the language.

source/gui/CText.h
|  29| class·CText·:·public·IGUIObject,·public·IGUIScrollBarOwner,·public·IGUITextOwner
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCText:' is invalid C code. Use --std or --language to configure the language.

source/gui/IGUIButtonBehavior.h
|  38| class·IGUIButtonBehavior
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classIGUIButtonBehavior{' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/878/display/redirect

elexis edited the test plan for this revision. (Show Details)Sep 30 2019, 12:48 PM

Excerpt of the testplan performance:

CInput::HandleMessage is the only thing not ordered correctly, and that wasn't changed by the patch: void CInput::HandleMessage(SGUIMessage& Message), void CInput::UpdateCachedSize().

IGUIButtonBehavior::ResetStates() call IGUIObject::ResetStates. Now CButton, CCheckbox must perform that. CRadioButton inherits.
IGUIButtonBehavior::HandleMessage function and calls were not changed.

IGUIScrollBarOwner is inherited by CList, CInput, CText.
IGUIScrollBarOwner::HandleMessage, function unchanged, calls unchanged.
IGUIScrollBarOwner::ResetStates() previously called IGUIObject::ResetStates();, this is now being done by the 3 child classes, in the same order.
IGUIScrollBarOwner::Draw() function and calls unchanged.

IGUITextOwner::HandleMessage function and calls unchanged.
Notice IGUIObject::HandleMessage(Message); is empty, so not calling it in the child class CText should compile to the same binary as calling it. But calling it would make it more accurate in case someone decides to insert a statement there. However it also makes it more complicated with increasing inheritance, for example CDropdown::HandleMessage calling CList::HandleMessage etc. . Just gonna add for completeness. Notice CProgressBar already has the useless statement with an // Import over it.
IGUITextOwner::UpdateCachedSize is called by CList, CButton, CText. IGUITextOwner is inherited by these and by CTooltip and CChart, so these were missing it, but only in this version of the patch, not in the committed code, since IGUITextOwner::UpdateCachedSize called IGUIObject::UpdateCachedSize.
IGUITextOwner::SetupText is only implemented by leaf classes.
IGUITextOwner::DrawText is not overridden anywhere.

CDropDown::HandleMessage and COList::HandleMessage both call CList::HandleMessage which takes care of distributing the calls to the base classes. The two classes don't override ResetStates(), so the inherited CList takes care of ResetStates().

MouseOverIcon() always returns false except for CText. Its only used for the weird tooltip code. So not broken, yet should be improved elsewhere later independently.

With regards to the function implementation order:

void CButton::SetupText()
void CButton::ResetStates()
void CButton::UpdateCachedSize()
void CButton::HandleMessage(SGUIMessage& Message)
void CButton::Draw()

void CChart::HandleMessage(SGUIMessage& Message)
void CChart::DrawLine(const CShaderProgramPtr& shader, const CGUIColor& color, const std::vector<float>& vertices) const

void CCheckBox::ResetStates()
void CCheckBox::HandleMessage(SGUIMessage& Message)
void CCheckBox::Draw()


void CDropDown::SetupText()
void CDropDown::UpdateCachedSize()
void CDropDown::HandleMessage(SGUIMessage& Message)
InReaction CDropDown::ManuallyHandleEvent(const SDL_Event_* ev)


void CInput::UpdateBufferPositionSetting()
void CInput::ClearComposedText()
InReaction CInput::ManuallyHandleEvent(const SDL_Event_* ev)
void CInput::ManuallyMutableHandleKeyDownEvent(const SDL_Keycode keyCode)
void CInput::ManuallyImmutableHandleKeyDownEvent(const SDL_Keycode keyCode)
InReaction CInput::ManuallyHandleHotkeyEvent(const SDL_Event_* ev)
void CInput::HandleMessage(SGUIMessage& Message)
void CInput::UpdateCachedSize()
void CInput::Draw()
void CInput::UpdateText(int from, int to_before, int to_after)


void CList::SetupText()
void CList::ResetStates()
void CList::UpdateCachedSize()
void CList::HandleMessage(SGUIMessage& Message)
InReaction CList::ManuallyHandleEvent(const SDL_Event_* ev)
void CList::Draw()
void CList::DrawList(const int& selected, const CGUISpriteInstance& sprite, const CGUISpriteInstance& sprite_selectarea, const CGUIColor& textcolor)


void COList::SetupText()
CRect COList::GetListRect() const
void COList::HandleMessage(SGUIMessage& Message)

void CProgressBar::HandleMessage(SGUIMessage& Message)

void CRadioButton::HandleMessage(SGUIMessage& Message)

float CSlider::GetSliderRatio() const
void CSlider::IncrementallyChangeValue(const float difference)
void CSlider::HandleMessage(SGUIMessage& Message)
void CSlider::Draw()

void CText::SetupText()
void CText::ResetStates()
void CText::UpdateCachedSize()
void CText::HandleMessage(SGUIMessage& Message)
void CText::Draw()

void CTooltip::SetupText()
void CTooltip::HandleMessage(SGUIMessage& Message)
void CTooltip::Draw()

bool GUITooltip::GetTooltip(IGUIObject* obj, CStr& style)

void IGUIButtonBehavior::ResetStates()
void IGUIButtonBehavior::HandleMessage(SGUIMessage& Message)

void IGUIObject::ResetStates()
void IGUIObject::UpdateCachedSize()

void IGUIScrollBar::HandleMessage(SGUIMessage& Message)

void IGUIScrollBarOwner::ResetStates()
void IGUIScrollBarOwner::AddScrollBar(IGUIScrollBar* scrollbar)
const SGUIScrollBarStyle* IGUIScrollBarOwner::GetScrollBarStyle(const CStr& style) const
void IGUIScrollBarOwner::HandleMessage(SGUIMessage& msg)

That should be it.

Another prima facie test at last, structree, template viewer dialog, summary screen charts, replay menu, gamesetup dropdowns, ToS text dialog, all good (that is without testing all possible combinations, for example where an event is sent before the first draw call etc..)

source/gui/IGUIButtonBehavior.cpp
141 ↗	(On Diff #9995)	One could make this a cached member too, but doesn't sound necessary and increases the complexity.

Add some unneedeed and some missing inherited calls.
Remove some remnants of JSInterface_IGUITextOwnere.

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/365/display/redirect

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

Linter detected issues:
Executing section Source...

source/gui/CChart.h
|  38| »   std::vector<CVector2D>·m_Points;
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'std::vector' is invalid C code. Use --std or --language to configure the language.

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

source/gui/CTooltip.h
|  28| class·CTooltip·:·public·IGUIObject,·public·IGUITextOwner
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCTooltip:' is invalid C code. Use --std or --language to configure the language.

source/gui/IGUIScrollBarOwner.h
|  32| class·IGUIScrollBarOwner
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classIGUIScrollBarOwner{' is invalid C code. Use --std or --language to configure the language.

source/gui/CList.h
|  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.

source/gui/CButton.h
|  27| class·CButton·:·public·IGUIObject,·public·IGUITextOwner,·public·IGUIButtonBehavior
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCButton:' is invalid C code. Use --std or --language to configure the language.

source/gui/IGUITextOwner.h
|  44| class·IGUITextOwner
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classIGUITextOwner{' is invalid C code. Use --std or --language to configure the language.

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

source/gui/CText.h
|  29| class·CText·:·public·IGUIObject,·public·IGUIScrollBarOwner,·public·IGUITextOwner
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCText:' is invalid C code. Use --std or --language to configure the language.

source/gui/CCheckBox.h
|  24| class·CCheckBox·:·public·IGUIObject,·public·IGUIButtonBehavior
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCCheckBox:' is invalid C code. Use --std or --language to configure the language.

source/gui/IGUIButtonBehavior.h
|  38| class·IGUIButtonBehavior
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classIGUIButtonBehavior{' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/881/display/redirect

Harbormaster completed remote builds in B9670: Diff 10027.Sep 30 2019, 3:44 PM

Coding_Conventions say

Don't use RTTI (dynamic_cast etc). Exception: source/tools/atlas/AtlasUI/ can use RTTI.

We no line in the codebase with "class " + "virtual" after the patch.
When searching for dynamic_cast we find:

tools/atlas/AtlasUI/General/Observable.h:		*dynamic_cast<T*>(this) = rhs;
tools/atlas/AtlasUI/ScenarioEditor/Sections/Map/Map.cpp:	AtObj scriptSettings = dynamic_cast<AtObjClientData*>(scriptChoice->GetClientObject(scriptChoice->GetSelection()))->GetValue();
tools/atlas/AtlasUI/ScenarioEditor/Sections/Player/Player.cpp:			wxStringClientData* str = dynamic_cast<wxStringClientData*>(choice->GetClientObject(j));
tools/atlas/AtlasUI/ScenarioEditor/Sections/Player/Player.cpp:				wxStringClientData* str = dynamic_cast<wxStringClientData*>(choice->GetClientObject(j));
tools/atlas/AtlasUI/ScenarioEditor/Sections/Player/Player.cpp:			wxStringClientData* str = dynamic_cast<wxStringClientData*>(choice->GetClientObject(choice->GetSelection()));
tools/atlas/AtlasUI/ScenarioEditor/Sections/Player/Player.cpp:				wxStringClientData* str = dynamic_cast<wxStringClientData*>(choice->GetClientObject(choice->GetSelection()));
gui/GUITooltip.cpp:	// Must be a CTooltip*, but we avoid dynamic_cast
gui/GUITooltip.cpp:	// Must be a CTooltip*, but we avoid dynamic_cast
gui/GUITooltip.cpp:	// Must be a CTooltip*, but we avoid dynamic_cast

The last ones will be candy that can be removed separately, the entire class should be considered to be refactored. There would be not few code behavior changes with regards to GetSetting so I would recommend to separate it from this diff (only removing the wording dynamic_cast since it doesnt apply anymore).

IRC discussions can be found via keyword search inheritance + Vladislav:

2019-07/2019-07-30-QuakeNet-#0ad-dev.log:16:25 < Vladislav> elexis: about 2136, I already said to wraitii, that we have enough information about types. I'd prefer to use JS proxy/inheritance instead of such workarounds (for an earlier function present check).
2019-07/2019-07-30-QuakeNet-#0ad-dev.log:16:44 < Vladislav> elexis: I think an object shouldn't be derived from IGUIObject twice, it seems strange. It needs to use virtual inheritance.
2019-07/2019-07-30-QuakeNet-#0ad-dev.log:17:10 < Vladislav> elexis: I think we need to refactor it and remove multiple inheritance from one base class.
2019-07/2019-07-30-QuakeNet-#0ad-dev.log:17:30 < Vladislav> I don't want to remove inheritance, I want to remove multiple inheritance from a base class, since I don't see a strong reason for that.
2019-07/2019-07-30-QuakeNet-#0ad-dev.log:17:33 < Vladislav> The problem that we have objects that have virtual inheritance from IGUIObject and that don't have.
2019-07/2019-07-30-QuakeNet-#0ad-dev.log:18:43 < elexis> Vladislav: the only thing one can do is remove virtual inheritance as far as I see
2019-07/2019-07-30-QuakeNet-#0ad-dev.log:18:54 < Vladislav> elexis: yep, you can do static_cast for non-virtual inheritance.
2019-07/2019-07-30-QuakeNet-#0ad-dev.log:21:34 < Vladislav> In common words - yes, in fact we don't have multiple inheritance in the JS interface tree. So we can use it.
2019-08/2019-08-22-QuakeNet-#0ad-dev.log:23:59 < Vladislav> I'd like to remove multiple inheritance.
2019-08/2019-08-23-QuakeNet-#0ad-dev.log:00:03 < Vladislav> It's not multiple inheritance.
2019-09/2019-09-19-QuakeNet-#0ad-dev.log:18:50 < elexis> Vladislav: more virtual inheritance for COList proposed, should we use COListImpl instead to abstract duplicate code?
2019-09/2019-09-20-QuakeNet-#0ad-dev.log:18:59 < elexis> Vladislav: problem with those are that they dont work well when requiring inheritance of multiple classes, also all the classes need to communicate with each other, i.e. CList with IGUIScrollBarOwner, CList with IGUITextOwner, IGUITextOwner with CList, IGUIScrollBarOwner with CList
2019-09/2019-09-20-QuakeNet-#0ad-dev.log:19:03 < Vladislav> You try to fit current classes into non-virtual inheritance. My thoughts are about refactoring of our current heirarchy.
2019-09/2019-09-22-QuakeNet-#0ad-dev.log:09:39 < elexis> Vladislav mentioned three ways to get rid of the virtual inheritance and I understood none of them
2019-09/2019-09-23-QuakeNet-#0ad-dev.log:20:24 < elexis> Vladislav: any specific better idea for virtual inheritance removal than that diff? otherwise I'll remove the virtualenss that way

This revision was not accepted when it landed; it landed in state Needs Review.Sep 30 2019, 4:08 PM

Closed by commit rP23020: Remove virtual inheritance from the codebase, refs rP73, rP141 rP290. (authored by elexis). · Explain Why

This revision was automatically updated to reflect the committed changes.

elexis added a commit: rP23020: Remove virtual inheritance from the codebase, refs rP73, rP141 rP290..

elexis added inline comments.Oct 4 2019, 11:41 PM

source/gui/IGUIButtonBehavior.cpp
136 ↗	(On Diff #9980)	Actually there is a difference between `m_MouseHovering` and `IsMouseOver`. The `m_MouseHovering` is not true when the mouse is in the according rect but hidden by another object. Hence hovering dropdown item still highlights button thats below the dropdown list:

elexis mentioned this in rP23020: Remove virtual inheritance from the codebase, refs rP73, rP141 rP290..Oct 4 2019, 11:42 PM

elexis mentioned this in D2354: Correct button hovering following rP23020 / D2325, clarify difference between IsMouseOver and IsMouseHovering following rP74.Oct 5 2019, 11:46 AM

elexis mentioned this in rP23054: Correct button hovering following rP23020 / D2325, clarify difference between….Oct 5 2019, 11:58 AM

elexis mentioned this in D2592: CSlider IGUIButtonBehavior inheritance to gain Sound, Press and Release events.Jan 24 2020, 1:53 AM

wraitii mentioned this in D1781: Fix lobby chat performance issue..May 24 2020, 9:54 AM

Remove virtual inheritance
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 10029

ps/trunk/source/gui/CButton.h

ps/trunk/source/gui/CButton.cpp

ps/trunk/source/gui/CChart.h

ps/trunk/source/gui/CChart.cpp

ps/trunk/source/gui/CCheckBox.h

ps/trunk/source/gui/CCheckBox.cpp

ps/trunk/source/gui/CDropDown.cpp

ps/trunk/source/gui/CInput.h

ps/trunk/source/gui/CInput.cpp

ps/trunk/source/gui/CList.h

ps/trunk/source/gui/CList.cpp

ps/trunk/source/gui/COList.cpp

ps/trunk/source/gui/CProgressBar.cpp

ps/trunk/source/gui/CRadioButton.cpp

ps/trunk/source/gui/CSlider.cpp

ps/trunk/source/gui/CText.h

ps/trunk/source/gui/CText.cpp

ps/trunk/source/gui/CTooltip.h

ps/trunk/source/gui/CTooltip.cpp

ps/trunk/source/gui/GUITooltip.cpp

ps/trunk/source/gui/IGUIButtonBehavior.h

ps/trunk/source/gui/IGUIButtonBehavior.cpp

ps/trunk/source/gui/IGUIObject.h

ps/trunk/source/gui/IGUIObject.cpp

ps/trunk/source/gui/IGUIScrollBarOwner.h

ps/trunk/source/gui/IGUIScrollBarOwner.cpp

ps/trunk/source/gui/IGUITextOwner.h

ps/trunk/source/gui/IGUITextOwner.cpp

ps/trunk/source/gui/MiniMap.cpp

ps/trunk/source/gui/scripting/JSInterface_IGUITextOwner.h

ps/trunk/source/gui/scripting/JSInterface_IGUITextOwner.cpp

Remove virtual inheritanceClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 10029

ps/trunk/source/gui/CButton.h

ps/trunk/source/gui/CButton.cpp

ps/trunk/source/gui/CChart.h

ps/trunk/source/gui/CChart.cpp

ps/trunk/source/gui/CCheckBox.h

ps/trunk/source/gui/CCheckBox.cpp

ps/trunk/source/gui/CDropDown.cpp

ps/trunk/source/gui/CInput.h

ps/trunk/source/gui/CInput.cpp

ps/trunk/source/gui/CList.h

ps/trunk/source/gui/CList.cpp

ps/trunk/source/gui/COList.cpp

ps/trunk/source/gui/CProgressBar.cpp

ps/trunk/source/gui/CRadioButton.cpp

ps/trunk/source/gui/CSlider.cpp

ps/trunk/source/gui/CText.h

ps/trunk/source/gui/CText.cpp

ps/trunk/source/gui/CTooltip.h

ps/trunk/source/gui/CTooltip.cpp

ps/trunk/source/gui/GUITooltip.cpp

ps/trunk/source/gui/IGUIButtonBehavior.h

ps/trunk/source/gui/IGUIButtonBehavior.cpp

ps/trunk/source/gui/IGUIObject.h

ps/trunk/source/gui/IGUIObject.cpp

ps/trunk/source/gui/IGUIScrollBarOwner.h

ps/trunk/source/gui/IGUIScrollBarOwner.cpp

ps/trunk/source/gui/IGUITextOwner.h

ps/trunk/source/gui/IGUITextOwner.cpp

ps/trunk/source/gui/MiniMap.cpp

ps/trunk/source/gui/scripting/JSInterface_IGUITextOwner.h

ps/trunk/source/gui/scripting/JSInterface_IGUITextOwner.cpp

Remove virtual inheritance
ClosedPublic
Actions

Revision Contents
Changeset List