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

feature: implement xcb for windowing #462

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

pollend
Copy link
Contributor

@pollend pollend commented Feb 21, 2023

Description

WIP support that replaces x11 with xcb

@pollend pollend force-pushed the feature/add-support-xcb branch 5 times, most recently from e655449 to 46c0eb0 Compare March 1, 2023 05:20
Comment on lines 27 to 54
void process(const IWindowXcb* window, xcb_generic_event_t* event);
private:
core::smart_refctd_ptr<XcbConnection> m_xcbConnection;

xcb_window_t getClipboardWindow();

struct {
std::string m_data;
std::vector<xcb_atom_t> m_formats;
} m_stagedClipboard;

std::mutex m_clipboardMutex;
std::condition_variable m_clipboardResponseCV;
std::string m_clipboardResponse;
// bool ready = false;

XcbConnection::XCBAtomToken<core::StringLiteral("CLIPBOARD")> m_CLIPBOARD;
XcbConnection::XCBAtomToken<core::StringLiteral("TARGETS")> m_TARGETS;
XcbConnection::XCBAtomToken<core::StringLiteral("INCR")> m_INCR;


XcbConnection::XCBAtomToken<core::StringLiteral("UTF8_STRING")> m_formatUTF8_0;
XcbConnection::XCBAtomToken<core::StringLiteral("text/plain;charset=utf-8")> m_formatUTF8_1;
XcbConnection::XCBAtomToken<core::StringLiteral("text/plain;charset=UTF-8")> m_formatUTF8_2;
XcbConnection::XCBAtomToken<core::StringLiteral("GTK_TEXT_BUFFER_CONTENTS")> m_formatGTK;
XcbConnection::XCBAtomToken<core::StringLiteral("STRING")> m_formatString;
XcbConnection::XCBAtomToken<core::StringLiteral("TEXT")> m_formatText;
XcbConnection::XCBAtomToken<core::StringLiteral("text/plain")> m_formatTextPlain;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you split this class into IClipboardManagerXCB and CClipboardManagerXCB : public IClipboardManagerXCB which would be in src/nbl/ui

I'm about to refactor all this nbl::ui crap so it doesn't leak system headers into includes in include/nbl/ui

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactor is done, merge latest master to yourself.

@devshgraphicsprogramming
Copy link
Member

Btw please decide on XCB vs Xcb in the class names and document that decision.

@devshgraphicsprogramming
Copy link
Member

I'm currently rewriting the future_t and AsyncQueueDispatcher because of deadlock bugs.

This will need to be brought in line with the changes later.

@pollend pollend marked this pull request as ready for review March 18, 2023 17:31
@pollend
Copy link
Contributor Author

pollend commented Mar 18, 2023

Comment on lines 23 to 60
NBL_SYSTEM_DECLARE_DYNAMIC_FUNCTION_CALLER_CLASS(Xcb, system::DefaultFuncPtrLoader,
xcb_destroy_window,
xcb_generate_id,
xcb_create_window,
xcb_connect,
xcb_disconnect,
xcb_map_window,
xcb_get_setup,
xcb_setup_roots_iterator,
xcb_flush,
xcb_intern_atom,
xcb_intern_atom_reply,
xcb_unmap_window,
xcb_get_property,
xcb_get_property_reply,
xcb_get_property_value_length,
xcb_change_property,
xcb_configure_window_checked,
xcb_get_property_value,
xcb_wait_for_event,
xcb_send_event,
xcb_request_check,
xcb_delete_property,
xcb_change_window_attributes,
xcb_warp_pointer,
xcb_query_pointer,
xcb_query_pointer_reply,
xcb_get_selection_owner_reply,
xcb_get_selection_owner
);

NBL_SYSTEM_DECLARE_DYNAMIC_FUNCTION_CALLER_CLASS(XcbIcccm, system::DefaultFuncPtrLoader,
xcb_icccm_set_wm_hints,
xcb_icccm_size_hints_set_size,
xcb_icccm_size_hints_set_min_size,
xcb_icccm_size_hints_set_max_size,
xcb_icccm_set_wm_normal_hints
);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can actually move these inside CWindowManagerXCB to make a nested class, so this stuff isn't freely handing in the namespace

