[Applicaion/GuiComponent] - don't call init/deinit of subcomponents of guicomponent from application #15825

Memphiz · 2019-03-27T21:02:47Z

Instead refactor init/deinit of guicomponent to do the righthing - fixes #15561

This might be the proper fix instead of #13878
It ensures that loadskin/unloadskin calls init/deinit of guicomponent instead of the subcomponent (windowmanager). To make it work the init/deinit of guicomponent was altered to not deregister from servicebroker during deinit - but only in the dtor.
Also the application cleanup now only calls reset on the gui because the dtor already does the deinit (would have been a double deinit before which is a nop).

With this the stereoscopic mode selection works properly again (and it turns off 3d mode after playback if setting is configured that way)

Memphiz · 2019-03-27T21:03:42Z

@FernetMenta did i get your intention right here? At least that is what i understood from your comments here: #13743

FernetMenta · 2019-03-28T07:53:32Z

@Memphiz if you want to fix #13878 I suggest https://pastebin.com/BX5makLi

Memphiz · 2019-03-28T15:50:49Z

But with this you still have application init a sub component of GUI (the window manager) + you added a circular dependency between gui and window manager.
Not sure how this is better?

Also why did you add the GUI message header to all the GUI classes?

FernetMenta · 2019-03-28T16:19:38Z

One step after another. Moving msg target one level higher solves the linked issue but leaves some issues in the design.
Deinitializing a component and leaving it registered on ServiceBroker for other components to use is imo worse. If you want to follow this path you should unregister it and fix all client calls that don't check for gui being nullptr. That would fix it properly.

Memphiz · 2019-03-28T18:18:49Z

Right - that’s worse indeed.
How about modifying your approach in the following way:
Add an interface IGuiMsgHookHandler and add the method ProcessMsgHooks to it.
Pass this interface to the window manager ctor instead of the full blown GUI component.

This would get rid of the circular dependency involving friend class and gives window manager only what it needs from GUI component.

Memphiz · 2019-03-28T20:28:37Z

@FernetMenta updated with your changes and adapted by splitting CGUIComponent up into 2 interfaces ... one for windowmanager (containing the ProcessMsgHooks only) and one for the rest of kodi (containing all but the ProcessMsgHooks).

FernetMenta · 2019-03-29T08:30:15Z

@Memphiz I used the concept of a friend class to protect the msg driver method. I agree, this is ugly but it is recognisable as a quick and dirty approach. If you introduce interfaces, you should start to do it right or upcoming changes will be a mess too. Init/Deinit do not fit into the same interface as methods like GetInfoManager.
If you touch ServiceBroker, you should change the raw pointer to a shared pointer like this is already done for a couple of objects.

Memphiz · 2019-03-29T17:34:13Z

So what’s the preferred approach - using your approach as is or removing deinit/init from the interface, make it a shared pointer in broker and leave application with the complete GUI component object?

FernetMenta · 2019-03-29T18:27:21Z

So what’s the preferred approach

If this is directed to me, you ask the wrong person. I just wanted to give some insights on why my patch looks like it is. Since nobody else cared about the issue for months, I think it's your preference that counts.

da-anda · 2019-04-01T22:16:37Z

xbmc/network/EventServer.cpp

@@ -337,7 +337,7 @@ bool CEventServer::ExecuteNextAction()
          unsigned int actionID;
          CActionTranslator::TranslateString(actionEvent.actionName, actionID);
          CAction action(actionID, 1.0f, 0.0f, actionEvent.actionName);
-          CGUIComponent* gui = CServiceBroker::GetGUI();
+          IGUIComponent* gui = CServiceBroker::GetGUI();
          if (gui)


I know this is all very much WIP and there is a long way to go with refactoring things step by step, but by browsing those changes I really wondered why the EventServer needs to know anything about the GUI? Having it trigger a sound by itself is IMO terribly wrong and should be handled by g_application.OnAction(action) consumers. I know it's something for a different patch, just wondered if this oddity will be gone in the long run.

Well this oddity will go as soon as someone makes it go ;) - I have no plans for the GUI component I just wanted to fix the stereoscopic regression in a way that was accepted.

Memphiz · 2019-04-04T19:00:37Z

updated. removed init/deinit from the interface and made gui a shared_ptr in application an servicebroker.

xbmc/Application.cpp

FernetMenta · 2019-04-09T11:44:27Z

