Win32 UI with tabs and buttons

Question

I am making a UI library for the fun of it, and decided to use move semantics instead of pointers with new/delete.

Everything is working, but I am not satisfied.

Given this code:

void ControlGroup::AddControl(Control&& control) noexcept
{
    if(control.tabstop)
        control.taborder = GetMaxTabOrder() + 1;

    control.SetParent(*parent_control);

    HWND hwnd = control.hwnd;
    control_map.emplace(control.hwnd, std::move(control));

    Control* c = &control_map.at(hwnd);
    tab_map.emplace(c->taborder, c);
}

I would like advice for a better technique to add Control* c inside tab_map.

Before going further - Are move semantics really worth the trouble?

The complete related code follows:

Control.hpp

#pragma once
#include "../class_macro.hpp"
#include "Window.hpp"
#include <cassert>

class ControlGroup;

struct UserControlCreateInfo {
    SIZE client_size{};
    int control_id{};
    Window::Layout layout{};
    POINT position{};
    int taborder{};
    bool tabstop{true};
    LPCTSTR text{};
    SIZE window_size{};
};

class Control : public Window {
    ControlGroup* control_group;

    friend ControlGroup;

protected:
    int control_id;
    int taborder;
    bool tabstop;

public:
    ENABLE_MOVE_CONSTRUCTOR(Control)

    struct ControlCreateInfo {
        LPCTSTR class_name{};
        SIZE client_size{};
        int control_id{};
        DWORD ex_style{};
        HWND hwnd_parent{};
        Layout layout{};
        POINT position{};
        DWORD style{};
        int taborder{};
        bool tabstop{true};
        LPCTSTR text{};
        SIZE window_size{};
    };

    Control(const ControlCreateInfo& cci) noexcept;

protected:
    Control(const UserControlCreateInfo& cci, LPCTSTR class_name, DWORD style = {}, DWORD ex_style = {}) noexcept;

public:
    virtual ~Control() noexcept;

    virtual void SetFocus() noexcept;
    void AddChild(Control&& control) noexcept;

    virtual bool BeforeKeyDown(HWND hwnd, WPARAM wparam) noexcept override;

private:
    void SetParent(const Window& parent_window) noexcept;
};

Control.cpp

#include "Control.hpp"
#include "ControlGroup.hpp"

Control::Control(Control&& other) noexcept :
    Window{std::move(other)},
    control_group{other.control_group},
    control_id{other.control_id},
    taborder{other.taborder},
    tabstop{other.tabstop}
{
    other.control_group = nullptr;
}

Control::Control(const ControlCreateInfo& cci) noexcept :
    Window{{
        .class_name = cci.class_name,
        .client_size = cci.client_size,
        .ex_style = cci.ex_style,
        .hwnd_parent = cci.hwnd_parent,
        .layout = cci.layout,
        .position = cci.position,
        .style = cci.style,
        .text = cci.text,
        .window_size = cci.window_size,
    }},
    control_group{new ControlGroup(this)},
    control_id{cci.control_id},
    taborder{cci.taborder},
    tabstop{cci.tabstop}
{}

Control::Control(const UserControlCreateInfo& cci, LPCTSTR class_name, DWORD style, DWORD ex_style) noexcept :
    Window{{
        .class_name = class_name,
        .client_size = cci.client_size,
        .ex_style = ex_style,
        .layout = cci.layout,
        .position = cci.position,
        .style = style,
        .text = cci.text,
        .window_size = cci.window_size,
    }},
    control_group{new ControlGroup(this)},
    control_id{cci.control_id},
    taborder{cci.taborder},
    tabstop{cci.tabstop}
{}

Control::~Control() noexcept
{
    if(control_group) {
        delete control_group;
        control_group = nullptr;
    }
}

void Control::SetFocus() noexcept
{
    ::SetFocus(hwnd);
}