Comment on lines 62 to 81
class CWindowManagerXCB : public IWindowManager
{
public:

virtual bool setWindowSize_impl(IWindow* window, uint32_t width, uint32_t height) override;
virtual bool setWindowPosition_impl(IWindow* window, int32_t x, int32_t y) override;
virtual bool setWindowRotation_impl(IWindow* window, bool landscape) override;
virtual bool setWindowVisible_impl(IWindow* window, bool visible) override;
virtual bool setWindowMaximized_impl(IWindow* window, bool maximized) override;

inline SDisplayInfo getPrimaryDisplayInfo() const override final {
return SDisplayInfo();
}

CWindowManagerXCB();
~CWindowManagerXCB() override = default;

virtual core::smart_refctd_ptr<IWindow> createWindow(IWindow::SCreationParams&& creationParams) override;

virtual void destroyWindow(IWindow* wnd) override final {}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make the class final and remove any virtual keywords from the overrides

Comment on lines 100 to 101
core::smart_refctd_ptr<CWindowManagerXCB> m_windowManager;
xcb_connection_t* m_connection = nullptr;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is an XCB Connection and why is it holding onto the window manager?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need it for xcb and xcbIccm

Comment on lines 29 to 80
virtual bool setWindowSize_impl(uint32_t width, uint32_t height) = 0;
virtual bool setWindowPosition_impl(int32_t x, int32_t y) = 0;
virtual bool setWindowRotation_impl(bool landscape) = 0;
virtual bool setWindowVisible_impl(bool visible) = 0;
virtual bool setWindowMaximized_impl(bool maximized) = 0;

static XCBConnection::MotifWmHints fetchMotifMWHints(IWindow::E_CREATE_FLAGS flags) {
core::bitflag<XCBConnection::MotifFlags> motifFlags(XCBConnection::MWM_HINTS_NONE);
core::bitflag<XCBConnection::MotifFunctions> motifFunctions(XCBConnection::MWM_FUNC_NONE);
core::bitflag<XCBConnection::MotifDecorations> motifDecorations(XCBConnection::MWM_DECOR_NONE);
motifFlags |= XCBConnection::MWM_HINTS_DECORATIONS;

if (flags & IWindow::ECF_BORDERLESS) {
motifDecorations |= XCBConnection::MWM_DECOR_ALL;
} else {
motifDecorations |= XCBConnection::MWM_DECOR_BORDER;
motifDecorations |= XCBConnection::MWM_DECOR_RESIZEH;
motifDecorations |= XCBConnection::MWM_DECOR_TITLE;

// minimize button
if(flags & IWindow::ECF_MINIMIZED) {
motifDecorations |= XCBConnection::MWM_DECOR_MINIMIZE;
motifFunctions |= XCBConnection::MWM_FUNC_MINIMIZE;
}

// maximize button
if(flags & IWindow::ECF_MAXIMIZED) {
motifDecorations |= XCBConnection::MWM_DECOR_MAXIMIZE;
motifFunctions |= XCBConnection::MWM_FUNC_MAXIMIZE;
}

// close button
motifFunctions |= XCBConnection::MWM_FUNC_CLOSE;
}

if(motifFunctions.value != XCBConnection::MWM_FUNC_NONE) {
motifFlags |= XCBConnection::MWM_HINTS_FUNCTIONS;
motifFunctions |= XCBConnection::MWM_FUNC_RESIZE;
motifFunctions |= XCBConnection::MWM_FUNC_MOVE;
} else {
motifFunctions = XCBConnection::MWM_FUNC_ALL;
}

XCBConnection::MotifWmHints hints;
hints.flags = motifFlags.value;
hints.functions = motifFunctions.value;
hints.decorations = motifDecorations.value;
hints.input_mode = 0;
hints.status = 0;
return hints;

}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all this stuff you want to push into CWindowXCB, so that you may remove any XCB and X11 header includes from headers in include/nbl/ui

@@ -143,5 +143,29 @@ namespace nbl::video
return nullptr;
}
}
#elif defined(_NBL_PLATFORM_LINUX_)

#include <xcb/xcb.h>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for this include to work you need to remove the enclosing namespace nbl::video { and do

using namespace nbl;
using namespace nbl::video;

at the top instead

Comment on lines 25 to 27
const void* getNativeHandle() const { return nullptr; }
virtual xcb_window_t getXcbWindow() const = 0;
virtual xcb_connection_t* getXcbConnection() const = 0;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest forward declaring outside the namespace

using xcb_window_t = uint32_t;
struct xcb_connection_t;

and then following our convention which is

struct native_handle_t
{
    xcb_window_t window;
    xcb_connection_t* connection;
};
const native_handle_t& getNativeHandle() const =0;

Comment on lines +1 to +28
#ifndef _NBL_UI_I_CLIPBOARD_MANAGER_XCB_INCLUDED_
#define _NBL_UI_I_CLIPBOARD_MANAGER_XCB_INCLUDED_

#ifdef _NBL_PLATFORM_LINUX_

#include "nbl/ui/IClipboardManager.h"

