TextBox::SetText() & Signals::TextBox::TextChanged

Started by chrishox, 21 April 2020, 01:01:09

chrishox

Hi, again.
I've recently switched one widget from an 'EditBox' to a 'TextBox' but it did not behave as I would have thought:



//// This responded they way I wanted: to both 'setText()' and to keyboard entry
//auto box{ tgui::EditBox::create() };
//box->connect(tgui::Signals::EditBox::TextChanged, []() { /* worked as expected */ });

// This does *not* respond when 'setText()' is called, but does respond to keyboard entry
auto box{ tgui::TextBox::create() };
box->connect(tgui::Signals::TextBox::TextChanged, []() { /* deaf to 'setText()' */ });

// This only seems to send the "TextChanged" signal when 'box' is an EditBox (not a TextBox)
box->setText("hi");



It appears I could be missing something obvious; might someone be able to point me down the proper path?

texus

The behavior is definitely inconsistent, but neither is really wrong.
Whether a widget should send out a signal when it is programmatically changed is debatable. On one hand, having setText send the TextChanged signal allows you to manually set a value which would be processed in the same way as what the user would have typed. On the other hand, if your TextChanged callback checks whether the text is valid or not and calls setText again when invalid, you probably wouldn't want that to trigger another callback.

I've discussed this topic with people in the past with CheckBox and in the end I removed the callback from the setChecked function. So it is possible that if I start looking into this, I would remove the callback from EditBox::setText instead of adding a callback in TextBox::setText.

chrishox

#2
Keeping the lambda, is there a way I can programmatically trigger that callback after calling 'setText()'?
[edit]: This code below works, but surely there's a better way...
  box->setText("hi");
  box->onTextChange.emit(box.get(), box->getText()); // yikes!

Comment:
I agree that consistency is king.
From the standpoint of a user of your library,  I would prefer that widget setters (such as 'setText()') do indeed signal their respective callbacks (if I change the text, programmatically or otherwise, it makes sense 'TextChanged' would be triggered).
I certainly understand your example where calling the callback from within the setter might be less than desirable; however wouldn't you prefer to leave that decision to the user? Perhaps a skip-any-callbacks bool argument for each setter, defaulted (to false?) to preserve the existing interface?
Just my thoughts, though. Thank you for creating & maintaining this helpful library. And thanks for your very prompt replies!

texus

QuotePerhaps a skip-any-callbacks bool argument for each setter
I don't really like this, mostly because of the amount of code that needs to be added in order to do this everywhere.

Quoteis there a way I can programmatically trigger that callback after calling 'setText()'?
This can't really be done properly as the signal often needs to know some extra parameters. The emit function is the correct way to do it, but it definitely isn't something that I would like to recommend.

QuoteI would prefer that widget setters (such as 'setText()') do indeed signal their respective callbacks
I personally also lean towards always sending a callback in a setter as well, it is even simpler to implement (setters can be called internally to not duplicate signal code).
I just checked the code for CheckBox and I noticed that I was wrong about what I said earlier. I thought I had removed the callback in CheckBox, but it was the other way around, it does trigger a callback in setChecked.

Since it turns out we actually want the same thing, I will add the callback to TextBox. Although the chance is small that it would break anyone's code, I should probably add a boolean parameter like you suggested to keep it compatible. Then for 0.9 (which is likely going to arrive very soon) I will remove the bool again and it will always send the callback.

chrishox

QuoteI will add the callback to TextBox
Awesome, thanks!  ;D
This, coupled with 'Signal::setEnabled()', should be able to cover any scenarios I can imagine.

texus

The boolean parameter has been added to TextBox::setText in 0.8 on github.

Now that we are on the subject of signals, maybe you are willing to answer a poll about it? Which way would you prefer to use in TGUI 0.9 to connect signals?
Code (cpp) Select
1. button->connect("pressed", []{});
2. button->connect(tgui::Signals::Button::Pressed, []{});
3. button->getSignal(tgui::Signals::Button::Pressed).connect([]{});
4. button->onPress.connect([]{});
5. button->signalPressed.connect([]{});

And if you have to choose between 4 or 5, which would you prefer?

chrishox

I obviously cannot know some of the deeper Signals implications as well as you; however, from my perspective as a user, either '4' or '5' seem preferable.
I could even like '2' if the first argument was a strongly-typed enum instead of a 'constexpr char *'. (Efficiency aside, the strings in '1' & '2' just don't feel very C++ & that bugs me. Granted, I don't load any parameters from text files.)

If I had to choose between '4' & '5', I suppose I would have a very minor preference for '5', only because it reads well as english in your example.
And, while you're breaking interfaces and I'm being OCD, would you consider changing 'connect()' to 'assign()'?  ;D
5. button->signalPressed.assign(myCallbackFunc);
Or better yet, for '4', you could use an 'operator()(..)' overload in your 'Signal[String?]' struct to make it even more succinct by skipping 'connect()' altogether:
4'. button->onPress(myCallbackFunc);
Ooo; yeah, baby.

Thanks for humoring me.

texus

I'm not so fond of assign. What would the opposite be, "unassign"? It sound a bit like you are completely changing the object as opposed to adding one callback functions to the list of existing ones.

The downside of using operator() is that the way you connect and disconnect isn't symmetric. If the signal was named differently then using operator() would also typically mean transmit the signal instead of connecting to it. It has been suggested a while ago by someone else as well though, and I have no objections to implement an operator() as alternative method which just calls the connect function in its implementation.

Another method that I'm thinking about is by using "+=" and "-=" operator (similar to the C# code for TGUI.Net). Something like this:
Code (cpp) Select
button->signalPressed += myCallbackFunc;