xbmc/guilib/GUIComponent.cpp

 }

 void CGUIComponent::Deinit()
 {
-  CServiceBroker::UnregisterGUI();


unregister from ServiceBroker is mandatory here. Deinit puts the service out of order

But it’s inconsistent to have it done in the object that gets registered. The owner is application - the owner is responsible to resolve the injected dependencies - like it is done with other broker registrees already.

you only unregister it at cleanup but it has to be unregistered during unload/load skin and maybe other operation too. encapsulation makes sense here.
btw: unregister needs to wait until last client has released the shared pointer. that's the reason why it is a shared pointer: https://github.com/xbmc/xbmc/blob/master/xbmc/Application.cpp#L2535

Is this new behavior? Can't see where the GUI was deregistered during unload/load skin before tbh. But its a good idea to change skins only while no GUI Users are calling into the component for sure.

I have added some more commits towards your directions. Though the check for the gui pointer might be not easily doable ... i started it but still have 1025 hits on unguarded accesses ... :(

Is this new behavior? Can't see where the GUI was deregistered during unload/load skin before tbh

Sorry, I forgot to mention that this was my personal view on this, not an official direction of team kodi.
The scenario I had in mind was i.e. that you run headless kodi, launch an X11 desktop and Kodi Ui gets visible, close the desktop -> Kodi falls back to headless, log on to Wayland -> you get Wayland UI of Kodi, ...

As far as I am concerned nobody in team kodi has GUI ownership atm - so no need to stress that your opinion is not official or what not. Just discussing improvements here and trying to find a way to give that shared pointer a meaning ;)

True and true.
We still want to support the scenario you mentioned @FernetMenta and appreciate your feedback a lot.
I think I speak for the team, when I say this.

…et - workaround for xbmc#13878 for leia" This reverts commit 5424c25.

…Component

…lled in via windowmanager anymore

…ion the full CGuiComponent as member

…er from gui component to application

…to own methods and prepare for being able to activate/deactivate the gui component by utilising the shared pointer reference counter

jenkins4kodi · 2019-06-09T20:08:46Z

@Memphiz this needs a rebase

phunkyfish · 2020-03-08T22:44:10Z

@Memphiz would you like to progress this, close it or put it on the backburner?

Memphiz added Type: Fix non-breaking change which fixes an issue Component: GUI engine v18 Leia labels Mar 27, 2019

Memphiz force-pushed the fix_stereoscopic branch 2 times, most recently from 6c1df18 to b2884a4 Compare March 28, 2019 20:27

Memphiz mentioned this pull request Mar 30, 2019

[Application] - register Stereoscopicmanager for message target #15845

Merged

da-anda reviewed Apr 1, 2019

View reviewed changes

Memphiz added v19 Matrix and removed v18 Leia labels Apr 4, 2019

Memphiz force-pushed the fix_stereoscopic branch from e1a2a61 to 7938666 Compare April 4, 2019 18:59

Memphiz force-pushed the fix_stereoscopic branch from 7938666 to 46a06e6 Compare April 4, 2019 19:03

jenkins4kodi added the Rebase needed PR that does not apply/merge cleanly to current base branch label Apr 4, 2019

afedchin reviewed Apr 5, 2019

View reviewed changes

xbmc/Application.cpp Outdated Show resolved Hide resolved

FernetMenta reviewed Apr 9, 2019

View reviewed changes

xbmc/Application.cpp Outdated Show resolved Hide resolved

FernetMenta reviewed Apr 9, 2019

View reviewed changes

Memphiz and others added 5 commits April 9, 2019 18:57

Revert "[Application] - register Stereoscopicmanager for message targ…

03ef6a3

…et - workaround for xbmc#13878 for leia" This reverts commit 5424c25.

[gui] - move processing of msg callbacks from WindowManager up to Gui…

80ef151

…Component

[Application] - small refactor for registration for msg callback classes

cb95625

[gui] - include guimessage were needed now that this header is not pu…

ffbf186

…lled in via windowmanager anymore

[gui] - cleanup IMsgTargetCallback.h

97cb519

Memphiz added 16 commits April 9, 2019 18:57

IGuiComponent: - removed Deinit/Init from interface and give applicat…

0c77b6c

…ion the full CGuiComponent as member

[ServiceBroker] - make gui a shared ptr

ce7b00f

[GUIComponent] - move registration/deregistration to/from servicebrok…

f4dc9b0

…er from gui component to application

[Application/GuiComponent] - refactor init/deinit of gui component in…

1d854cc

…to own methods and prepare for being able to activate/deactivate the gui component by utilising the shared pointer reference counter

AddonDll: - check gui pointer

ca547e2

AddonCallbacksGUI: - check gui pointer

343afe9

Label: - check gui pointer

29b7cff

SettingsSlider: - check gui pointer

e6ba646

Slider: - check gui pointer

248d253

Spin: - check gui pointer

06b5990

TextBox: - check gui pointer

9eb613e

ContextMenu: - check gui pointer

3542058

ExtendedProgressBar: - check gui pointer

70708d9

Progress: - check gui pointer

85abb29

Select: - check gui pointer

a144c5b

TextViewer: - check gui pointer

4f90199

Memphiz force-pushed the fix_stereoscopic branch from 46a06e6 to 4f90199 Compare April 9, 2019 18:49

jenkins4kodi removed the Rebase needed PR that does not apply/merge cleanly to current base branch label Apr 9, 2019

Memphiz added 2 commits April 9, 2019 21:08

PVR/DTOR: - check gui pointer in dtors

c896ed0

AddonCallbacksGUI: - some more gui pointer checks

4dbdfb7

jenkins4kodi added the Rebase needed PR that does not apply/merge cleanly to current base branch label Jun 9, 2019

a1rwulf mentioned this pull request Aug 8, 2019

Fix abort due to double busy dialog #16450

Closed

13 tasks

phunkyfish added the PR Cleanup: Ask Author PR appears to still be relevant and mergeable. Comment to author to check on status. label Mar 8, 2020

Memphiz closed this Mar 9, 2020

Rechi removed the v19 Matrix label Mar 9, 2020

phunkyfish added PR Cleanup: Author Closed PR closed on the authors request. and removed PR Cleanup: Ask Author PR appears to still be relevant and mergeable. Comment to author to check on status. labels Mar 9, 2020

phunkyfish added this to the Abandoned, obsolete or superseeded milestone Mar 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Applicaion/GuiComponent] - don't call init/deinit of subcomponents of guicomponent from application #15825

