Tab container/PanelHolder/Notebook widget

Panel holder is a control, which manages multiple windows with associated tabs. This control also known either as Tab container or Notebook widget. It's popular widget so it would be good to have such control in TGUI.

I've made such simple implementation that I current use in own project:

class tgPanelHolder : public tgui::SubwidgetContainer
{
public:

    typedef std::shared_ptr<tgPanelHolder> Ptr; //!< Shared widget pointer
    typedef std::shared_ptr<const tgPanelHolder> ConstPtr; //!< Shared constant widget pointer

    tgPanelHolder(const tgui::Layout2d& size = { "100%", "100%" }) : m_index(-1)
    {
        m_type = "PanelHolder";
        setSize(size);
        m_tabs = tgui::Tabs::create();
        m_tabs->setSize({ size.x, m_tabs->getSize().y * 2 });
        m_tabs->onTabSelect([this]()
            {
                auto cur = m_tabs->getSelectedIndex();
                Select(cur);
                if (m_tabs->getSelectedIndex() != m_index)
                {
                    m_tabs->select(m_index);
                }
            });

        m_container->add(m_tabs, "Tabs");
    }

    static tgPanelHolder::Ptr create(const tgui::Layout2d& size = { "100%", "100%" })
    {
        return std::make_shared<tgPanelHolder>(size);
    }

    static tgPanelHolder::Ptr copy(tgPanelHolder::ConstPtr pholder)
    {
        if (pholder)
            return std::static_pointer_cast<tgPanelHolder>(pholder->clone());
        else
            return nullptr;
    }

    Widget::Ptr clone() const override
    {
        return std::make_shared<tgPanelHolder>(*this);
    }

    void addPanel(tgui::Panel::Ptr ptr, const tgui::String& name, bool select = true)
    {
        auto size = getSizeLayout();
        ptr->setSize({ size.x , size.y - m_tabs->getSize().y });
        ptr->setPosition({ tgui::bindLeft(m_tabs), tgui::bindBottom(m_tabs) });
        m_panels.push_back(ptr);
        m_tabs->add(name, select);
        if (select)
        {
            Select(m_panels.size() - 1, false);
        }
    }

    bool addPanelAt(tgui::Panel::Ptr ptr, const tgui::String& name, std::size_t index, bool select = true)
    {
        if (index > m_panels.size())
        {
            return false;
        }

        addPanel(ptr, name, select);
        auto size = m_panels.size();
        if (index != size)
        {
            std::swap(m_panels[index], m_panels[size - 1]);
        }
    }

    void RemovePanel(tgui::Panel::Ptr ptr)
    {
        if (ptr != nullptr)
        {
            auto idx = getIndex(ptr);
            if (idx != -1)
            {
                m_tabs->remove(idx);
                m_container->remove(m_panels[idx]);
                m_panels.erase(m_panels.begin() + idx);
                if (idx == 0)
                {
                    Select(m_panels.size() - 1);
                }
                else
                {
                    Select(idx - 1);
                }
            }
        }
    }

    void Select(std::size_t index, bool genEvents = true)
    {
        if (index >= m_panels.size() || index == static_cast<std::size_t>(m_index))
        {
            return;
        }
        if (genEvents)
        {
            bool isVetoed = false;
            onSelectionChanging.emit(this, index, &isVetoed);
            if (isVetoed)
            {
                return;
            }
        }

        if (m_container->getWidgets().size() == 2)
        {
            m_container->remove(m_panels[m_index]);
        }
        m_container->add(m_panels[index]);
        m_tabs->select(index);
        m_index = index;
        if (genEvents)
        {
            onSelectionChanged.emit(this, m_index);
        }
    }

    std::size_t Count() const
    {
        return m_panels.size();
    }

    int getIndex(tgui::Panel::Ptr ptr)
    {
        for (std::size_t i = 0; i < m_panels.size(); i++)
        {
            if (m_panels[i] == ptr)
            {
                return static_cast<int>(i);
            }
        }
        return -1;
    }

    tgui::Panel::Ptr getSelected()
    {
        return getPanel(m_index);
    }

    int getSelectedIndex() const
    {
        return m_index;
    }

    tgui::Panel::Ptr getPanel(int index)
    {
        if (index < 0 || index >= static_cast<int>(m_panels.size()))
        {
            return nullptr;
        }
        return m_panels[index];
    }

    tgui::Tabs::Ptr getTabs()
    {
        return m_tabs;
    }

    tgui::String getTabText(std::size_t index) const
    {
        return m_tabs->getText(index);
    }

    bool changeTabText(std::size_t index, const tgui::String& text)
    {
        return m_tabs->changeText(index, text);
    }

public:
    tgui::SignalInt onSelectionChanged = { "SelectionChanged" };
    tgui::SignalTyped2<int, bool*> onSelectionChanging = { "SelectionChanging" }; //can be vetoed

private:
    std::vector<tgui::Panel::Ptr> m_panels;
    int m_index;

