Implement EditBoxSlider

[Schweini07]

Addresses #163.

This is a WIP implementation of a widget that allows the user to modify a numerical value through a slider.

I am unsure on some design choices, programming-wise and aesthetic-wise, which is why I put this PR up as a WIP.

Firstly, the design of the widget itself. I am using an EditBox and a Slider in a class deriving from SubwidgetContainer. I think the result looks okay, but there is definitely room for improvement. The Slider itself is a little small, and I am unsure if the numerical value should be in the middle of the EditBox or to the left. It would be nice to hear another opinion on the design first, before I finish it.

Secondly, the majority of code I was able to just copy and reimplement from SpinControl as it is structured in a very similar way, also being derived from SubwidgetContainer. I only had to undertake small changes to account for the Slider widget being used, and not the SpinButton. This unfortunately creates a lot of the same structures and functions, which I feel could be put into a new parent class. On the other hand though, this might unnecessarily increase complexity and not even be too necessary, so it'd be great to have an opinion on this as well.

Lastly, I am a bit confused on why SpinControl uses move and copy constructors whilst other widgets don't. I have implemented the constructors in EditBoxSlider as well, as I figured they might be necessary here as well (and the save/load test makes use of them, if I am not mistaken). But out of pure interest, why are the other widgets not using these constructors?

Tasks still in need of being completed, aside from the asked design questions:

• Change slider position upon value being entered
• Fix signal tests
• Fix save/load tests
• Work on size/layout
• Integrate into GUI builder
• Check for value when going out of focus
• Let the user set the alignment of the text value

[texus]

The Slider itself is a little small

I'm not going to use the widget myself, so I can't give much valuable aestetic feedback.

I don't think the slider needs to be bigger though.
Should there be a left and right arrow next to the slider? Or should it only be a draggable thumb? If arrows would be useful then you could use a Scrollbar widget (with ViewportSize=1) instead of a Slider.

I am unsure if the numerical value should be in the middle of the EditBox or to the left

This should probably be configurable (with a setTextAlignment function that calls the setAlignment on the edit box). I have no preference for a default value.

This unfortunately creates a lot of the same structures and functions, which I feel could be put into a new parent class. On the other hand though, this might unnecessarily increase complexity and not even be too necessary, so it'd be great to have an opinion on this as well.

For now I would just ignore that a lot of code gets duplicated.

I am a bit confused on why SpinControl uses move and copy constructors whilst other widgets don't

Those constructors are only added when the implicitly-generated constructors aren't sufficient. Ideally they shouldn't be needed anywhere (except for a few places like the Widget and Container base classes).

A widget like EditBox for example has no explicit copy constructor because it only contains objects that can be copied (e.g. strings, sprites, rects, integers, ...). SpinControl on the other hand contains an EditBox::Ptr. The default copy constructor would cause both spin controls to share the same edit box, so an explicit copy constructor is needed to clone the edit box.

I was going to say that we need some kind of CopiedSharedPtr class, but it turns out that I've already created it. You should check if you can replace EditBox::Ptr and Slider::Ptr in your class with CopiedSharedPtr<EditBox> and CopiedSharedPtr<Slider>. Normally the rest of the code can remain the same but then you wouldn't need the manual copy and move constructor and operators anymore. This is also what widgets use to store their scrollbar. If it works then the same can probably be done in SpinControl.

[Schweini07]

Should there be a left and right arrow next to the slider? Or should it only be a draggable thumb? If arrows would be useful then you could use a Scrollbar widget (with ViewportSize=1) instead of a Slider.

I've looked into this right now, but I don't think it is feasible. The scrollbar widget lacks functionality that is needed for the slider. For example, one is not able to provide a minimum value, as it is always 0. Furthermore, the value is an unsigned int, so we wouldn't be able to use negative numbers with the EditBoxSlider. For this to work, I'd have to rework the scrollbar widget into a totally new one, so at the end, just a slider widget should be good enough I think 🙂.

I was going to say that we need some kind of CopiedSharedPtr class, but it turns out that I've already created it. You should check if you can replace EditBox::Ptr and Slider::Ptr in your class with CopiedSharedPtr and CopiedSharedPtr. Normally the rest of the code can remain the same but then you wouldn't need the manual copy and move constructor and operators anymore. This is also what widgets use to store their scrollbar. If it works then the same can probably be done in SpinControl.

Ah, that's fantastic! I'll implement this then 😄. SpinControl should the probably also be stripped of its constructors and have them replaced by the copy pointer, right? Should I do this in this PR as well, or do you just want to make a commit after?

And thanks very much for the feedback! I'll implement the missing functionality now 👍

[texus]

I don't think it is feasible. The scrollbar widget lacks functionality that is needed for the slider. For example, one is not able to provide a minimum value, as it is always 0.

You're right, I didn't think that one through.

I'll implement this then

I've had another look at the code, and you actually can't get rid of the constructors. The situation turns out to be very different to widgets with scrollbars. The child widgets exist in two places: inside the m_container and inside the EditBoxSlider as m_editBox and m_slider. Both of these places need to refer to the same widgets.