[Applicaion/GuiComponent] - don't call init/deinit of subcomponents of guicomponent from application #15825

Memphiz commented Mar 27, 2019 •

edited

Loading

Memphiz commented Mar 27, 2019

FernetMenta commented Mar 28, 2019

Memphiz commented Mar 28, 2019

FernetMenta commented Mar 28, 2019

Memphiz commented Mar 28, 2019

Memphiz commented Mar 28, 2019

FernetMenta commented Mar 29, 2019

Memphiz commented Mar 29, 2019

FernetMenta commented Mar 29, 2019

da-anda Apr 1, 2019

Memphiz Apr 2, 2019 •

edited

Loading

Memphiz commented Apr 4, 2019

FernetMenta Apr 9, 2019

Memphiz Apr 9, 2019

FernetMenta Apr 9, 2019

Memphiz Apr 9, 2019

Memphiz Apr 9, 2019

FernetMenta Apr 9, 2019

Memphiz Apr 10, 2019

a1rwulf Apr 10, 2019

jenkins4kodi commented Jun 9, 2019

phunkyfish commented Mar 8, 2020

[Applicaion/GuiComponent] - don't call init/deinit of subcomponents of guicomponent from application #15825

[Applicaion/GuiComponent] - don't call init/deinit of subcomponents of guicomponent from application #15825

Conversation

Memphiz commented Mar 27, 2019 • edited Loading

Memphiz commented Mar 27, 2019

FernetMenta commented Mar 28, 2019

Memphiz commented Mar 28, 2019

FernetMenta commented Mar 28, 2019

Memphiz commented Mar 28, 2019

Memphiz commented Mar 28, 2019

FernetMenta commented Mar 29, 2019

Memphiz commented Mar 29, 2019

FernetMenta commented Mar 29, 2019

Choose a reason for hiding this comment

Memphiz Apr 2, 2019 • edited Loading

Choose a reason for hiding this comment

Memphiz commented Apr 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenkins4kodi commented Jun 9, 2019

phunkyfish commented Mar 8, 2020

Memphiz commented Mar 27, 2019 •

edited

Loading

Memphiz Apr 2, 2019 •

edited

Loading