TGUI: a c++ GUI for SFML (with Form Builder)

[FRex]

I learned something new today. I didn't knew what happened when casting to a wrong type.

With all due respect for being author of tgui and all, but I'm facepalming hard now.
I don't have copy of standard but I think it says classes must have virtual methods to be dynamic castable.
And references throw bad_cast exceptions while pointers come out as nulls, since they can't be 'null' or point to invalid location.

[texus]

I learned something new today. I didn't knew what happened when casting to a wrong type.

With all due respect for being author of tgui and all, but I'm facepalming hard now.

That's what you get from not learning with a book, but learning only what you need at a given moment: you'll skip small parts.

I am going through "The C++ Programming Language" right now, so eventually (when I reach chapter 15) I will finally get a decent explanation. I only need to know the first 12 chapters this year for university, but I'll probably read the rest too.

Thanks for helping to find an alternative method for checking the objects type by the way.

[FRex]

Oh god, I never learned from a book. Don't scare me.
That's how sfgui does that, they have their own cast(the reason why new style casts mimic template function is so that you can roll your own and use it similarly) that does some magic with their smart pointer class but inside it's dynamic cast too.

[netrick]

do that:

ListBox * ptr2;
if(ptr2=dynamic_cast<ListBox>(ptr))
{
//do something with ptr2, its valid for sure
}
 

+1 to it and it's also what I'm using in my project. This way, you don't have problems with types' names and you even don't need get type function at all. I think that's very clean C++ish solution. Of course you can keep the enum, but you can as well remove it.

Look at it, so easy and clean. I think it's faster than get object type (not that it makes any difference, but you know ) I think many libs I know don't provide get object type at all.

std::vector<tgui::OBJECT*> & objects = tgui.getObjects();
for (unsigned int i = 0; i < objects.size(); i++)
{
       if (tgui::ChildWindow * ptr = dynamic_cast <tgui::ChildWindow*> (objects[i])
        {
               //do your stuff
        }
}
 

EDIT:
I looked a bit through tgui docs and you can improve some things:
- when you pass int by value, why you have it like "const int argument"? const is pointless here, just pass basic types by normal value
- you pass strings as "const std::string argument", while it should be "const std::string & argument", and the same applies for every object, like sf::String (look at sfml for example)
Actually you pass some objects by const reference and some by const value, you should correct it (will be faster and better designed)

Also, there is another thing to improve. Your install script for linux is inconsistent. First of all, usually when you "configure make make install" lib on linux, it goes to /usr/local/lib and headers go to /usr/local/include. Your script gives libs to "/usr/lib" and doesn't copy headers anywhere at all (which is very windowish solution and you should just copy everything to /usr/local, as this way you don't need to set include/lib/code completion paths, but just #include or -ltgui. And much more transparent system. Of course, when you make install sfml it goes to /usr/local/lib and /usr/local/include. Its just default location for make install (sudo make install on ubuntu).

You know, windows itself doesn't offer any place to keep user libs and headers so you need to store them on your own, but linux has such an amazing location as /usr/local And adding libs to /usr/lib isn't good, because that's place for libs that aren't compiled by user, but for those that are already provided by system.

Just sugestions to make TGUI even better

[texus]

Of course you can keep the enum, but you can as well remove it.

It will be kept for a very simple reason: if you try to look for all Panels then with the dynamic_cast method you will find ChildWindows as well (because they inherit from Panel). So the enum will definitely be kept for such cases where the exact type is important.

I looked a bit through tgui docs and you can improve some things

I'll look into those things. The problem with the consts is caused because I added const parameters and functions in just one evening. I should have taken more time and do it a bit more concentrated.

Also, there is another thing to improve. Your install script for linux is inconsistent.

I think I used /usr/lib instead of /usr/local/lib because I read something about some distros not looking in the /usr/local/lib folder. But the install script should be redesigned anyway, together with the whole installation way on linux (the Form Builder should be installed to the correct place and stuff like that).
But I am not going to spend all my available time in fixing something that isn't broken, so this will be delayed for a while.

Just sugestions to make TGUI even better

Thanks for the input, I'll try to change all those things.

[Nexus]

To my experience, dynamic_cast is not "the clean solution". It is often considered bad design among the C++ community

Case differentiations with dynamic_cast, typeid or enumerators share one problem: They defeat polymorphism. Polymorphism intends to access objects of different types uniformly, internally they dispatch to different functionality using virtual functions. The user need not know what dynamic type an object has in order to perform operations on it. In addition, dynamic_cast may be rather slow depending on the class hierarchy and the RTTI implementation. Of course, it has its use cases, but one should look for alternatives when available.

Where appropriate, prefer virtual functions over dynamic_cast. Here, this is of course difficult, because you can't change the TGUI class hierarchy. Some time ago, I couldn't accept this limitation any longer and had to write dispatchers in Aurora

aurora::SingleDispatcher extends virtual functions in order to work non-intrusively with existing class hierarchies. You can have functions or functors for different dynamic types, and register them at the dispatcher. When you invoke them without knowing the dynamic type, the correct function is looked up and invoked. Like this, you can cleanly separate different functionalities in their own functions, and do not need to perform an explicit case differentiation.

#include <Aurora/Dispatch/SingleDispatcher.hpp>
#include <Aurora/Tools/ForEach.hpp>

#include <iostream>
#include <vector>
#include <memory>

// Example class hierarchy
struct Widget { virtual ~Widget() {} };
struct Button : Widget {};
struct ListBox : Widget {};

// Your function for buttons
void MyFunction(Button& b)
{
        std::cout << "MyFunction(Button&)\n";
}

// Your function for list boxes
void MyFunction(ListBox& l)
{
        std::cout << "MyFunction(ListBox&)\n";
}

int main()
{
        // Create dispatcher, register both functions
        aurora::SingleDispatcher<Widget&> dispatcher;
        dispatcher.add<Button>(&MyFunction);
        dispatcher.add<ListBox>(&MyFunction);

        // Create container of widgets
        std::vector<std::unique_ptr<Widget>> widgets;
        widgets.emplace_back(new Button);
        widgets.emplace_back(new Button);
        widgets.emplace_back(new ListBox);

        // For each widget (VC++10 lacks range based for), invoke correct function
        AURORA_FOREACH(std::unique_ptr<Widget>& w, widgets)
                dispatcher.call(*w);
}

Output:

MyFunction(Button&)
MyFunction(Button&)
MyFunction(ListBox&)

[FRex]

To my experience, dynamic_cast is not "the clean solution". It is often considered bad design among the C++ community

If they seen the sh*t that goes on in SFGUI and SFML..
Singletons, reinterpret casts to void* and back, const casts, mutables, memset on a RenderWindow to hack into vertex and view cache. And it works great.

[netrick]

@Texus
As for that install script - you should just call make then sudo make install - this will install form builder to /usr/local/bin and libs and headers to proper locations. What's more, if you run this on distro which doesn't support /usr/local it will copy the files to proper location! So that's the best way imo.

@Nexus
That looks um... stunning, but to be honest I'm yet to see library which uses that solution to this problem. I think tgui is such a small project that such compilcated designs aren't needed. I may be wrong though. You know, KISS!

Also, if I understand correctly it won't solve tgui's problem. The class hierarchy is quite good and when you have virtual function like setsize etc everything goes okay. The problem is that the base object class can't have getTitlebarHeight functions which is only for child window and that's why polimorphism won't help here.

[texus]

As for that install script - you should just call make then sudo make install

To do that you need a makefile. The whole idea from the install script was to provide an easy alternative to using cmake and makefiles. The install script was mainly added for people to lazy to download and setup cmake. Perhaps it would be better to just remove the install script instead of trying to optimize it.

[netrick]

You know - sfml on linux requires cmake (2.0 is not in repos) and tgui requires github sfml version, so that may be the best option. Because we can assume that if you have github sfml2 version, you have cmake installed.

[Laurent]

The problem is that the base object class can't have getTitlebarHeight functions which is only for child window and that's why polimorphism won't help here.

But why would you need to call a specific function on an object that you don't store, that you need to find within the untyped children of a parent object?

The whole idea from the install script was to provide an easy alternative to using cmake and makefiles

CMake provides a unified and simple way to do things, especially on Linux. All you have to do is:

cd tgui
cmake .
make
sudo make install

Scripts may simplify things on Windows, but definitely not on Linux

[Nexus]

That looks um... stunning, but to be honest I'm yet to see library which uses that solution to this problem.

I know, you won't find most parts of Aurora anywhere else, that's why I have written this library. Also the principle of copyable smart pointers, which I meanwhile use all the time and which simplify my life a lot, is widely unknown in the C++ community. Interestingly, people prefer to rewrite the Big Three all the time and to use new/delete

I think tgui is such a small project that such compilcated designs aren't needed. I may be wrong though. You know, KISS!

It wasn't a proposal to include this in TGUI, the problem occurs on client side anyway. I wanted to show an elegant alternative to explicit type differentiations. I am aware that in simple situations, a dynamic_cast can be the easier approach.

dynamic_cast becomes problematic when many classes are involved. The central case differentiation turns out to be a dependency bottleneck (all types have to be complete), and the order of the if/else if statements must be correct, and kept synchronous with the class hierarchy. In such a case, a more scalable solution might be worth while.

The problem is that the base object class can't have getTitlebarHeight functions

If I've understood correctly, the problem is rather that the user wants to call getTitlebarHeight(), but he only has a base object. That is, he must find out if this object is also a derived object.

[netrick]

If I've understood correctly, the problem is rather that the user wants to call getTitlebarHeight(), but he only has a base object. That is, he must find out if this object is also a derived object.

That's what I meant but my english is bad. The user has a pointer to basic object and wants to call function that only exist in child class. If base class had getTitlebarHeight() virtual function then there would be no problem. But of course it's impossible, so as you said we need to check type of the object that pointer shows on.

Thanks for explaination, now I see the purpose of your approach. I will remember it, maybe one day when I will be doing really big project it will be useful. However I think that we can agree that for current state of tgui, simple dynamic cast approach is enough (there aren't really that much classes in hierarchy).

edit:
A little off topic. You said about foreach loop in C++ 11. I love it from script languages and now I see that my g++ supports it C++11 is really great, I still don't know much of it, but every part that I know is so useful.

@Lauren
Well if I understand you correctly, that's what I need to do in TGUI. There parent class object and child class child window. I only have base class pointer and I need to find out if it's child window to call non-virtual function which only child window has.

[Laurent]

I only have base class pointer and I need to find out if it's child window to call non-virtual function which only child window has.

But why are you in this situation? Can't you store/access the object directly with the correct type?

I use Qt all the time. Things are similar: it has a QObject base class, functions to get/find children, a custom metatype system, and a qobject_cast conversion function similar to dynamic_cast. But I never use this stuff. Objects that I need to manipulate are stored directly (often as members of their parent class), and I don't have to find and typecast them.

[netrick]

Well, because in tgui there isnt getAllObjects<Type>() (hmm Texus, what do you think about such a function?), but only getAllObjects() which gives pointers to base class. And I need to call for every child window function which keeps them in window, because there are some design problems with it and I must do it manually in my loop.

There are 2 solutions to avoid the whole problem:
1) Make child windows stay in window at global level (but currently it won't be done because there are some design problems, but I would just keep every child window inside main window and ignore panels for this)
2) Write function getAllObject<type>() which returns proper pointer for proper type (pointer to child class)