Added alignment to TabContainer #174

SteffenPloetz · 2022-08-21T09:11:18Z

Added [Top, TopFixedWidth, Bottom and BottomFixedWidth] alignment to TabContainer; including the new Layout::Operation::BindingNumberOfChildren to achive this.

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 or left-aligned with fixed width.

texus · 2022-08-21T15:55:43Z

I'm grateful for your contribution, but I fear you did too much work. I apologize for the long list of critics, I don't want to discourage contributing, but it just seem like you have have over-engineered some things, and many comments are about things that can be removed again.

The reason ObjectConverter exists is to pass properties for renderers, it only needs to contain the types that are used in widget renderers (and preferably as few types as possible). So TabAlign doesn't really belong in ObjectConverter. The TabAlignment class is also not needed for the same reason. You seem to have taken inspiration from the TextStyles class, but that class is unfortunately used in a very different circumstance compared to TabAlign (which is more similar to e.g. EditBox::Alignment). After this, the only place where the TabAlign enum would still be used is by the TabContainer, and then I think it makes more sense to move it inside the TabContainer (e.g. like Alignment is part of in EditBox or VerticalAlignment is part of Label).
When the ObjectConverter is removed, you also don't need the code in Serializer.cpp and Deserializer.cpp, instead the serialization can be moved into the TabContainer itself. Widgets usually have a save and load function that serializes them to a file, but TabContainer didn't have any special properties yet which is why it doesn't override the save function. It should ideally override the save function and have code in save and load similar to e.g. the Label widget and its VerticalAlignment property. The FixedWidth would also need to be saved and loaded there.
As mentioned on the forum, I don't think layouts needs to be used for this and the changes to Layout.hpp and Layout.cpp can be removed. As the alternative, I would update the width manually. Have the addTab, insertTab and removeTab functions simply call layoutTabs and call m_tabs->setWidth(m_tabs->getTabCount() * m_tabFixedSize) there when the fixed size is set. On first sight that should remove the need to use layouts and simplify the code in layoutTabs().
It's great that you found the bug in removeTab, but the fix introduced a subtle change in behavior. The removeTab must always call select to generate an event, while your fix is now only calling it when removing the selected tab. So maybe it would be best to keep the old select(m_tabs->getSelectedIndex()); line and just add if (m_tabs->getSelectedIndex() == -1) m_tabs->select(m_panels.size() - 1); right before it.
As I mentioned on the forum, my preference is that TabAlign only contains Top and Bottom and is independent on whether the tabs uses fixed width or not. I'll explain why here, but I will leave the decision up to you, I will accept this PR even if you don't change this part.
With your current code, someone who just wants to make the tabs fixed-size need to call both setTabAlign(TopFixedWidth) and setTabFixedSize(width) because the default width of 100px probably isn't exactly what they need (there is no default that will work for everyone as each program will have a different requirement). There will be people that forget to call setTabAlign (because they don't read documentation or they overlook it) and only call setTabFixedSize and would be surprised that their tab width isn't changing.
If the default width is set to 0 and this implies the old behavior where the the tabs are stretched then you eliminate that issue. Those who want to change the tab width only need to call a single function (setTabFixedSize), and they don't have to call some other function that has a name which is unrelated to what they want to change.
Private class members don't really need doxygen comments because they don't generate documentation. Having 3 lines of comments on top of e.g. m_position just to state that it stores the widget position might be a bit too much. I would try keeping the code more compact by using a normal comment there, possibly even positioned behind the declaration if the description is short (e.g. Layout2d m_position; // Stores the position of this widget).
I do feel like the documentation of that line is a unnecessary though. In comparison, the comment you added to m_isSet in TextStyle actually adds information to just seeing the variable name, so there the comment is more useful.
Public members need verbose comments because documentation is generated from those comments, but private members should only be commented if the comment actually adds some value. I know I put too few comments on private members, and there are places where I should provide more information, but you don't have to compensate for that by adding comments for everything :)

SteffenPloetz · 2022-08-22T21:04:58Z

Thank you for your detailed comment. It makes some things clear that are not apparent from the code and documentation - and that's a real shame, because it unnecessarily increases the idle effort.

ObjectConverter: I'll remove TabAlign as suggested. You are right - I have taken inspiration from the TextStyles class.
Serializer.cpp and Deserializer.cpp: I'll remove TabAlign as suggested.
TabContainer:save(): I'll add it.
TabContainer::load(): I'll extend it.
I don't think layouts needs to be used for this: That's the only point I'm not entirely convinced about.
Bug in removeTab: I'll change it as suggested.
TabAlign should contain Top and Bottom only: I'll change it as you suggested.
Private class members don't really need doxygen comments: I totally agree. I will take back unnecessary comments if I come across them. Unfortunately your example is protected, and protected members are important in case if inheritance. I also find many lines full of hyphens (above comments, below comments, as delimiters) and also examples where these lines are omitted. What is the idea for the future? As compact comments as possible?

texus · 2022-08-23T18:05:20Z

Thanks for making the changes, there are still a few things I notice:

TabAlignment.cpp and TabAlignment.hpp files still exist
I think the TabAlignment class can still be removed, m_tabAlign could be of type TabAlign.
The tests are crashing on your save/load functions somehow (the "android-sdl-ttf-gles2" job failure is because TabAlignment.cpp still exists, the "macos" job failure can be ignored, but the "linux-latest" job is the one that runs the tests and crashes on vector::_M_default_append while loading the TabContainer from a file). If you don't know what the problem could be then I will debug this once the rest of this pull request is finished, so you don't have to worry about this.

It makes some things clear that are not apparent from the code and documentation

If you have specific suggestions on where I could add which information then I'm definitely open for them.

That's the only point I'm not entirely convinced about.

The reason layouts aren't useful for this is because they don't do anything automatically. Unless I'm overlooking something, your implementation won't actually do anything. When adding new tabs or removing new tabs, how is the width of the Tabs widget updated? When you set the fixed size then you call layoutTabs which calls (a slightly more verbose version of) m_tabs->setWidth(m_tabFixedSize * Layout(Layout::Operation::BindingNumberOfChildren, m_tabs.get()));, but how does the layout know when the amount of children changes? You would have to manually tell the layout on every tab insert and remove, at which point I believe you might as well call m_tabs->setWidth on each insert or remove.

Having BindingNumberOfChildren for containers is a bit different, because there such logic (i.e. informing the layout about adding and removing widget from the container) could be implemented in the Container base class and you would simplify such functionality for any derived class (if such functionality would be needed). But even then I would need to see some use case to consider it, because the only use case that I can come up with is to auto-size a BoxLayout widget. And if that is the only use case then it would probably be better to implement it inside that class instead of involving the layouts (although that would be debatable). But specifically for the tabs widget I don't see how BindingNumberOfChildren simplifies anything (neither for maintenance nor for the user of the library).

If I overlooked something and BindingNumberOfChildren is functional while inserting new tabs then you can keep the code and I will think about it again, my current opinion is based on the assumption that it won't work without extra work.

Unfortunately your example is protected, and protected members are important in case if inheritance. What is the idea for the future? As compact comments as possible?

That's a very good point. I've ignored protected members for now but I guess a few people (those that inherit from the widget) would still benefit from the comments. I would keep it compact, without lines full of hyphens. If the comment is above the variable, the comment (one or more lines) needs to have /// on each line instead of a normal // so that doxygen picks it up. For short comments behind the variables (which I would do for most variables as they don't need much explaining), the comment can start with //!< instead of the normal // for protected members (again for doxygen).

I checked the documentation and it does already list the protected members, even those that aren't documented, so maybe documentating trivial members still isn't necessary.

I also find many lines full of hyphens

Those are used for increasing readability. There are 3 situations where they are used:

To seperate function definitions in the cpp files.
To separate sections (e.g. separating public and private parts in a class). You will also find them before or after namespaces or after class declarations, but I'm not even certain if there is a consistent rule for such cases, so the usefulness of these lines is debatable.
Surrounding the description of functions (until now both public and private functions). This is something that I've inherited from the SFML codebase.

I've never questioned my use of whitespace or hyphen-lines, as this is just my personal style and I've never had to create rules for them, so I'm open for changes in this style.

I've noticed that e.g. your layoutTabs doesn't have the hyphen-lines surrounding its description, probably because I told you that it isn't needed for private members. But until now I've always added them for all functions (even private), just not for variables. Looking at the code I'm fine with the new style though, i.e. keeping private functions compact as well without hypen-lines surrounding its explanation.

The two lines of whitespace between those functions looks bad though, I would recuce that to a single line. Which got me thinking further, why am I even using 2 lines between documented functions? While I did that for readability, I would be fine with just reducing the whitespace to only a single empty line between all functions, even the public ones, to be able to have a more consistent style.

texus · 2022-09-10T09:37:33Z

Will you still be updating this PR? Or should I make the last changes myself to finish what you started?

SteffenPloetz · 2022-10-03T11:15:54Z

Sorry for the late reply - I had a very intense time professionally. I had to successfully complete the monitoring audit for ISO 27001 as well as the audits for information security and data protection in order to pass TISAX label demands for my employer.

I will try to work through the open items quickly now....
I'll start with the failing tests. I've found 2 issues and fixed them.

I agree that the TabAlignment class (hpp and cpp) can be deleted. In my source tree it is, but I'm not sure if that arrived in the pull request.

Regarding BindingNumberOfChildren. I have to adjust to your philosophy first - which I would call "solve requirements locally if possible instead of providing generic extensions" based on your comments. I have mostly dealt with Microsoft frameworks so far - whose philosophy I would describe as "provide generic extensions, we don't know all the use cases of the application programmers out there".

In fact, I have a GUI test app that adds and deletes tabs and also tests whether the currently active tab can be deleted without crashing (buttonTabSizeCallBack). It also tests the 4 possible combinations of alignment/fixedSize (buttonTabAlignCallBack).

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

void buttonTabAlignCallBack()
{
    if(m_tabContainer->getTabAlignment() == tgui::TabContainer::TabAlign::Top)
    {
        if (m_tabContainer->getTabFixedSize() <= 0.0F)
        {
            m_tabContainer->setTabFixedSize(m_tabFixedSize);
            this->setStatusText(L"The tab container alignment is now: Top, FixedSize");
        }
        else
        {
            m_tabContainer->setTabFixedSize(0.0f);
            this->setStatusText(L"The tab container alignment is now: Bottom, FullSize");
            m_tabContainer->setTabAlignment(tgui::TabContainer::TabAlign::Bottom);
        }
    }
    else if(m_tabContainer->getTabAlignment() == tgui::TabContainer::TabAlign::Bottom)
    {
        if (m_tabContainer->getTabFixedSize() <= 0.0F)
        {
            m_tabContainer->setTabFixedSize(m_tabFixedSize);
            this->setStatusText(L"The tab container alignment is now: Bottom, FixedSize");
        }
        else
        {
            m_tabContainer->setTabFixedSize(0.0f);
            this->setStatusText(L"The tab container alignment is now: Top, FullSize");
          
8000
  m_tabContainer->setTabAlignment(tgui::TabContainer::TabAlign::Top);
        }
    }
}

So - if TabAlignment class is removed now and you agree to the BindingNumberOfChildren implementation - I'd say I'm done.

Two tests are failing because the TabAlign class isn't deleted completely.

…ignment

Adjustment of overlooked minor inadequacies.

…e tabs a fixed width (#174)

texus · 2022-10-05T14:05:14Z

Sorry for the late reply - I had a very intense time professionally

No problem, I didn't have much time myself either (this is the first day since August where I would have enough time to further look into this myself, so your timing was perfect).

I've merged your changes in 5dccd0d
This PR will be marked closed instead of merged as I didn't merge your PR directly. Your latest changes added many files that existed in TGUI 0.9 but were deleted since. So I bundled all your commits together and removed those files, but of course kept you as author of that commit.

I've made another commit afterwards that removes the BindingNumberOfChildren again. This is partially a philosophy difference, but the more "generic" method was hiding the fact that your code wasn't actually doing anything. Your code worked because you recreated the layout every time, so using a layout had no use. I could literally replace your BindingNumberOfChildren layout with a constant value, and the code still does exactly the same thing.

This has nothing to do with your changes, but I also made several other changes, such as removing m_index. I didn't like that it could potentially get out of sync with the selected index in the Tabs widget and it made implementing removeTab difficult without introducing subtle bugs (like the one you fixed). Because of this, you may want to re-run the tests that you created with the new code, to make certain that I didn't introduce any new bugs in doing so.

SteffenPloetz · 2022-10-19T05:41:34Z

Thank you so much for finishing my PR! I've looked at your changes, including replacing the m_index with a direct pointer. I also understand your objections to some of my changes better now. I think I need to get much more involved with the TGUI source code before I can make any meaningful contributions. But maybe I can do something for TGUI in another way, for example: https://www.codeproject.com/Articles/5344092/First-Steps-Tutorial-How-to-Start-with-TGUI-Runnin

texus · 2022-10-23T14:33:54Z

I haven't had time to look at the code yet, but here are some minor questions/remarks on the article:

After the second [Configure] run, I fixed these errors: X11_xcb_*-NOTFOUND

Did it actually show an error message, or were those values just red (which only means that properties were added since the last time you pressed Configure)? This can be very confusing, the "NOTFOUND" does mean that those libraries couldn't be found, but that shouldn't be a problem: TGUI doesn't use them (it only needs X11/Xlib.h and X11/cursorfont.h). So even though it is red and says "NOTFOUND", it isn't necessarily an error.

TGUI just asks CMake where to find X11, and CMake automatically also searches for X11 related components such as XCB. Whether XCB is found should make no difference for building TGUI. It would be a better user-experience if those XCB properties didn't show up, but that is up to CMake and outside my control.

TGUI only uses X11 when the backend provide insufficient support for mouse cursors btw, so you won't see any of that output anymore once SFML 2.6 is released and available in openSUSE.

ideally the project would be set to the C++ standard 14 as well, or if not available at least to the C++ standard 11

TGUI requires the project to be build with at least c++14, you will get compile errors if you include a TGUI header when using "-std=c++11" (the version used in your project should be equal or higher to the version specified with TGUI_CXX_STANDARD when compiling TGUI).

GCC >= 11 uses c++17 by default, while GCC >= 6 uses c++14 by default, so you typically don't even need to specify the c++ version unless you need a specific version for your own code.

Your screenshot is showing the C11 option by the way, not C++11.

I assumed that I can set the system language (I'd like to use English) and the keyboard layout (I need to use German) separately during installation

I've had plenty of similar experiences in the past when swapping linux distros before I ended up with Manjaro, I really dislike installers that force a layout based on the language. I think I have switched the system language in the past after installation, but what I usually do is install it in English and then just change the keyboard layout after the installation.

Mesa 20.2.5 pretends 3.3 on my 3.0 hardware

Out of curiosity: what hardware do you still have that doesn't support OpenGL 3.3?

SteffenPloetz · 2022-10-29T06:37:42Z

Thank you for your feedback. At the time I ran Cmake, I already knew that red lines are not errors, but additions.

After the second [Configure] run, I fixed these errors: X11_xcb_*-NOTFOUND

If I misinterpreted the message, it is poorly done from my point of view. If this library is not required, it should be removed from the list of prerequisites. If this is out of your control, it should be described how to handle this. - If it is really sure that this library is not needed, I would like to add that in the article.

ideally the project would be set to the C++ standard 14 as well, or if not available at least to the C++ standard 11

Good hint - thanks! I have changed it.

I think I have switched the system language in the past after installation, but what I usually do is install it in English and then just change the keyboard layout after the installation.

I was going to do that too - but it ends up in a wild orgy of command line calls. 20 years ago I even calculated the Hsync and Vsync values for my monitor and entered them manually into the X11 monitor configuration. But in 2022 I simply don't feel like fiddling around like that anymore - especially since other distros show how it can be done. - Manjaro I actually do not know yet. The WiKi article sounds interesting. I think I'll do a test installation.

What hardware do you still have that doesn't support OpenGL 3.3?

lenovo ThinkPad T520, 12GB RAM, core i5-2540M @ 2.60GHz, 1920x1080 SAMSUNG display
This is my test hardware. However, I usually work on a Microsoft Surface Studio.

texus · 2022-10-29T08:05:55Z

If this library is not required, it should be removed from the list of prerequisites. If this is out of your control, it should be described how to handle this. - If it is really sure that this library is not needed, I would like to add that in the article.

There are 2 cases. When using GLFW or SFML >= 2.6, TGUI has no dependency to X11 and it won't search for it. When using SDL or SFML < 2.6, TGUI requires X11 and a call to find_package(X11) is made (unless you set TGUI_USE_X11 to OFF in which case you simply won't get proper mouse cursors when resizing a child window).

The find_package(X11) call will use CMake's built-in FindX11.cmake script, which actually looks for different libraries depending on the CMake version: https://cmake.org/cmake/help/latest/module/FindX11.html
Based on the documentation, it will look for XCB when using CMake >= 3.18.

TGUI only needs the X11_INCLUDE_DIR and X11_X11_LIB variables (although I should probably update my script to use the more modern X11::X11 target). That entire list of variables on that FindX11 documentation page can be created by the find_package(X11) call however, even though I don't need them.

There are 2 things that I can think of which could do to improve the experience, but neither of them sound like a good idea.

Not call find_package(X11) and instead hardcode a few paths where I except to find X11.
For each of these variables in the documentation, call unset in my cmake script when they contain a NOTFOUND value.

lenovo ThinkPad T520, 12GB RAM, core i5-2540M

I did a quick search and the HD 3000 graphics that come with that processor might actually support 3.3 on Linux and macOS. On Windows the drivers limit it to OpenGL 3.1, but the mesa driver on Linux might actually provide support for it, it might do more than just pretend to work 😄

SteffenPloetz · 2022-11-06T09:01:16Z

Thanks for all the effort you took with my problems and questions and the many suggestions that came out of it. I will adjust the article again according to your comments.

Thanks for the tip about Manjaro. Summarized in one word: Nice!

lenovo ThinkPad T520, 12GB RAM, core i5-2540M

Yes I can ask Mesa to provide me with an OpenGL 3.3 and I will get it. Why Mesa only provides an OpenGL 3.1 without the request is not important to me - I can request a 3.3 and the world is nice.

Steffen Ploetz and others added 4 commits August 20, 2022 10:39

Added alignment to TabContainer

05c410f

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 or left-aligned with fixed width.

Added alignment to TabContainer

7d41d31

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 or left-aligned with fixed width.

Remove bindNumberOfTags().

87f20e7

Hopefully final small fixes.

e867aed

Implement PR comments (2022-08-22)

a005650

SteffenPloetz closed this Aug 22, 2022

texus reopened this Aug 23, 2022

Repository owner deleted a comment from SteffenPloetz Aug 23, 2022

SteffenPloetz and others added 6 commits October 3, 2022 17:25

Implement PR comments (2022-08-22)

6d1d404

Merge remote-tracking branch 'origin/0.10' into TabContainerAlignment

df5a89f

Try to fix the failing tests.

d1be0ea

Two tests are failing because the TabAlign class isn't deleted completely.

Merge branch 'texus:0.10' into TGUI

185e9d7

Merge remote-tracking branch 'SteffenPloetz/TGUI' into TabContainerAl…

90b958e

…ignment

Adjustments of TabContainer documentation and load() method.

2d4c771

Adjustment of overlooked minor inadequacies.

texus pushed a commit that referenced this pull request Oct 5, 2022

Added ability to TabContainer to put tabs below the panels and to giv…

9fb32c9

…e tabs a fixed width (#174)

texus added a commit that referenced this pull request Oct 5, 2022

Cleanup TabContainer changes from PR #174

7b1715a

texus pushed a commit that referenced this pull request Oct 5, 2022

Added ability to TabContainer to put tabs below the panels and to giv…

5dccd0d

…e tabs a fixed width (#174)

texus added a commit that referenced this pull request Oct 5, 2022

Cleanup TabContainer changes from PR #174

76a02e6

texus closed this Oct 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added alignment to TabContainer #174

Added alignment to TabContainer #174

Added alignment to TabContainer #174

Added alignment to TabContainer #174

Conversation