The default copy constructor would cause the ones in the container to be duplicated (which is good), but would leave m_editBox and m_slider in the new EditBoxSlider pointing to the widgets from the original EditBoxSlider.

Changing the widgets to use CopiedSharedPtr changes what happens, but still doesn't do what is needed. In that case the default copy constructor would duplicate both the ones in the container and also duplicate the m_editBox and m_slider elements. So you end up with 3 edit boxes: one in the original EditBoxSlider, one in the m_container of the new EditBoxSlider and one in the m_editBox member of the new EditBoxSlider.

So a custom copy constructor MUST be written that lets m_editBox and m_slider point to the widget that was duplicated in the container (which is e.g. done in SpinControl with m_spinText{m_container->get<EditBox>(U"SpinText")} in the copy constructor). Also the callbacks need to be reset (which is done in the init() function), so there really isn't a way to let the compiler automatically generate the right constructors.

[Schweini07]

Also the callbacks need to be reset (which is done in the init() function), so there really isn't a way to let the compiler automatically generate the right constructors.

Ah, that's a shame. But not too bad, I'll just leave the implementation as it is right now. (Also, I just noticed my initial commit didn't even include the constructors, I think I accidentally deleted them 😅. But I've reimplemented them now.)

Another design-choice, were it would be nice to get some feedback on, is the handling of numbers being entered that are out of bounds. I currently set the value as the maximum or the minimum depending on the out of bounds number being below or above the allowed range. SpinControl though resets the number to the last used value. Is it fine if I leave it be, or should there be an unified way?

I've also removed a test inherited from SpinControl where mouse clicks are emulated, as it was too tiring to implement, I hope that's fine.

[Schweini07]

All tasks are completed now 🙂. I'll look over the code tomorrow morning again, I wouldn't wonder if some errors were able to slip trough my tiredness 😅.

[texus]

I currently set the value as the maximum or the minimum depending on the out of bounds number being below or above the allowed range. SpinControl though resets the number to the last used value. Is it fine if I leave it be, or should there be an unified way?

I think it's fine to leave it like it is now. If the slider wouldn't move while typing then I would probably suggest resetting to the last value. Since the slider thumb is already moving anyway, it's ok for it to get stuck at the edges when you type something that is out of bounds.

I've also removed a test inherited from SpinControl where mouse clicks are emulated, as it was too tiring to implement, I hope that's fine.

No problem. Many widgets are actually lacking event and/or drawing tests because they are so much work to implement.
Also, while tests are definitely appreciated, they are never require to have a PR merged, any test case that exists is already considered a bonus.

[Schweini07]

Alright, I'm happy. The PR is now ready to be merged 😄

Although I have one last concern, in SpinControlProperties.hpp and now also EditBoxSliderProperties.hpp in m_textProperties the last property "Borders" is not part of the properties outlined by the buttonRenderer. I am not sure if this is intentional or just a mistake. Should this be removed?

const auto textRenderer = spinControl->getSpinTextSharedRenderer();
pair.second["SpinText.Padding"] = {"Outline", textRenderer->getPadding().toString()};
pair.second["SpinText.CaretWidth"] = {"Float", tgui::String::fromNumber(textRenderer->getCaretWidth())};
pair.second["SpinText.TextColor"] = {"Color", tgui::Serializer::serialize(textRenderer->getTextColor())};
pair.second["SpinText.TextColorDisabled"] = {"Color", tgui::Serializer::serialize(textRenderer->getTextColorDisabled())};
pair.second["SpinText.TextColorFocused"] = {"Color", tgui::Serializer::serialize(textRenderer->getTextColorFocused())};
pair.second["SpinText.SelectedTextColor"] = {"Color", tgui::Serializer::serialize(textRenderer->getSelectedTextColor())};
pair.second["SpinText.SelectedTextBackgroundColor"] = {"Color", tgui::Serializer::serialize(textRenderer->getSelectedTextBackgroundColor())};
pair.second["SpinText.DefaultTextColor"] = {"Color", tgui::Serializer::serialize(textRenderer->getDefaultTextColor())};
pair.second["SpinText.BackgroundColor"] = {"Color", tgui::Serializer::serialize(textRenderer->getBackgroundColor())};
pair.second["SpinText.BackgroundColorHover"] = {"Color", tgui::Serializer::serialize(textRenderer->getBackgroundColorHover())};
pair.second["SpinText.BackgroundColorDisabled"] = {"Color", tgui::Serializer::serialize(textRenderer->getBackgroundColorDisabled())};
pair.second["SpinText.BackgroundColorFocused"] = {"Color", tgui::Serializer::serialize(textRenderer->getBackgroundColorFocused())};
pair.second["SpinText.CaretColor"] = {"Color", tgui::Serializer::serialize(textRenderer->getCaretColor())};
pair.second["SpinText.CaretColorHover"] = {"Color", tgui::Serializer::serialize(textRenderer->getCaretColorHover())};
pair.second["SpinText.CaretColorFocused"] = {"Color", tgui::Serializer::serialize(textRenderer->getCaretColorFocused())};
pair.second["SpinText.BorderColor"] = {"Color", tgui::Serializer::serialize(textRenderer->getBorderColor())};
pair.second["SpinText.BorderColorHover"] = {"Color", tgui::Serializer::serialize(textRenderer->getBorderColorHover())};
pair.second["SpinText.BorderColorDisabled"] = {"Color", tgui::Serializer::serialize(textRenderer->getBorderColorDisabled())};
pair.second["SpinText.BorderColorFocused"] = {"Color", tgui::Serializer::serialize(textRenderer->getBorderColorFocused())};
pair.second["SpinText.Texture"] = {"Texture", tgui::Serializer::serialize(textRenderer->getTexture())};
pair.second["SpinText.TextureHover"] = {"Texture", tgui::Serializer::serialize(textRenderer->getTextureHover())};
pair.second["SpinText.TextureDisabled"] = {"Texture", tgui::Serializer::serialize(textRenderer->getTextureDisabled())};
pair.second["SpinText.TextureFocused"] = {"Texture", tgui::Serializer::serialize(textRenderer->getTextureFocused())};
pair.second["SpinText.TextStyle"] = {"TextStyle", tgui::Serializer::serialize(textRenderer->getTextStyle())};
pair.second["SpinText.DefaultTextStyle"] = {"TextStyle", tgui::Serializer::serialize(textRenderer->getDefaultTextStyle())};
return pair;