void Control::AddChild(Control&& control) noexcept
{
    control_group->AddControl(std::move(control));
}

bool Control::BeforeKeyDown(HWND hwnd, WPARAM wparam) noexcept
{
    switch(wparam) {
        case VK_TAB: {
            bool cycle_up = GetAsyncKeyState(VK_SHIFT);
            control_group->CycleTab(cycle_up);
            return true;
        }
        case VK_ESCAPE:
            Destroy();
            return true;
    }

    return Window::BeforeKeyDown(hwnd, wparam);
}

void Control::SetParent(const Window& parent_window) noexcept
{
    SetWindowLongPtr(hwnd, GWL_STYLE, style | WS_CHILD);
    ::SetParent(hwnd, parent_window.GetHandle());
    SetWindowPos(hwnd, 0, 0, 0, window_size.cx, window_size.cy, SWP_NOZORDER | SWP_NOMOVE);
    Show();
}

ControlGroup.hpp

#pragma once
#include "Control.hpp"
#include <map>

class ControlGroup {
    const Control* parent_control;
    std::map<HWND, Control> control_map;
    std::multimap<int, Control*> tab_map;

public:
    DISABLE_COPY_AND_MOVE(ControlGroup)

    ControlGroup(const Control* parent_control) noexcept;
    ~ControlGroup() noexcept;

    void AddControl(Control&& control) noexcept;
    void CycleTab(bool cycle_up) noexcept;

private:
    int GetMaxTabOrder() const noexcept;
    Control* FindControlByHandle(HWND control) noexcept;
    Control* FindNextControlInTabOrder(Control* control, bool cycle_up) noexcept;
    void SetFocusToControl(Control* control) noexcept;
    void SetFocusToFirstControl() noexcept;
};

ControlGroup.cpp

#include "ControlGroup.hpp"

ControlGroup::ControlGroup(const Control* parent_control) noexcept :
    parent_control{parent_control} {}

ControlGroup::~ControlGroup() noexcept
{
    parent_control = nullptr;
}

void ControlGroup::AddControl(Control&& control) noexcept
{
    if(control.tabstop)
        control.taborder = GetMaxTabOrder() + 1;

    control.SetParent(*parent_control);

    HWND hwnd = control.hwnd;
    control_map.emplace(control.hwnd, std::move(control));

    Control* c = &control_map.at(hwnd);
    tab_map.emplace(c->taborder, c);
}

void ControlGroup::CycleTab(bool cycle_up) noexcept
{
    HWND focus = GetFocus();
    if(!focus) {
        SetFocusToFirstControl();
        return;
    }

    auto control = FindControlByHandle(focus);
    if(!control) {
        SetFocusToFirstControl();
        return;
    }

    control = FindNextControlInTabOrder(control, cycle_up);
    if(control)
        control->SetFocus();
}

int ControlGroup::GetMaxTabOrder() const noexcept
{
    auto it = tab_map.crbegin();
    if(it != tab_map.crend())
        return it->first;
    return 0;
}

Control* ControlGroup::FindControlByHandle(HWND control) noexcept
{
    auto it = control_map.find(control);
    if(it == control_map.end())
        return nullptr;

    return &it->second;
}

Control* ControlGroup::FindNextControlInTabOrder(Control* control, bool cycle_up) noexcept
{
    if(tab_map.size() == 1)
        return control;

    auto current = tab_map.find(control->taborder);
    if(current == tab_map.end())
        return nullptr;

    if(cycle_up) {
        if(current == tab_map.begin())
            return tab_map.rbegin()->second;

        return std::prev(current)->second;
    }

    auto next = std::next(current);
    if(next == tab_map.end())
        return tab_map.begin()->second;

    return next->second;
}

void ControlGroup::SetFocusToControl(Control* control) noexcept
{
    control->SetFocus();
}

void ControlGroup::SetFocusToFirstControl() noexcept
{
    const auto it = tab_map.cbegin();
    if(it != tab_map.cend())
        SetFocusToControl(it->second);
}