    tgui::Tabs::Ptr m_tabs;
};

If it's OK - I'm going to open PR with proper changes (code style, tests, ...)

I would call it TabContainer.
- The name PanelHolder doesn't indicate that there are tabs.
- Notebook widget is hopeless to search for due to Jupyter Notebook.
- I didn't have any better ideas myself, TabbedPanel doesn't sound good.
Googling for TabContainer does provide some results on what it is, so even if people don't immediately know what it does from the name they will easily be able to find a description.

The code mostly looks fine. A few remarks:
- Why double the height of the tab in the constructor?
- addPanelAt should be called insertPanel for consistency with other widgets
- Is "m_tabs->select(m_index);" in "m_tabs->onTabSelect" in tgPanelHolder constructor needed? Wouldn't it already be selected before this callback function is even called (and changed a second time in the Select function)?
- Perhaps you should only call "m_tabs->select(index);" in the Select function when the index in the tab is different (i.e. when the user called select himself). Right now you have something that looks like a recursion: Select calls m_tabs->select which triggers onTabSelect in which you call Select again. This works because the Tabs widget doesn't send another callback when the index didn't change, but it might be better to not rely on such internal behavior of Tabs (even if the behavior is never going to change).
- The documentation (comment above class) should probably mention that if you want just the tabs without the panels then the Tabs widget can be used. Similarly, the documentation for the existing Tabs widget should get an extra note that it doesn't contain any panels but if people want a panel directly below their tabs then they can use the TabContainer widget.

The code mostly looks fine. A few remarks:
- Why double the height of the tab in the constructor?

For me default height in Tabs is a bit narrow, in PR I changed to default one, but I think it would be good to provide method for changing height of Tabs in some form.
In code above I just use getTabs function, but I removed it in PR because I don't see any sense to provide full access to private member in library. Or am I wrong and such function is OK?

- Is "m_tabs->select(m_index);" in "m_tabs->onTabSelect" in tgPanelHolder constructor needed? Wouldn't it already be selected before this callback function is even called (and changed a second time in the Select function)?

Yes, it's needed for case if user forbid selection changing in onSelectionChanging handler, i.e:

            tabContainer->onSelectionChanging([&tabContainerelectedCount](int idx, bool* Vetoed)
            {
                if (someCondition)
                {
                    *Vetoed = true;
                }
            });

- Perhaps you should only call "m_tabs->select(index);" in the Select function when the index in the tab is different (i.e. when the user called select himself). Right now you have something that looks like a recursion: Select calls m_tabs->select which triggers onTabSelect in which you call Select again.

It's indeed was reason of one extra call Select function. Index should be changed first, fixed.

I think it would be good to provide method for changing height of Tabs in some form

I agree, there should be some way to customize the height. A setTabsHeight function could be added.

In code above I just use getTabs function, but I removed it in PR because I don't see any sense to provide full access to private member in library. Or am I wrong and such function is OK?

I'm ok with having some internal classes exposed. Ideally, all functionality should be in the outer class so that nobody would ever need to access the internal classes directly, but in cases like this, having access to the Tabs class would allow people to do some extra things that wouldn't be possible with only the functions in TabContainer. That being said, such a function should only exist to provide internal access for use cases that someone else might think about in the future, not for existing use cases. Being able to do something via getTabs() is not an excuse to not provide the functionality in TabContainer. So a function to set the height of the tabs should still be added, even if getTabs() is added as well.

In the documentation of the getTabs function you say that it returns the internal Tabs object. The word "internal" should definitely appear in the description to indicate that you are typically not supposed to use the function. I used to mark such functions as "@internal" as well, but that removes them from the documentation by default, so now I try to only do that if I really don't expect a user to ever call the function.

it's needed for case if user forbid selection changing in onSelectionChanging handler

I didn't look at the code closely enough to realize that you added the ability to abort the change. I should probably do something similar for closing ChildWindow.

Some other minor remarks:
- There are still some functions starting with capital letters (Select and Count).
- Documentation should only start with "//!<" if it is placed next to the code. If above the code, it should just be "///".
- Count() should probably be renamed to getPanelCount() for consistency with other widgets.

About the coding style, I usually don't write braces around bodies in if statements when they only contain a single line of code. For example
if (idx == 0)
{
    Select(m_panels.size() - 1);
}
else
{
    Select(idx - 1);
}could be written more compactly as
if (idx == 0)
    Select(m_panels.size() - 1);
else
    Select(idx - 1);
I don't care enough about it to remove them when cleaning up the code (I kept them in SpinControl as well), but I do see them as superfluous (unless they are added to increase readability such as with nested for loops or when the body of the if statement is split over multiple lines).

Updated.
Single remaining part is saving/loading the widget to/from file.
Default implementation won't work, because not all panels are holding in inner container so their renderers are not inserted at this point into the renderer map.

So the problem is that only the selected panel is part of the container, right?
I think all panels should be added to the container, you should just use setVisible to show/hide to right one instead of removing and adding from the container.