const std::set<tgui::String> m_textProperties = {
        "Padding", "CaretWidth", "TextColor", "TextColorDisabled", "TextColorFocused", "SelectedTextColor",
        "SelectedTextBackgroundColor", "DefaultTextColor", "BackgroundColor", "BackgroundColorHover",
        "BackgroundColorDisabled", "BackgroundColorFocused", "CaretColor", "CaretColorHover", "CaretColorFocused",
        "BorderColor", "BorderColorHover", "BorderColorDisabled", "BorderColorFocused", "Texture", "TextureHover",
        "TextureDisabled", "TextureFocused", "TextStyle", "DefaultTextStyle", "Borders"
    };

[texus]

There is one inconsistent behavior that I noticed when testing the widget. Suppose the range is 0-10 and the value is 4. Typing a 0 after the 4 will cause the text to change to "10". However, typing another 0 at that point will allow the text to become "100".

Personally I don't like that the text changes while typing (I would have preferred the text to be "40" while the internal value is 10), but if the text were to remain "10" while typing additional characters then at least it would be more consistent.

So maybe you can have a look if this is something that could be easily improved. If it's hard to do then it can be kept like it is though, it's only a minor detail.

the last property "Borders" is not part of the properties outlined by the buttonRenderer. I am not sure if this is intentional or just a mistake. Should this be removed?

From what I can tell the entire m_textProperties (and m_buttonProperties) property can be removed. I can't find any place where it is used. I believe it's code that remained from before properties where prefixed with SpinButton. and SpinText.. You can remove it as part of this PR.

Shouldn't the sliderRenderer properties in gui-builder/include/EditBoxSliderProperties.hpp have a "Slider." prefix? (based on what I'm seeing in the SpinControlProperties.hpp file)

[Schweini07]

So maybe you can have a look if this is something that could be easily improved. If it's hard to do then it can be kept like it is though, it's only a minor detail.

I just improved this behaviour now, by having it be more aligned with how it works with SpinControl. So if you go from 4 to 40, the value stays at 4, but it doesn't go to 10 as it would before. This seemed like the easiest solution to me 🙂.

The rest of your feedback is also implemented now! Unfortunately, it seems the change to use the setValue member function of EditBoxSlider instead of the one from Slider is causing a SEGFAULT when running the tests. I'll look into it tomorrow evening, when I am back home. I am currently using my laptop whilst being away for the easter weekend, and it's a chore to do anything on it, everything's incredibly slow...

[Schweini07]

Unfortunately, the bug seems a bit harder to fix than anticipated. The SEGFAULT happens in the setString function when the value is put into the ostringstream. To be honest, I am completely stumped to what the reason for this could be. I'll look at it tomorrow.

[Schweini07]

Alright, it's fixed now 🙂. I am still not quite sure, but I assume that the segfault occurred because of a recursive function call. In the code before the fix, on a text change setValue of EditBoxSlider would be called. Here setString would be called to change the text of the EditBox, and therefore the onTextChanged signal would be emitted, with the consequences of setValue being called again, and so on and forth again. Now I just set the slider value as before, with a check to if the value is in range, and the same behaviour is achieved.
It just shows how much better a well-rested mind is at solving a task, than a tired one, I only had to spent 5 minutes now on figuring out the problem compared to the 2 hours I invested yesterday 😅.

[texus]

The behavior when typing invalid values is a lot better now 👍
Thanks for taking the time to create the widget.