Window.hpp

#pragma once
#define WIN32_LEAN_AND_MEAN
#include <Windows.h>
#include "../class_macro.hpp"
#include "../tstring.hpp"

class Window {
protected:
    HWND hwnd;
    SIZE window_size;
    DWORD style;
    bool active;

public:
    DISABLE_COPY(Window)
    ENABLE_MOVE_CONSTRUCTOR(Window)

    static const DWORD DEFAULT_STYLE;

    enum class Layout {
        Custom,
        CenterParent,
        FillParent
    };

    struct WindowCreateInfo {
        LPCTSTR class_name{};
        SIZE client_size{};
        DWORD ex_style{};
        HMENU hwnd_menu{};
        HWND hwnd_parent{};
        Layout layout{};
        POINT position{};
        DWORD style{};
        LPCTSTR text{};
        SIZE window_size{};
    };

    Window(const WindowCreateInfo& wci) noexcept;
    virtual ~Window() noexcept;

    virtual void Show() noexcept;
    virtual void Hide() noexcept;
    virtual bool MessageUpdate() noexcept;
    virtual bool MessageLoop() noexcept;
    virtual void Destroy() noexcept;

    virtual bool OnMouseClick(WPARAM wparam, int x, int y) noexcept;
    virtual bool BeforeKeyDown(HWND hwnd, WPARAM wparam) noexcept;
    virtual bool OnKeyDown(WPARAM wparam) noexcept;

    [[nodiscard]] bool IsDestroyed() const noexcept { return !active; }
    [[nodiscard]] virtual HWND GetHandle() const noexcept { return hwnd; }
    [[nodiscard]] virtual SIZE GetWindowSize() const noexcept;
    [[nodiscard]] virtual SIZE GetClientSize() const noexcept;
    [[nodiscard]] virtual tstring GetText() const noexcept;

protected:
    virtual LRESULT WndProc(UINT msg, WPARAM wparam, LPARAM lparam) noexcept;

private:
    static LRESULT CALLBACK WndProcStatic(HWND hwnd, UINT msg, WPARAM wparam, LPARAM lparam) noexcept;
    void PrepareWndClass(HINSTANCE hinstance, LPCTSTR class_name) const noexcept;
    void ProcessMessage(const MSG& msg) noexcept;
};

inline SIZE RectToSize(const RECT& rect) noexcept;

Window.cpp

#include "Window.hpp"
#include <cassert>

constexpr LONG DEFAULT_WIDTH = 384;
constexpr SIZE GetDefaultSize(const int size) { return {size, LONG(size / (16 / 9.0))}; }

static LPCTSTR DEFAULT_CLASS_NAME = TEXT("ShigotoShoujinWndClass");
const DWORD Window::DEFAULT_STYLE = WS_SYSMENU | WS_CAPTION | WS_MINIMIZEBOX;

static SIZE GetParentSize(HWND hwnd_parent) noexcept;
static SIZE AdjustWindowSize(SIZE client_size, DWORD style, DWORD ex_style) noexcept;
static POINT CenterWindow(SIZE parent_size, SIZE window_size) noexcept;

Window::Window(Window&& other) noexcept :
    hwnd{other.hwnd},
    window_size{other.window_size},
    style{other.style},
    active{other.active}
{
    other.hwnd = {};
    other.active = false;
}

