feature: implement xcb for windowing #462

pollend · 2023-02-21T03:41:58Z

Description

WIP support that replaces x11 with xcb

src/nbl/CMakeLists.txt

devshgraphicsprogramming · 2023-03-01T08:41:08Z

include/nbl/ui/CClipboardManagerXcb.h

+		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;


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

Refactor is done, merge latest master to yourself.

devshgraphicsprogramming · 2023-03-01T08:42:06Z

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

devshgraphicsprogramming · 2023-03-10T08:09:28Z

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 · 2023-03-18T17:31:59Z

ref: Devsh-Graphics-Programming/Nabla-Examples-and-Tests#32

devshgraphicsprogramming · 2023-03-18T20:15:38Z

include/nbl/ui/CWindowManagerXCB.h

+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
+);


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

devshgraphicsprogramming · 2023-03-18T20:16:08Z

include/nbl/ui/CWindowManagerXCB.h

+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 {}


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

devshgraphicsprogramming · 2023-03-18T20:17:33Z

include/nbl/ui/XCBConnection.h

+    core::smart_refctd_ptr<CWindowManagerXCB> m_windowManager;
+    xcb_connection_t* m_connection = nullptr;


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

I need it for xcb and xcbIccm

devshgraphicsprogramming · 2023-03-18T20:19:42Z

include/nbl/ui/IWindowXCB.h

+        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;
+
+        }


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

devshgraphicsprogramming · 2023-03-18T20:22:00Z

src/nbl/video/CSurfaceVulkan.cpp

@@ -143,5 +143,29 @@ namespace nbl::video
 			return nullptr;
 		}
 	}
+#elif defined(_NBL_PLATFORM_LINUX_)
+
+	#include <xcb/xcb.h>


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

devshgraphicsprogramming · 2023-03-18T20:28:02Z

include/nbl/ui/IWindowXCB.h

+        const void* getNativeHandle() const { return nullptr; }
+        virtual xcb_window_t getXcbWindow() const = 0;
+        virtual xcb_connection_t* getXcbConnection() const = 0;


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;

devshgraphicsprogramming · 2023-03-18T20:28:59Z

include/nbl/ui/IClipboardManagerXCB.h

+#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


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

devshgraphicsprogramming · 2023-03-18T20:29:41Z

include/nbl/ui/CWindowManagerXCB.h

+#ifndef C_WINDOW_MANAGER_XCB
+#define C_WINDOW_MANAGER_XCB


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

devshgraphicsprogramming · 2023-03-18T20:29:57Z

include/nbl/ui/CCursorControlXCB.h

+#ifndef __NBL_SYSTEM_C_CURSOR_CONTROL_XCB_H_INCLUDED__
+#define __NBL_SYSTEM_C_CURSOR_CONTROL_XCB_H_INCLUDED__


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

devshgraphicsprogramming · 2023-03-18T20:32:07Z

include/nbl/ui/CCursorControlXCB.h

+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;
+};
+}


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

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 ...

devshgraphicsprogramming · 2023-03-18T20:33:04Z

include/nbl/ui/CClipboardManagerXCB.h

+#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


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

devshgraphicsprogramming · 2023-03-18T20:33:24Z

include/nbl/ui/CWindowManagerXCB.h

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


devshgraphicsprogramming · 2023-03-18T20:34:59Z

include/nbl/ui/CWindowManagerXCB.h

+#include <xcb/xcb.h>
+#include <xcb/xcb_icccm.h>
+#include <xcb/xproto.h>


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

include/nbl/ui/CWindowXCB.h

devshgraphicsprogramming · 2023-03-18T20:37:54Z

src/nbl/ui/CClipboardManagerXCB.cpp

+        {
+            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);


what on earth is going on here?

devshgraphicsprogramming · 2023-03-18T20:38:43Z

src/nbl/ui/CClipboardManagerXCB.cpp

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


arent you missing some event you should send or something?

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

devshgraphicsprogramming · 2023-03-18T20:41:11Z

src/nbl/ui/CClipboardManagerXCB.cpp

+    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);


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

why didn't I do that originally ...

devshgraphicsprogramming · 2023-03-18T20:42:36Z

src/nbl/ui/CWindowXCB.cpp

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


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

init isn't but exit is

src/nbl/ui/CWindowXCB.cpp

devshgraphicsprogramming · 2023-03-18T20:46:33Z

src/nbl/ui/CWindowXCB.cpp

+    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());


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?

umm I'll have to explore that

src/nbl/ui/CWindowXCB.cpp

devshgraphicsprogrammingjenkins · 2023-04-29T12:19:13Z

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

Signed-off-by: Michael Pollind <mpollind@gmail.com>

devshgraphicsprogramming reviewed Feb 21, 2023

View reviewed changes

src/nbl/CMakeLists.txt Outdated Show resolved Hide resolved

pollend force-pushed the feature/add-support-xcb branch 5 times, most recently from e655449 to 46c0eb0 Compare March 1, 2023 05:20

devshgraphicsprogramming reviewed Mar 1, 2023

View reviewed changes

pollend marked this pull request as ready for review March 18, 2023 17:31