Constructing widgets rather than widget builders

[JamesBremner]

In your sample code you construct ( build ) widgets like this

lecui::widgets::label_builder label(pane_one.get(), "my_label");

This looks like it will call the widget's destructor when it goes out of scope. Usually people want to prevent this happening until the application ends, or when the container is destroyed.

A smaller point: it seems odd to me that you are constructing an instance of a widget builder, rather than an instance of a label.

So something like

lecui::widgets::label * plabel1 = lecui::widgets::label_builder( label(pane_one.get(), "my_label");

( the builder returns a pointer to the widget )

or

lecui::widgets::label& label1 = lecui::widgets::label_builder( label(pane_one.get(), "my_label");

( the builder returns a reference to the widget )

Personally, I prefer the second style.

[alecmus]

Personally, I prefer the second style.

I like the second style. We can do it in the current design by just calling the .specs() method or the () overload as follows:

Either

auto& pane_one = pane_one_builder.get();
auto& label = lecui::widgets::label_builder(pane_one, "my_label").specs();

or just

auto& pane_one = pane_one_builder.get();
auto& label = lecui::widgets::label_builder(pane_one, "my_label")();

What do you think?

This looks like it will call the widget's destructor when it goes out of scope.

The naming in the example code could use some improvements since those are actually builders, and builders can be short-lived since their purpose is just to build the widget and once that's done they're no longer needed. The widget however lives on until either the form or the container is closed, or until the widget is explicitly closed through the widget_manager.

[JamesBremner]

the example code could use some improvements

The sample code is very important because this is how potential users decide if they want to use your library.

Right now your sample code does not look super useable.

The concept of builders just to build the widget and once that's done they're no longer needed seems a complication that the application coders should not have to deal with. Most gui frameworks I have used simply construct the widget and the library looks after everything else.

I think you should find a way of hiding this complexity from the application coder.

[tmnyoni]

That's an interesting suggestion.

[alecmus]

Thanks for the feedback @JamesBremner. We'll work at simplifying the interface further and updating the sample code as well once that's done.

[JamesBremner]

May I suggest the approach I used in windex: a templated factory method to construct all the widgets.

template<class W , class P >
W& factory(P & parent	)	

W is the class of widget to construct
P is the class of the parent widget ( container in your terms, I think )
parent a reference to the parent class
return reference to widget constructed

Every widget can thus be constructed with the same call ( except for the top level window which needs to be special anyway )

For example, constructing a label

auto& label1 = factory< label >( form );

An internal database associates the widgets with their parents so they can destroyed just before the parent is destroyed and can inherit changes to attributes like background color and font.

https://jamesbremner.github.io/windex/classwex_1_1maker.html

[alecmus]

Thank you for the feedback. We are currently working on reverting the interface to something similar to 1674eb2 so that the builders are not public. However, we will not be using templates as this would mean every header is included by default which we have avoided for performance reasons. We prefer that the client code only sees widgets that have been explicitly added to the file, e.g. to see progress indicators one has to explicitly call:

#include <liblec/lecui/widgets/progress_indicator.h>

The solution will address the issue you raised but using a slightly different approach because we wouldn't want client code having to be recompiled every time we add new widgets to the library.

[JamesBremner]

every header is included by default which we have avoided for performance reasons.

Including a header slows the compilation by a few milliseconds. It has no effect on run time performance.

As an application programmer I find it extremely annoying to have to remember to include the correct headers. I add a feature and compilation fails. Why? Oh yes,I need to add some tiny header file. What was it called again?

After doing this ten or twenty times, I am really annoyed at whoever designed the library like this.

It is much kinder to your library users to tell them to include one header file to get all the core widgets. You can have a few extra includes, for heavy duty features that are often not used. For example to use almost all the feature of windex, include windex.h and you get them all. But if you want a property grid or a plot then you do need extra includes for those.

[alecmus]

Libraries are different because there is no one-size-fits-all. It now appears like all you are really trying to do is draw attention to your own library. I would never talk about my library on your forum.

Using words like "annoying" just because the library is different from yours really doesnt help. It's just a different architecture it doesnt have to annoy you. This sort of library and what it does could never really work in a single header file. Qt, wxWidgets, nana etc arent single header for a reason and just because they use a different architecture doesnt make them terrible. Kindly avoid talking about your own library on other forums.

[alecmus]

There is always a way of making constructive suggestions without talking other people's work down.

Kindly consider how you put your words across. Your suggestions are always welcome but it doesnt help when it comes as an attack.

[alecmus]

This issue was addressed in 156f37b and the example code was updated accordingly in relevant sections. Thank you for the suggestion.

For example, the following code:

auto& home = _page_man.add("home");

auto& label = widgets::label::add(home);
label
    .text("<em>Progress indicator</em>")
    .color_text(color().red(0).green(150).blue(0))
    .rect(rect()
        .left(10.f)
        .right(home.size().get_width() - 10.f)
        .top(10.f)
        .bottom(30.f))
    .center_h(true);

auto& indicator = widgets::progress_indicator::add(home);
indicator
    .percentage(68.f)
    .rect().snap_to(label.rect(), rect::snap_type::bottom, 20.f);

Produces this:

The lines of interest here are the following:

auto& label = widgets::label::add(home);
auto& indicator = widgets::progress_indicator::add(home);
...

We simply reverted to the initial interface from back here 1674eb2 while retaining all the improvements and additions that have been to the library since then.

[JamesBremner]

auto& label = widgets::label::add(home);

This is nice and clean.

The library user constructs a new widget AND adds it to a pane all in one call to a static method of the widget class. There is no widget class constructor.

The gives clean application code.

It is however a very unusual way of doing things. So I think that you should provide a small example application that does this and which explains the procedure for most people who will be expecting to call a constructor.

[JamesBremner]

The docs for the label class method add needs to be updated. It gives no clue that this constructs a new instance of the class and should be used rather then a constructor

[JamesBremner]

Question: why do this in a static method when, I think, it could be done just as easily in a constructor?

The application code would be even cleaner

auto& label = widgets::label(home);

Most importantly, this is the 'normal' way of doing things so you would not have to provide any special documentation, Just say, every widget constructor needs a reference to the page that will contain it.

[alecmus]

This is nice and clean.

Thank you.

I think that you should provide a small example application that does this and which explains the procedure for most people who will be expecting to call a constructor

The wiki pages that are available so far have been updated to reflect these changes. For example, the example code here shows this in action.

The docs for the label class method add needs to be updated

The in-code XML documentation had already been updated in e3e2995 and we have just pushed the changes over to the XML documentation in 8670e9b as suggested.

Question: why do this in a static method when, I think, it could be done just as easily in a constructor?

So that the user doesn't have to worry about managing the lifetime of the object. Adding a widget to a container through a constructor would mean the user has to keep the class variable of the widget within scope. By relieving the user from having to manage the widget lifetime themselves this results in less class variables, which is neater. Furthermore, if a widget could be made as a class variable it wouldn't be possible to delete it later using widget_manager.close() as destroying it would require to somehow bring the object out of scope. The alternative would be to use pointers, which we avoided to minimize programming mistakes. As such, the library is responsible for managing the lifetime of the object. The user can get a reference to the object at any time, anywhere within the class by calling the .get() method (or using the helper macros provided for more terse code, e.g. get_label(), and if the widget has been destroyed (either explicitly or because its container was closed), then the .get() method throws an exception, as documented in the respective .get() methods.

Kindly note that this code:

auto& label = widgets::label(home);

Would not be possible since constructors cannot return values. This is another reason why a static method was used.