kill global g_windowManager #13722

FernetMenta · 2018-03-31T19:33:51Z

kills g_windowManager
created gui components that owns gui objects
CWindowManager is embedded into CGUIComponent
StereoscopicManager follows

clients of GUI should check the interface pointer they get from ServiceBroker. GUI won't be mandatory in future version.

xbmc/guilib/GUIComponent.h

+  CGUIWindowManager& GetWindowManager();
+
+protected:
+  std::unique_ptr<CGUIWindowManager> m_pWindowManager;


garbear · 2018-03-31T19:37:54Z

I didn't know you'd come to the rescue with a thousand lines of code ;) I'll review this weekend and relocate StereoscopicsManager.

garbear

PR looks good. I'll port CStereoscopicsManager in the next day or two

xbmc/utils/FileUtils.cpp

@@ -53,7 +54,7 @@ bool CFileUtils::DeleteItem(const CFileItemPtr &item, bool force)
  if (!item || item->IsParentFolder())
    return false;

-  CGUIDialogYesNo* pDialog = g_windowManager.GetWindow<CGUIDialogYesNo>(WINDOW_DIALOG_YES_NO);
+  CGUIDialogYesNo* pDialog = CServiceBroker::GetGUI()->GetWindowManager().GetWindow<CGUIDialogYesNo>(WINDOW_DIALOG_YES_NO);


FernetMenta · 2018-04-01T11:51:51Z

@da-anda 014e8a4 shows the tedious tasks we have to do. No fance framework or technology solves those issues. GUI logic has to be removed from lower layers.

@garbear more bullet points for your how-to:

never ever use default parameters in functions/methods. In our case here test suite was not aware that it requested confirmation dialog for DeleteItem
components like generic utils, network, input, etc, must not depend on GUI.

xbmc/interfaces/legacy/ModuleXbmcvfs.cpp

@@ -78,7 +78,11 @@ namespace XBMCAddon
    bool rmdir(const String& path, bool force)
    {
      DelayedCallGuard dg;
-      return (force ? CFileUtils::DeleteItem(path,force) : XFILE::CDirectory::Remove(path));


Voyager1 · 2018-04-02T21:05:55Z

this segfaults on Windows. There are calls to GetWindowManager() from the WM_PAINT event before the CGUIComponent pointer is even initialized.

xbmc/Application.cpp

@@ -721,7 +722,7 @@ bool CApplication::CreateGUI()
            info.iHeight,
            info.strMode.c_str());

-  g_windowManager.Initialize();
+  m_pGUI.reset(new CGUIComponent());


DaveTBlake · 2018-04-03T07:56:59Z

Would it not be better in future to create test builds for all platforms and do some testing (or ask others to test for you) before merging such large changes?

Because of this PR I just spent several hours on a messy rebase of my work, that has been ready for merge for weeks but waiting for review, only to find it seg faults. Very unimpressed.

[win32] fix crash after #13722 + cosmetics

garbear · 2018-04-03T10:42:24Z

@DaveTBlake killing globals is a thankless task. It takes hours and it's almost guaranteed something will break every time. Nevertheless, it's important that we fight these architectural road blocks. More globals-killing is needed, so keep a lookout for these PRs. Thanks for the understanding.

peak3d · 2018-04-03T13:40:27Z

@garbear yep, I believe that it is an thankless task, and therefore we should produce something for testing. Don't matter if internal or external. I suggest to trigger a test build and ask kodi members to test it on different platforms. If the test version is public available, I could also ask the userbase or we make a thread in forum.kodi.tv for such critical test builds.

FernetMenta · 2018-04-03T13:55:27Z

Well, sorry for the inconvenience but I don't think the trouble was that hard as it seems when reading those complaints. Sorry for the breakage of Windows and PI build and many thanks to @Voyager1 @afedchin @popcornmix for helping out fixing the issues quickly.
Apart from a crash at exit, fix is already provided, current master is fine. Do you really consider this as a major case to worry about? Why not feeling happy that the application has been improved?

DaveTBlake · 2018-04-03T14:27:29Z