namespace nbl::ui
{
class XCBConnection;

// details on XCB clipboard protocol: https://tronche.com/gui/x/icccm/sec-2.html#s-2
class NBL_API2 IClipboardManagerXCB : public IClipboardManager
{
public:
IClipboardManagerXCB() : IClipboardManager() {}
virtual ~IClipboardManagerXCB() = default;

virtual std::string getClipboardText() = 0;
virtual bool setClipboardText(const std::string_view& data) = 0;
virtual void process(const IWindowXCB* window, xcb_generic_event_t* event) = 0;
};

}

#endif

#endif

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you probably don't need an IClip in header dir and CClip in source, see if you can go with CClip in source dir right away

Comment on lines 1 to 2
#ifndef C_WINDOW_MANAGER_XCB
#define C_WINDOW_MANAGER_XCB

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you want to start and end your headerguard with _ and include full namespace and the INCLUDED suffix

Comment on lines +1 to +2
#ifndef __NBL_SYSTEM_C_CURSOR_CONTROL_XCB_H_INCLUDED__
#define __NBL_SYSTEM_C_CURSOR_CONTROL_XCB_H_INCLUDED__

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrong namespace, also one too many _ on the beginning and end

Comment on lines 11 to 28
class XCBConnection;
class NBL_API2 CCursorControlXCB final : public ICursorControl
{
public:
inline CCursorControlXCB(
core::smart_refctd_ptr<XCBConnection>&& xcbConnection) :
m_connection(std::move(xcbConnection)) {}

void setVisible(bool visible) override;
bool isVisible() const override;

void setPosition(SPosition pos) override;
void setRelativePosition(IWindow* window, SRelativePosition pos) override;

SPosition getPosition() override;
SRelativePosition getRelativePosition(IWindow* window) override;
private:
core::smart_refctd_ptr<XCBConnection> m_connection;
};
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dunno if its a good idea in your case, but for the Win32 stuff (#476) it ended up making more sense to have the manager inherit both from ICursorControl and IWindowManager because of the global nature of the cursor on Windows.

Don't know if this is applicable here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure how that works in terms of xcb I know x11 supports more then one cursor but applications tends to catch fire if you try to set it up like that ...

Comment on lines 1 to 40
#ifndef _NBL_UI_C_CLIPBOARD_MANAGER_XCB_INCLUDED_
#define _NBL_UI_C_CLIPBOARD_MANAGER_XCB_INCLUDED_

#ifdef _NBL_PLATFORM_LINUX_

#include "nbl/core/decl/Types.h"
#include "nbl/ui/IClipboardManagerXCB.h"
#include "nbl/ui/XCBConnection.h"
namespace nbl::ui
{

class IWindowXCB;
class XCBConnection;

// details on XCB clipboard protocol: https://tronche.com/gui/x/icccm/sec-2.html#s-2
class NBL_API2 CClipboardManagerXCB final : public IClipboardManagerXCB
{
public:
inline CClipboardManagerXCB(core::smart_refctd_ptr<XCBConnection>&& connect):
IClipboardManagerXCB(),
m_connection(std::move(connect)) {}

virtual std::string getClipboardText() override;
virtual bool setClipboardText(const std::string_view& data) override;

void process(const IWindowXCB* window, xcb_generic_event_t* event) override;
private:
core::smart_refctd_ptr<XCBConnection> m_connection;
std::mutex m_clipboardMutex;
std::condition_variable m_clipboardResponseCV;
std::string m_clipboardResponse; // data sent to the clipboard by another application

std::string m_savedClipboard; // data saved to the clipboard for another application to read

XCBConnection::XCBAtomToken<core::StringLiteral("CLIPBOARD")> m_CLIPBOARD;
XCBConnection::XCBAtomToken<core::StringLiteral("TARGETS")> m_TARGETS;
XCBConnection::XCBAtomToken<core::StringLiteral("INCR")> m_INCR;

XCBConnection::XCBAtomToken<core::StringLiteral("UTF8_STRING")> m_formatUTF8_0;
XCBConnection::XCBAtomToken<core::StringLiteral("text/plain;charset=utf-8")> m_formatUTF8_1;
XCBConnection::XCBAtomToken<core::StringLiteral("text/plain;charset=UTF-8")> m_formatUTF8_2;
XCBConnection::XCBAtomToken<core::StringLiteral("GTK_TEXT_BUFFER_CONTENTS")> m_formatGTK;
XCBConnection::XCBAtomToken<core::StringLiteral("STRING")> m_formatString;
XCBConnection::XCBAtomToken<core::StringLiteral("TEXT")> m_formatText;
XCBConnection::XCBAtomToken<core::StringLiteral("text/plain")> m_formatTextPlain;
};

}

#endif

#endif

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this header should probably be in src/nbl/ui don't think anyone outside the engine needs to know about it

Comment on lines +72 to +26
inline SDisplayInfo getPrimaryDisplayInfo() const override final {
return SDisplayInfo();
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO

Comment on lines 16 to 18
#include <xcb/xcb.h>
#include <xcb/xcb_icccm.h>
#include <xcb/xproto.h>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would be decent to follow the Win32 pattern in #476 and create a IWindowManagerXCB as a public header with a single method

static core::smart_refctd_ptr<IWindowManagerXCB> create();

then hide all these implementation details in src

Comment on lines 14 to 18
{
std::unique_lock<std::mutex> lk(m_clipboardMutex);
m_clipboardResponseCV.wait_until(lk, std::chrono::system_clock::now() + std::chrono::seconds(1));
}
std::lock_guard lk(m_clipboardMutex);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what on earth is going on here?


bool CClipboardManagerXCB::setClipboardText(const std::string_view& data) {
std::lock_guard lk(m_clipboardMutex);
m_savedClipboard = data;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

arent you missing some event you should send or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the clipboard is a mess ... I need to rethink this

Comment on lines 31 to 40
void CClipboardManagerXCB::process(const IWindowXCB* window, xcb_generic_event_t* event) {
const auto& xcb = m_connection->getXcbFunctionTable();

auto TARGETS = m_connection->resolveAtom(m_TARGETS);

switch(event->response_type & ~0x80) {
// XCB_ATOM
// Somone is requesting the clipboard data
case XCB_SELECTION_REQUEST: {
auto* sne = reinterpret_cast<xcb_selection_request_event_t*>(event);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like this is a Window event, ergo CWindowXCB should probably inherit from IClipboardManager and you should have no ClipboardManagerXCB

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why didn't I do that originally ...

Comment on lines 100 to 107
void CWindowXCB::CDispatchThread::init()
{

}

void CWindowXCB::CDispatchThread::exit()
{
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw these methods are wholly optional IIRC (if you don't define them, everything will work anyway

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

init isn't but exit is

Comment on lines 158 to 206
auto& xcb = m_windowManager->getXcbFunctionTable();
auto& xcbIccm = m_windowManager->getXcbIcccmFunctionTable();

m_xcbWindow = xcb.pxcb_generate_id(m_connection->getRawConnection());

const auto* primaryScreen = m_connection->primaryScreen();

uint32_t eventMask = XCB_CW_BACK_PIXEL | XCB_CW_EVENT_MASK;
uint32_t valueList[] = {
primaryScreen->black_pixel,
XCB_EVENT_MASK_STRUCTURE_NOTIFY | XCB_EVENT_MASK_KEY_PRESS | XCB_EVENT_MASK_KEY_RELEASE |
XCB_EVENT_MASK_FOCUS_CHANGE | XCB_EVENT_MASK_PROPERTY_CHANGE
};

xcb_void_cookie_t xcbCheckResult = xcb.pxcb_create_window(
m_connection->getRawConnection(), XCB_COPY_FROM_PARENT, m_xcbWindow, primaryScreen->root,
static_cast<int16_t>(m_x),
static_cast<int16_t>(m_y),
static_cast<int16_t>(m_width),
static_cast<int16_t>(m_height), 4,
XCB_WINDOW_CLASS_INPUT_OUTPUT, primaryScreen->root_visual, eventMask,
valueList);

setWindowSize_impl(m_width, m_height);

auto WM_DELETE_WINDOW = m_connection->resolveAtom(m_WM_DELETE_WINDOW);
auto NET_WM_PING = m_connection->resolveAtom(m_NET_WM_PING);
auto WM_PROTOCOLS = m_connection->resolveAtom(m_WM_PROTOCOLS);

const std::array atoms {WM_DELETE_WINDOW, NET_WM_PING};
xcb.pxcb_change_property(
m_connection->getRawConnection(),
XCB_PROP_MODE_REPLACE,
m_xcbWindow,
WM_PROTOCOLS, XCB_ATOM_ATOM, 32, atoms.size(), atoms.data());


auto motifHints = fetchMotifMWHints(getFlags().value);
m_connection->setMotifWmHints(m_xcbWindow, motifHints);

if(isAlwaysOnTop()) {
XCBConnection::XCBAtomToken<core::StringLiteral("NET_WM_STATE_ABOVE")> NET_WM_STATE_ABOVE;
m_connection->setNetMWState(
primaryScreen->root,
m_xcbWindow, false, m_connection->resolveAtom(NET_WM_STATE_ABOVE));
}

xcb.pxcb_map_window(m_connection->getRawConnection(), m_xcbWindow);
xcb.pxcb_flush(m_connection->getRawConnection());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can any of this fail?

if yes, can you do all this inside the CWindowManagerXCB::createWindow and pass xcbWindow and any other variables we need to remembers straight to the constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

umm I'll have to explore that

@devshgraphicsprogrammingjenkins
Copy link
Contributor

[CI]: Can one of the admins verify this patch?

Signed-off-by: Michael Pollind <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants