How do I start code contribution?

Started by SteffenPloetz, 20 August 2022, 12:05:00

SteffenPloetz

I like what I have seen so far. But even good things can be improved.

I could well imagine being involved in that. I wanted to start with something simple and so I extended the TabContainer.

The tabs of a TabContainer can now be aligned at the top (above the panels - the previous and default behavior) or at the bottom (below the panels) and additionally spread over the whole widget width (the previous and default behavior) or left-aligned with fixed width.

Bildschirmfoto_2022-08-20_11-50-18.png

But what do I do now exactly to make my solution available to the community? How can I support TGUI with code contribution? Didn't find any hint regarding that in https://github.com/texus/TGUI/blob/0.10/CONTRIBUTING.md.

I'm an advanced Visual Studio developer (my native language is German - if this could be of interest for translations). But I want to support OSS and so I'd like to contribute using Debian Linux and Code::Blocks 20.03. I already have a github account.

Thanks in advance.

texus

#1
First of all, thank you for being willing to help and contribute code.

You could send a Pull Request on github with the changes you made. That way I can see what you changed exactly, provide feedback on it if something still needs to be adjusted and eventually merge the change into TGUI for everyone.

TabContainer shouldn't become too complicated as the user can always create the Panel and Tabs widgets themselves if they need more control, but the changes you describe seem like helpful additions so I'd happily accept those.

How did you make the tabs fixed width exactly? By specifying the total width of the 3 tabs, or by adding some function that specifies the width of a single tab and then have the Tabs widget grow when adding another tab?
If implemented properly, the latter might be useful for the Tabs widget itself as well (and not just TabContainer).

I guess I should have the CONTRIBUTING file explicitly link to the Issues and Pull Requests on github to make it a bit clearer.

SteffenPloetz

Thank you for the quick response. I use GitKraken and get "Access denied". Do I have to create a fork? And if - how do you get informed about the fork?

The implementation is based on one single fixed value ( m_tabContainer->setTabFixedSize(150.0f); ) that is multiplied with the number of tabs ( m_tabs->setWidth(bindNumberOfTabs(m_tabs, m_tabFixedSize)); ). To achieve this, I added one binding and two helper to the Layout class:
        Layout bindNumberOfChildren(Container::Ptr container, const Layout& factor)
        {
            return Layout{Layout::Operation::Multiplies,
                          std::make_unique<Layout>(Layout::Operation::BindingNumberOfChildren, container.get()),
                          std::make_unique<Layout>(factor)};
        }

        Layout bindNumberOfTabs(Tabs::Ptr tabs, const Layout& factor)
        {
            return Layout{Layout::Operation::Multiplies,
                          std::make_unique<Layout>(Layout::Operation::BindingNumberOfChildren, tabs.get()),
                          std::make_unique<Layout>(factor)};
        }

My TabContainer creation code for UI test is:
        m_tabContainer = tgui::TabContainer::create();
        m_tabContainer->setPosition("0", "60%");
        m_tabContainer->setSize("100%", "80");
        m_tabContainer->setTabFixedSize(150.0f);
        auto tabPanel1 = m_tabContainer->addTab(L"Tab-Pane-01 (R)", true);
        tabPanel1->getRenderer()->setProperty("BackgroundColor", tgui::Color(255, 192, 192, 255));
        auto tabPanel2 = m_tabContainer->addTab(L"Tab-Pane-02 (G)", false);
        tabPanel2->getRenderer()->setProperty("BackgroundColor", tgui::Color(192, 255, 192, 255));
        this->getGui().add(m_tabContainer, "MyTabContainer");

To test the new functionality I use two call backs (that iterate the possible alignments and add/remove a tab to be able to test all possible combinations):
    void buttonTabAlignCallBack()
    {
        if(m_tabContainer->getTabAlign() == tgui::TabAlign::Top)
            m_tabContainer->setTabAlign(tgui::TabAlign::TopFixedWidth);
        else if(m_tabContainer->getTabAlign() == tgui::TabAlign::TopFixedWidth)
            m_tabContainer->setTabAlign(tgui::TabAlign::Bottom);
        else if(m_tabContainer->getTabAlign() == tgui::TabAlign::Bottom)
            m_tabContainer->setTabAlign(tgui::TabAlign::BottomFixedWidth);
        else
            m_tabContainer->setTabAlign(tgui::TabAlign::Top);
    }

    void buttonTabSizeCallBack()
    {
        if (m_tabContainer->getPanelCount() == 2)
        {
            auto tabPanel3 = m_tabContainer->addTab(L"Tab-Pane-02 (B)", false);
            tabPanel3->getRenderer()->setProperty("BackgroundColor", tgui::Color(192, 192, 255, 255));
        }
        else
            m_tabContainer->removeTab(m_tabContainer->getPanelCount() - 1);
    }

Beside that I also fixed a bug in removeTab(), that crashed if the tab to remove is the selected one.

texus

You can't push directly to texus/TGUI (which is what I assume you get "access denied" for), you have to create a fork (which I think you already did: https://github.com/SteffenPloetz/TGUI).

In your fork you should be able to commit changes and push them. If that works then you should see the changes at https://github.com/SteffenPloetz/TGUI

On your fork on github, press the "Pull Request" tab on top (which takes you to https://github.com/SteffenPloetz/TGUI/pulls), where you can press the "New pull request" button to send the changes to the main TGUI project. After you filled in your description and send the request, I would get notified about it and the pull request will appear publicly on https://github.com/texus/TGUI/pulls

Unfortunately I think your changes are a bit too invasive to the code to be merged as-is: I don't like seeing changed to the Layout or global scope (tgui::TabAlign) for something specific to one widget. While bindNumberOfChildren might have other use cases, I feel like such addition needs to be discussed first and not merged together with a small addition elsewhere.

The setTabAlign function combines 2 unrelated properties: location of the tabs and whether they use the full width or not. While you probably needed to change both for your use case, I think it would be more useful to others if they are separate. Both properties are useful, but I think there are going to be people who just want to have fixed-sized tabs and they won't expect the function they need to be called "setTabAlign".

If it helps reduce the code then I'm fine with combining multiple properties like in TabAlign, but you need a setTabFixedSize function anyway, so I think you might as well use that function to determine whether the tabs have a fixed size (e.g. the default value is 0 in which case tabs have the old behavior, while calling setTabFixedSize will choose a fixed tab width).

SteffenPloetz

Thanks for the helpful step-by-step instructions (this does run a bit differently than the corporate Visual Studio steps I'm used to). Would it be a good idea to include this guide in CONTRIBUTING.md?

QuoteUnfortunately I think your changes are a bit too invasive to the code to be merged as-is: I don't like seeing changed to the Layout or global scope (tgui::TabAlign) for something specific to one widget. While bindNumberOfChildren might have other use cases, I feel like such addition needs to be discussed first and not merged together with a small addition elsewhere.
Totally understandable, it's a matter of the philosophy TGUI follows: I will try to reduce it. The difficulty here is that container and tab widgets both have child elements, but they are counted in different ways - here the TGUI API could be a bit more consistent.

QuoteThe setTabAlign function combines 2 unrelated properties: location of the tabs and whether they use the full width or not. While you probably needed to change both for your use case, I think it would be more useful to others if they are separate.
I can well understand the objection. On the other hand, you would then have to consistently tear apart enum TabAlign - and that makes the API more complicated again. Any idea to solve that?
    enum TabAlign
    {
        Top              = 0,      //!< Tabs use the complete width and are above panes
        TopFixedWidth    = 1 << 0, //!< Tabs use fixed width multiplied with tab count and are above panes
        Bottom           = 1 << 1, //!< Tabs use the complete width and are below panes
        BottomFixedWidth = 1 << 2, //!< Tabs use fixed width multiplied with tab count and are below panes
    };

QuoteIf it helps reduce the code then I'm fine with combining multiple properties like in TabAlign, but you need a setTabFixedSize function anyway, so I think you might as well use that function to determine whether the tabs have a fixed size (e.g. the default value is 0 in which case tabs have the old behavior, while calling setTabFixedSize will choose a fixed tab width).

Of course. I have only posted excerpts of my changes.
I look forward to the review comments.

I now have a sense of how TGUI should evolve - I exaggerate wildly to make it clear: Keep things simple and separated in order to minimize maintenance effort (in contrast to other tools that try to maximize programmer's convenience).
If I'm right about that - shouldn't that be pointed out somewhere?

Maybe you've already noticed: I'm a big fan of good documentation - especially for a beginner in TGUI like me it would be very helpful.

Regards

texus

Quotebut they are counted in different ways - here the TGUI API could be a bit more consistent
That's a general issue in TGUI, the code and design has changed a few times over the many years of development, and there is a lack of direction.

There has been an attempt at the beginning of 0.10-dev to make all rendering and layout use reusable building blocks. The prototype can still be found in the Component.hpp file and is only used by the Button widget. The idea was that backgrounds and text (or each tab) would be a component, and widgets would also be a component. This would result in the consistency you are talking about: child widgets or individual tabs would be treated in a similar way.

The component design had its own issues and the idea has been put on hold for now. Maybe for TGUI 2.0 I can revisit this topic and figure out a better design.

Quoteyou would then have to consistently tear apart enum TabAlign - and that makes the API more complicated again. Any idea to solve that?
TabAlign can be moved inside TabContainer, and it would only need a Top and Bottom member as whether or not it uses FixedWidth would in my vision be detemined by some kind of setFixedTabWidth function.
Since there would only be 2 members, an alternative would be to not have an enum at all and just have a setTabBelowPanel(bool) function.
The bool function is how I would have done it, but I have a habit of choosing the most straightforward method to solve issues and not necessarily the best designed one (so I'm not saying you have to do it like that). Keeping the TabAlign enum has the advantage that it could be extended in the future if it becomes possible to have vertical tabs and align tabs on the left or right side of the panel, but this is unlikely to happen any time soon (if ever) so the design can always be changed later if it would be implemented.

QuoteKeep things simple and separated in order to minimize maintenance effort (in contrast to other tools that try to maximize programmer's convenience).
I think in many cases you have find a bit of a compromise between maintainability and programmer convenience.

Keeping things separated might also help to user to more quickly find what they are looking for (although good documentation can help a lot with that too). And I generally do like to keep things as simple as possible.

Isolated/separate code can usually be changed without consequences, but code that is used across widgets usually has an entire history and limitations surrounding it that you can't really be aware of. The layout code for example is newer than the Tabs widget, and it was only created to provide an alternative to fixed positions and sizes (because in TGUI 0.6 the position and size of a position was specified in pixels and couldn't be specified in percentages or relative to other widgets). While it is used from some places inside TGUI, it wasn't designed for that and the entire layout system actually needs a big redesign (also no planned until TGUI 2.0). The more complexity that gets added to the layout classes, the harder it will become to redesign it.

QuoteIf I'm right about that - shouldn't that be pointed out somewhere?
I find it difficult to really formulate rules about how things should be done as there is no clear philosophy, I generally just try to keep things simple.

But this is similar to another issue: there is no goal on how to go forward. Should the API be as easy as possible or should it allow as much customization as possible? While I aim for both, those goals often collide. While I lean more towards simplicity, I don't really have any philosophy about which direction TGUI should grow in, so I also can't provide rules to other on what to do.

QuoteMaybe you've already noticed: I'm a big fan of good documentation - especially for a beginner in TGUI like me it would be very helpful.
Documentation is definitely lacking in TGUI, but I'm not that good at writing documentation and tutorials :)

TGUI 0.10-dev is going to be rebranded as TGUI 1.0-dev soon, and I'm planning on allocating some more time to document stuff afterwards. The current TGUI design still has many limitations, but at some point I just have to stick with something and work within the limitations that exist instead of keep rewriting things.

SteffenPloetz

QuoteTabAlign can be moved inside TabContainer
I got the feeling that TGUI prefers to follow the principle "one class = one file". And additionally you already figured out ...
QuoteKeeping the TabAlign enum has the advantage that it could be extended in the future
... which was my intention.

What I could do to reduce the complexity is setFixedTabWidth() to <= 0.0f and this is equal to "use the complete available space". ;)

QuoteThe layout code for example is newer than the Tabs widget, and it was only created to provide an alternative to fixed positions and sizes
I really think the layout system is outstanding (which doesn't mean it can made even better) and unique compared to other GUIs!

QuoteDocumentation is definitely lacking in TGUI
That might be a point I could take up.