Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kill g_Windowing, another nasty global #13047

Merged
merged 6 commits into from Nov 24, 2017

Conversation

@FernetMenta
Copy link
Member

commented Nov 13, 2017

another chapter in the endless story of kill ugly globals

@pkerling this will eventually enable running X11 and Wayland from the same binary.

@pkerling

This comment has been minimized.

Copy link
Member

commented Nov 13, 2017

Nice, very much looking forward to this!

@@ -24,7 +24,7 @@
#include "cores/VideoPlayer/DVDCodecs/Overlay/DVDOverlayImage.h"
#include "cores/VideoPlayer/DVDCodecs/Overlay/DVDOverlaySpu.h"
#include "cores/VideoPlayer/DVDCodecs/Overlay/DVDOverlaySSA.h"
#include "windowing/WindowingFactory.h"
#include "windowing/WinSystem.h"

This comment has been minimized.

Copy link
@notspiff

notspiff Nov 13, 2017

Contributor

No other changes so have to ask; is this include necessary?

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Nov 13, 2017

Author Member

probably not, thanks


void CServiceManager::SetWinSystem(std::unique_ptr<CWinSystemBase> &winSystem)
{
m_winSystem.reset();

This comment has been minimized.

Copy link
@xhaggi

xhaggi Nov 13, 2017

Member

Necessary?

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Nov 13, 2017

Author Member

left over from debugging, thanks

@garbear

This comment has been minimized.

Copy link
Member

commented Nov 13, 2017

awesome. thanks for making the RetroPlayer changes

@@ -768,7 +775,9 @@ bool CApplication::InitWindow(RESOLUTION res)

bool CApplication::DestroyWindow()
{
return g_Windowing.DestroyWindow();
return CServiceBroker::GetWinSystem().DestroyWindow();
std::unique_ptr<CWinSystemBase> winSystem;

This comment has been minimized.

Copy link
@pkerling

pkerling Nov 14, 2017

Member

dead code?

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Nov 14, 2017

Author Member

oops

@lrusak lrusak added this to the L 18.0-alpha1 milestone Nov 14, 2017
@FernetMenta FernetMenta force-pushed the FernetMenta:windowing branch from d79d4f2 to 12a1e04 Nov 17, 2017
@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Nov 17, 2017

@afedchin would be great if you could adapt the Windows ports to this change

@afedchin

This comment has been minimized.

Copy link
Member

commented Nov 17, 2017

It will be not easy and I haven't time on this weekend, so may be at monday.

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Nov 18, 2017

@afedchin thanks!
@peak3d @lrusak @popcornmix could you adapt your platforms too?

@lrusak

This comment has been minimized.

Copy link
Contributor

commented Nov 18, 2017

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Nov 19, 2017

You are a silly bot that knows nothing. This pr is waiting for some contributions

@lrusak

This comment has been minimized.

Copy link
Contributor

commented Nov 20, 2017

Which platforms are we missing here?

  • Windows
  • OSX
  • iOS
  • X11
  • Wayland
  • Mir
  • GBM
  • Android
  • Amlogic
  • RPi
@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Nov 20, 2017

Windows, RPi, Android, (Mir) are missing.

@pkerling

This comment has been minimized.

Copy link
Member

commented Nov 21, 2017

What's the stance on Mir? Is it maintained?

@MartijnKaijser MartijnKaijser requested a review from popcornmix Nov 21, 2017
@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Nov 21, 2017

What's the stance on Mir? Is it maintained?

@BrandonSchaefer ^^

@BrandonSchaefer

This comment has been minimized.

Copy link

commented Nov 21, 2017

Mir now currently supports Wayland core clients So let me test if the that is good enough now. If not, we should maintain a Mir backend until Mir's Wayland support improves.

For this fix though it would be pretty easy to fix up the Mir parts (I can get a diff soon-ish if testing the Wayland core bits isnt good enough yet, also yay no more g_window globa!).

Also more info on Mir and its current direction:
https://community.ubuntu.com/c/mir

@popcornmix

This comment has been minimized.

Copy link
Member

commented Nov 21, 2017

@FernetMenta I have most of pi stuff fixed up. OMXImage is the trickier part:
https://github.com/xbmc/xbmc/blob/master/xbmc/cores/omxplayer/OMXImage.cpp#L339

Can you suggest the desired path to get egl display/context info in the new world?

@FernetMenta FernetMenta force-pushed the FernetMenta:windowing branch from 3f81055 to cc70625 Nov 22, 2017
@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Nov 22, 2017

@popcornmix the fastest method is to typecast the genering interface to the specific one of your implementation. A better way is to pass required info to the instance when you create it.

@FernetMenta FernetMenta force-pushed the FernetMenta:windowing branch 2 times, most recently from e92df47 to f977f8d Nov 22, 2017
@@ -231,7 +234,9 @@ void CMouseStat::SetActive(bool active /*=true*/)
// 1. The mouse is active (it has been moved) AND
// 2. The XBMC mouse is disabled in settings AND
// 3. XBMC is not in fullscreen.
g_Windowing.ShowOSMouse(m_mouseState.active && !IsEnabled() && !g_Windowing.IsFullScreen());
CWinSystemBase &winSystem = CServiceBroker::GetWinSystem();
if (&winSystem)

This comment has been minimized.

Copy link
@Rechi

Rechi Nov 22, 2017

Member

reference cannot be bound to dereferenced null pointer in well-defined C++ code; pointer may be assumed to always convert to true [-Wundefined-bool-conversion]

I think you already have to handle the null pointer case in CWinSystemBase &CServiceManager::GetWinSystem() if it can even happen.

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Nov 22, 2017

Author Member

this is a left-over and will disappear soon when I'll move MouseStat from InputManager to WinSystem

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Nov 22, 2017

@afedchin
C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\include\agile.h(19): fatal error C1189: #error: agile.h can only be used with /ZW (compiling source file C:\jenkins_slave\workspace\WIN-64\xbmc\guilib\D3DResource.cpp)

#endif
#include "xbmc/ServiceBroker.h"

#include <agile.h>

This comment has been minimized.

Copy link
@afedchin

afedchin Nov 22, 2017

Member

please move this inside #elif defined(TARGET_WINDOWS_STORE)
this will fix build error on win64/win32

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Nov 22, 2017

jenkins build this please

3 similar comments
@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Nov 23, 2017

jenkins build this please

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Nov 23, 2017

jenkins build this please

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Nov 23, 2017

jenkins build this please

@garbear

This comment has been minimized.

Copy link
Member

commented Nov 23, 2017

@FernetMenta small improvement for you: FernetMenta#440

@FernetMenta FernetMenta force-pushed the FernetMenta:windowing branch from a5b8b46 to 3090144 Nov 24, 2017
@@ -18,18 +18,17 @@
* http://www.gnu.org/copyleft/gpl.html
*
*/
#pragma once

This comment has been minimized.

Copy link
@Rechi

Rechi Nov 24, 2017

Member

in a cpp file?

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Nov 24, 2017

Author Member

oops :)

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Nov 24, 2017

@popcornmix do you plan to submit PI adaptations in the near future?

@popcornmix

This comment has been minimized.

Copy link
Member

commented Nov 24, 2017

@FernetMenta Builds but need to work out what to do for get egl display/context
popcornmix@c99c47f

but I'm looking into it now.

@FernetMenta FernetMenta force-pushed the FernetMenta:windowing branch from 285fff2 to 1b4c9f2 Nov 24, 2017
@FernetMenta FernetMenta force-pushed the FernetMenta:windowing branch from 1b4c9f2 to 46a4077 Nov 24, 2017
@lrusak

This comment has been minimized.

Copy link
Contributor

commented Nov 24, 2017

Do we want to wait on Mir?

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Nov 24, 2017

I am not aware that we have any/many alpha testers on Mir. Hence moving forward has higher prio. Patches for Mir can be submitted at any time later.

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Nov 24, 2017

Linux-64 test suite error in ThreadLocal unrelated to this PR. Test runs fine on my local system.

@FernetMenta FernetMenta merged commit 4b22591 into xbmc:master Nov 24, 2017
1 check failed
1 check failed
default Sorry, building this PR failed. Please check the logs for the errors.
Details
@FernetMenta FernetMenta deleted the FernetMenta:windowing branch Nov 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.