Window::Window(const WindowCreateInfo& wci) noexcept
{
    HINSTANCE hinstance = GetModuleHandle(NULL);
    POINT position = wci.position;
    LPCTSTR class_name = wci.class_name ? wci.class_name : DEFAULT_CLASS_NAME;

    style = wci.style;
    window_size = wci.window_size;

    bool is_child = wci.style & WS_CHILD;
    assert((is_child && wci.hwnd_parent) || (!is_child && !wci.hwnd_parent));

    if(!style && !wci.class_name)
        style = DEFAULT_STYLE;

    if(wci.hwnd_parent != HWND_DESKTOP)
        style |= WS_CHILD;

    PrepareWndClass(hinstance, class_name);

    switch(wci.layout) {
        case Layout::Custom:
        case Layout::CenterParent:
            if(wci.client_size.cx && wci.client_size.cy)
                window_size = AdjustWindowSize(wci.client_size, style, wci.ex_style);
            else if(!window_size.cx || !window_size.cy)
                window_size = GetDefaultSize(DEFAULT_WIDTH);

            if(wci.layout == Layout::CenterParent)
                position = CenterWindow(GetParentSize(wci.hwnd_parent), window_size);

            break;
        case Layout::FillParent:
            if(!wci.style && wci.hwnd_parent == HWND_DESKTOP)
                style = WS_POPUP;
            position = {};
            window_size = GetParentSize(wci.hwnd_parent);
    }

    hwnd = CreateWindowEx(wci.ex_style, class_name, wci.text, style, position.x, position.y, window_size.cx, window_size.cy, wci.hwnd_parent, wci.hwnd_menu, hinstance, NULL);
    assert(hwnd);

    active = true;
    SetWindowLongPtr(hwnd, GWLP_USERDATA, (LONG_PTR)(this));
}

Window::~Window() noexcept
{
    Destroy();
}

void Window::Show() noexcept
{
    assert(active);
    ShowWindow(hwnd, SW_SHOW);
}

void Window::Hide() noexcept
{
    assert(active);
    ShowWindow(hwnd, SW_HIDE);
}

bool Window::MessageUpdate() noexcept
{
    assert(active);

    MSG msg;

    while(active && PeekMessage(&msg, hwnd, 0, 0, PM_REMOVE))
        ProcessMessage(msg);

    return active;
}

bool Window::MessageLoop() noexcept
{
    assert(active);

    MSG msg;

    while(active && GetMessage(&msg, hwnd, 0, 0))
        ProcessMessage(msg);

    return active;
}

void Window::Destroy() noexcept
{
    if(hwnd && active) {
        active = false;
        DestroyWindow(hwnd);
        hwnd = 0;
    }
}

bool Window::OnMouseClick(WPARAM wparam, int x, int y) noexcept
{
    return false;
}

bool Window::BeforeKeyDown(HWND hwnd, WPARAM wparam) noexcept
{
    return false;
}

bool Window::OnKeyDown(WPARAM wparam) noexcept
{
    return false;
}

SIZE Window::GetWindowSize() const noexcept
{
    assert(active);

    RECT rect;
    GetWindowRect(hwnd, &rect);
    return RectToSize(rect);
}

SIZE Window::GetClientSize() const noexcept
{
    assert(active);

    RECT rect;
    GetClientRect(hwnd, &rect);
    return RectToSize(rect);
}

tstring Window::GetText() const noexcept
{
    assert(active);

    size_t max_count = SendMessage(hwnd, WM_GETTEXTLENGTH, 0, 0);
    TCHAR* buffer = new TCHAR[max_count + 1];
    SendMessage(hwnd, WM_GETTEXT, max_count + 1, (LPARAM)buffer);

    tstring text(buffer);
    delete[] buffer;

    return text;
}

LRESULT Window::WndProc(UINT msg, WPARAM wparam, LPARAM lparam) noexcept
{
    switch(msg) {
        case WM_LBUTTONDOWN:
            if(OnMouseClick(wparam, LOWORD(lparam), HIWORD(lparam)))
                return 0;
            break;
        case WM_KEYDOWN:
            if(OnKeyDown(wparam))
                return 0;
            break;
        case WM_DESTROY:
            Destroy();
            return 0;
    }

    return DefWindowProc(hwnd, msg, wparam, lparam);
}