Why not feeling happy that the application has been improved?

Happy for that @FernetMenta, well done have a cookie etc.

Unhappy to have wasted 8 hours of my life, which could have been avoided with a better communication. Unhappy that your response to that is "I don't think the trouble was that hard". Well that is just your opinion.

Equally that waste could have been avoided with some very basic platform testing before this extensive PR was merged. Of course testing does not catch everything, but we could at least try. I understand the need for speed to avoid yet more rework, but making test builds available on the mirrors is so easy, and members would use them to test something promptly if asked.

FernetMenta commented Mar 31, 2018

View reviewed changes

xbmc/guilib/GUIComponent.h Outdated

CGUIWindowManager& GetWindowManager();

protected:

std::unique_ptr<CGUIWindowManager> m_pWindowManager;

This comment was marked as spam.

Sign in to view

garbear approved these changes Apr 1, 2018

View reviewed changes

Rechi reviewed Apr 1, 2018

View reviewed changes

FernetMenta commented Apr 1, 2018

View reviewed changes

xbmc/interfaces/legacy/ModuleXbmcvfs.cpp

@@ -78,7 +78,11 @@ namespace XBMCAddon

bool rmdir(const String& path, bool force)

{

DelayedCallGuard dg;

return (force ? CFileUtils::DeleteItem(path,force) : XFILE::CDirectory::Remove(path));

This comment was marked as spam.

Sign in to view

FernetMenta added 2 commits April 1, 2018 14:39

kill global g_windowManager, co-author Rechi'

2dd51ec

remove yes-no dialog from DeleteItem

8c64960

FernetMenta force-pushed the gui branch from 014e8a4 to 8c64960 Compare April 1, 2018 12:40

FernetMenta merged commit 3848940 into xbmc:master Apr 1, 2018

FernetMenta deleted the gui branch April 1, 2018 19:12

Voyager1 reviewed Apr 2, 2018

View reviewed changes

afedchin added a commit to afedchin/xbmc that referenced this pull request Apr 3, 2018

[win32] fix crash after xbmc#13722

261451a

afedchin added a commit that referenced this pull request Apr 3, 2018

Merge pull request #13728 from afedchin/win32-fix

363475b

[win32] fix crash after #13722 + cosmetics

Rechi added Type: Cleanup non-breaking change which removes non-working or unmaintained functionality v18 Leia labels Apr 3, 2018

Rechi added this to the Leia 18.0-alpha2 milestone Apr 3, 2018

DaveTBlake mentioned this pull request Apr 3, 2018

[win32] fix crash after #13722 + cosmetics #13728

Merged

popcornmix mentioned this pull request Apr 3, 2018

[rpi] Protect against null GetGUI #13731

Merged

10 tasks

garbear mentioned this pull request Apr 6, 2018

Kill global CStereoscopicsManager #13743

Merged

4 tasks

Rechi mentioned this pull request Apr 20, 2018

Fix add-ons repository after #13647 #13792

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kill global g_windowManager #13722

kill global g_windowManager #13722

FernetMenta commented Mar 31, 2018

This comment was marked as spam.

garbear commented Mar 31, 2018

garbear left a comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

FernetMenta commented Apr 1, 2018

This comment was marked as spam.

Voyager1 commented Apr 2, 2018

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

DaveTBlake commented Apr 3, 2018

garbear commented Apr 3, 2018

peak3d commented Apr 3, 2018

FernetMenta commented Apr 3, 2018

DaveTBlake commented Apr 3, 2018

kill global g_windowManager #13722

kill global g_windowManager #13722

Conversation

FernetMenta commented Mar 31, 2018

This comment was marked as spam.

garbear commented Mar 31, 2018

garbear left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

FernetMenta commented Apr 1, 2018

This comment was marked as spam.

Voyager1 commented Apr 2, 2018

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

DaveTBlake commented Apr 3, 2018

garbear commented Apr 3, 2018

peak3d commented Apr 3, 2018

FernetMenta commented Apr 3, 2018

DaveTBlake commented Apr 3, 2018