LRESULT CALLBACK Window::WndProcStatic(HWND hwnd, UINT msg, WPARAM wparam, LPARAM lparam) noexcept
{
    Window* self;

    if(msg != WM_NCCREATE && (self = (Window*)(GetWindowLongPtr(hwnd, GWLP_USERDATA))))
        return self->WndProc(msg, wparam, lparam);

    return DefWindowProc(hwnd, msg, wparam, lparam);
}

void Window::PrepareWndClass(HINSTANCE hinstance, LPCTSTR class_name) const noexcept
{
    WNDCLASSEX wc;

    if(!GetClassInfoEx(hinstance, class_name, &wc)) {
        wc.cbSize = sizeof(wc);
        wc.style = CS_OWNDC;
        wc.lpfnWndProc = Window::WndProcStatic;
        wc.cbClsExtra = 0;
        wc.cbWndExtra = 0;
        wc.hInstance = hinstance;
        wc.hIcon = LoadIcon(NULL, IDI_APPLICATION);
        wc.hCursor = LoadCursor(NULL, IDC_ARROW);
        wc.hbrBackground = (HBRUSH)(COLOR_BTNFACE + 1);
        wc.lpszMenuName = NULL;
        wc.lpszClassName = class_name;
        wc.hIconSm = NULL;

        RegisterClassEx(&wc);
    }
}

void Window::ProcessMessage(const MSG& msg) noexcept
{
    TranslateMessage(&msg);

    if(msg.message == WM_KEYDOWN)
        if(BeforeKeyDown(msg.hwnd, msg.wParam))
            return;

    DispatchMessage(&msg);
}

inline SIZE RectToSize(const RECT& rect) noexcept
{
    return {rect.right - rect.left, rect.bottom - rect.top};
}

static SIZE GetParentSize(HWND hwnd_parent) noexcept
{
    if(hwnd_parent != HWND_DESKTOP) {
        RECT rect;
        GetClientRect(hwnd_parent, &rect);
        return RectToSize(rect);
    } else
        return {GetSystemMetrics(SM_CXSCREEN), GetSystemMetrics(SM_CYSCREEN)};
}

static SIZE AdjustWindowSize(SIZE client_size, DWORD style, DWORD ex_style) noexcept
{
    RECT rect{0, 0, client_size.cx, client_size.cy};
    AdjustWindowRectEx(&rect, style, 0, ex_style);
    return RectToSize(rect);
}

static POINT CenterWindow(SIZE parent_size, SIZE window_size) noexcept
{
    return {
        (parent_size.cx - window_size.cx) >> 1,
        (parent_size.cy - window_size.cy) >> 1};
}

Also available in this repo, but everything needed is shown above.

Answer 1

WndProc setup

It's been a while since I've done Windows API stuff, so my memory's kinda fuzzy, but I think there's an issue with how the window procedure is set up in the Window class:

CreateWindowEx only returns after various calls have already been made to the window procedure. Until it returns, hwnd isn't set properly, and this isn't passed to the userdata with SetWindowLongPtr. So in the meantime we might pass a null hwnd to DefWindowProc, and we won't call our own WndProc function until later than we should.

I think the correct way to solve the issue is to pass this as the lParam in CreateWindowEx. We can retrieve it in the window proc for the WM_NCCREATE message, and use it in SetWindowLongPtr. At the same time, we set the hwnd member to the correct value.

See here and here for more details and the "correct" way to do things.

---

Simplify settings structs

There's a lot of shared data between WindowCreateInfo, ControlCreateInfo and UserControlCreateInfo. Perhaps we can split these variables into groups to avoid duplication?

ControlCreateInfo adds control_id, taborder and tabstop and removes hwnd_menu. UserControlCreateInfo removes hwnd_parent, class_name, style and ex_style, but they're passed to the constructor as separate parameters anyway.

So perhaps we could use WindowCreateInfo throughout the codebase, and add a second parameter for Controls:

struct ControlCreateInfo {
    int control_id{};
    int taborder{};
    bool tabstop{true};
};

...

Control(WindowCreateInfo const& wci, ControlCreateInfo const& cci) { ... }

I'm not sure we need a separate protected constructor for Control.

---

Use plain C++ instead of macros

I admit that I used to use similar macros for move and copy when it was first introduced, but quickly decided that was more trouble than just typing the declarations.

For example, I spent a while thinking there was a bug with how the control_group was handled, because I assumed that ENABLE_MOVE_CONSTRUCTOR would = default the move constructor. Unexpectedly, it's actually just a declaration, and we have the full definition in the .cpp file.

I think it's much clearer to write out the full declaration in the class though: Control(Control&& other) noexcept; so people like me don't get confused. It's about the same amount of typing anyway.

---

Avoid manual memory management

We should use std::unique_ptr<ControlGroup> so that we don't have to manually delete it, and we can also use the = default move constructor.

---

Control and ControlGroup

The ControlGroup stores Controls by value in the control_map, and Control*s in the tab_map. This is safe, because the ControlGroup can ensure that we don't add or remove a Control without changing the tab_map.

However, that's not the case for the parent_control. Moving the owning Control will change its location in memory, but the ControlGroup's parent_control pointer won't be updated.

It might be reasonable to decide that a Control "Is A" ControlGroup (every Control allocates its own after all), and merge the two classes together. I think it's safe to allow ControlGroup to be moved, since moving a std::map won't invalidate pointers.

We can keep the functionality separate by having Control inherit ControlGroup:

class Control;

class ControlGroup
{
protected:
    ControlGroup();

    Control* GetOwner();
};

// note: function must be defined in the .cpp file so we have complete knowledge of Control
Control* ControlGroup::GetParent() { return static_cast<Control*>(this); }

class Control : public ControlGroup, public Window { ... };

Since our *this pointer is always up-to-date, we can get the Control, even if it (and the ControlGroup) is moved.

As a side-note, we should be able to implement move-assignment for the Control class as well as move-construction.

---

Control insertion

HWND hwnd = control.hwnd;
control_map.emplace(control.hwnd, std::move(control));

Control* c = &control_map.at(hwnd);
tab_map.emplace(c->taborder, c);

From the linked repo, it looks like you've already found that emplace returns an iterator. :)

It's probably worth an assertion or some other check to ensure that adding the control succeeded. (And maybe we should do the insertion before any of the other operations here).

---

Move semantics and pointers

I think the main issue with using move-semantics here is (as seen above), keeping references (or pointers), up to date. The ControlGroup class can ensure that it only uses pointers while a Control exists safely on it's control_map. Any outside use of Control pointers is likely to be unsafe, so we have to use hwnds (or some other id) everywhere else.

If you're happy with this restriction then that's fine. If not, then storing Controls in unique_ptrs might be easier. You still have a single point of ownership, but pointers are valid for the entire lifetime of the Control instance.

---

Move assignment

[In response to comments:] The copy and swap idiom is a good way to implement the move-assignment operator. It looks like this for a class T:

T& T::operator=(T&& other)
{
    T temp (std::move(other)); // use the move constructor to copy from other

    using std::swap;
    swap(m_var_1, temp.m_var_1); // swap each member variable between this and temp
    swap(m_var_2, temp.m_var_2);

    return *this;
}

We could provide a void swap(T& other) member function on our class to encapsulate all the swapping if we have many variables, and just call swap(temp).

If we wanted to, we could unify it with the copy constructor by providing a single operator= like so.

T& T::operator=(T other)
{
    swap(temp); // use the swap member function we defined
    return *this;
}

This is safe for self-assignment, so we don't need to check for it. (Although we could check for self-assignment and avoid the swapping, note that self-assignment is very rare. By doing the check we make the "normal" (non-self-assignment) case slower! So we just don't check).