Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Signals #112

Closed
wants to merge 1 commit into from
Closed

Signals #112

wants to merge 1 commit into from

Conversation

@1aam2am1
Copy link
Contributor

1aam2am1 commented Oct 3, 2019

Simple Signal Manager implementation that enables you to write your own manager.
Now you can set connect value to the widget in textform.
Widget will be send to your manager with this value.
You can now connect your widget from textform to enable any usability that you desire.

@texus

This comment has been minimized.

Copy link
Owner

texus commented Oct 4, 2019

I don't fully understand how this would be used, how do you e.g. connect the "pressed" event of a certain button? Based on what I can see in the code, you would have to check if signalName matches e.g. "BtnName" inside MySignalManager::connect and then call widget->connect("pressed", []{}); inside that function. But is that really any easier than just having gui.get("BtnName")->connect("pressed", []{}); in your code?

There will have to be some kind of SignalManager class in the final implementation in order to load signals from widget files, but unless I'm missing something I'm not convinced that this implementation really makes things better than they currently are.

@1aam2am1

This comment has been minimized.

Copy link
Contributor Author

1aam2am1 commented Oct 4, 2019

Well i wrote this part of code for my own use.
I used it like that:

  • I wrote class that changed connect function to connect passed widget to my value in my program.
  • Then if this was button i connected it like this: if value is 0 text is black, otherwise red, click changes value.
  • I wrote something like that to all widgets i use in form.
  • In form i simply set to widgets to what value they must be connected.

Doing it like that, i don't need iterate through all widgets to connect them.
I can change form delete some widget and my program wont throw because it couldn't find some widget with that name.

Although now when i write this, it could be better idea to simply iterate through all widgets.

@texus

This comment has been minimized.

Copy link
Owner

texus commented Oct 4, 2019

I think it would be much more useful if it had an API like SignalManager::connect("BtnName", "pressed", []{}).
That way you can bind a callback from any place in your code (as the place where you call gui.loadWidgetsFromFile or inside some MyCustomSignalManager::connect function you can't access private member functions in other classes). Plus it would have the benefit of not needing a custom class implementation which adds complexity.

I haven't previously though about connecting callbacks on widgets that don't exist yet. That is indeed a benefit of having a SignalManager class. When properly implemented, the SignalManager::connect like interface might be able to connect callbacks for forms that haven't even been loaded yet. That will give the ability to connect all callbacks for all widgets that will ever be created by the program during startup.

I don't know how the API should look like though, SignalManager::connect is just one option, it could also be some function in the Gui class. A separate SignalManager class might be better if there was going to be more than one function, e.g. when you also need a remove function (although I don't see any reason to have a remove function).

@1aam2am1

This comment has been minimized.

Copy link
Contributor Author

1aam2am1 commented Oct 4, 2019

It is a good idea, but what if "BtnName" is not button, but slider and he hasn't "pressed" signal at all.

I used my class by declaring in my program what values are available, and by widget type and "connect value" connected widgets to these values.

And even auto loading form by signal of panel resizing.
In form i placed panel with auto layout (auto size) and in connect i wrote "form: name.form"
Then my program searched form with name "name*.form" if finded multiple of these like:

  • name100x200.form
  • name200x200.form
  • name(width)x(size).form

Connected this panel to resize signal and loaded form with the biggest size that could be located inside.

In what you suggest it is impossible, because program need to know names of widget before, even connecting to signal.

Also in API like SignalManager::connect("BtnName", "pressed", []{}). are some problems like:

  • Multiple gui (in which gui we should search name).
  • If connected search further (unique names) or not.
  • Tgui don't have mechanism of searching widgets in specified widget by one call.
    Like: container.gettgui::Container("name").gettgui::Container("nextname").get("Widget_that_i_want");
    To: container.get("name.nextname.Widget_that_i_want");
@texus

This comment has been minimized.

Copy link
Owner

texus commented Oct 4, 2019

Multiple gui (in which gui we should search name).

You wouldn't have to search any gui objects, when a widget is added to its parent the global signal manager is accessed directly.

If connected search further (unique names) or not.

I can imagine that it could be useful sometimes to use the same callback function for different widgets. However, what I had in mind was similar to creating a unique function for each event. For example, the pressed event of the button would have "BtnName_Pressed" as ID and you would need to call connect("BtnName_Pressed", ...) to subscribe to that event. But instead of a unique string I just separated the widget name and signal name as two parameters in my previous examples.

Tgui don't have mechanism of searching widgets in specified widget by one call.

Maybe I'll add something like this in the future, but the get function supports recursive search, so if your name is unique then you can already just do root_container.get("Widget_that_i_want");

but what if "BtnName" is not button

This could also be said about using the get function or using the connect function with a wrong parameter. Any solution will actually have this problem. In your code you just moved this into the user code. Your SignalManager::connect function also only takes a string id and a Widget as parameter which means it is up to the user to make sure the widget is of the right type.
Although it is true that with your implementation has the advantage that the issue could still be detected by checking the widget type, I would consider it a bug in the user code if the type didn't match.

In what you suggest it is impossible

This is true.
I don't think either of our designs are bad, they just aim at different users for different use cases. The ideas that I had about loading signals from widget files were aimed to keep it simple. My ideas come from something like Visual Basic where a BtnName_Click function would be called when the button is pressed. Your design on the other hand is chosen to be extensible and be much more powerful, but being more complex.

I haven't chosen any design yet, when I talk about "my design" I'm just referring to the rough design that I though about so far but never worked out in detail. I do however expect the final implementation to be something similar to what I have thought about, i.e. something simple.

You are correct that with my design it is impossible to do what you want. If you could come up with a design that is still simple (and thus closer to what I had in mind) while still having the ability to be extended (by those who choose to) then I definitely would consider it for inclusion in TGUI.

@1aam2am1

This comment has been minimized.

Copy link
Contributor Author

1aam2am1 commented Oct 5, 2019

You wouldn't have to search any gui objects, when a widget is added to its parent the global signal manager is accessed directly.

That's only if widget was loaded after signal, but what if situation was reversed and signal was added after the widget to enable some situation. Gui isn't unique therefore in which gui should we search, or should it be marked that it don't work in that way.

I will write it in the way that you spoke about,
but what do you think of Widget member ("user_data") to be passed as well from form as string to enable my complex functionality.
And also configuration of form in panel to autoload textform from form and enable user change this functionality in some way in theirs program.

@texus

This comment has been minimized.

Copy link
Owner

texus commented Oct 5, 2019

but what if situation was reversed and signal was added after the widget

You could store a map of widgets names to Widget* inside the signal manager, so that it can quickly check if a widget with that name was already added. When a widget is added to its parent then it is added to the list (and a check is performed to see if a callback should be connected for this widget), when it is removed from its parent then it is removed from the list. So it looks like all this logic could be put inside Widget::setParent. Note that the map must use Widget* and not Widget::Ptr or it will cause a big memory leak in some cases.

but what do you think of Widget member ("user_data") to be passed as well from form as string

I think it is a good idea to add this.
You would have to decide whether it should be stored as an std::string or an sf::String. I guess sf::String would be better as it will allow the user to use unicode characters.

Btw, the member should probably be called "UserData" in the textform instead of "user_data", to conform with the others properties. In the code I also noticed you were using lower_case names for variables instead of camelCase like is being used in the rest of the code.
If you contribute something like this then I'm already happy enough that you contributed it and I won't mind that much to change it, but it would save me some effort if you could already write it with the right case.

And also configuration of form in panel to autoload textform from form and enable user change this functionality in some way in theirs program.

Could you elaborate on this? I didn't really understand what you mean.

@1aam2am1

This comment has been minimized.

Copy link
Contributor Author

1aam2am1 commented Oct 6, 2019

Form => file loaded by loadWidgetsFromFile

I mean to add some way to load another form by panel from form.

In example.
I create form with some panel in it. But I don't want to specify what in this panel should be. I could set some (form property) that will load form itself (its own content) and change somehow in program how should it load.
I could set right.txt and it should load in example right200x200.txt if that file exist and size of the panel is bigger than 200x200 but it should be changeable as user and not dictated by library.

PS. I now funded the userdata member in widget. If it should be saved to form the what should I do. Another function to get/set (only form string "userdata") or save only if convertible to sf::String or std::string

@texus

This comment has been minimized.

Copy link
Owner

texus commented Oct 6, 2019

I'm not sure if what you describe should really be handled by the library. The loadWidgetsFromFile exists to load widgets at one moment in time. Even if it would contain the necessary logic to load right200x200.txt, the code isn't executed again when you resize your windows so it won't magically replace your widgets with something from another file. Loading different widgets when resizing will always have to be implemented on a higher level, the loadWidgetsFromFile will just load widgets how they are described in the file and panels won't store that they were loaded from such a file.

What does seem like a good idea is referencing other files. You would create a panel in one form and the contents of that panel would be stored in a different form. But that relationship would be static, if the first form contains a reference to "right.txt", then it will request to load a file called "right.txt" and nothing else. You could do this via a user defined function (which would do the actual loading so that you could e.g. load the file from a different path or from a zip file). That would still allow you to load "right200x200.txt" at that moment but the widgets won't be changed afterwards.

Maybe you should just load all panels? You would have a group loaded from "right100x100.txt" and another one loaded from "right200x200.txt" and when resizing the window you just hide one and show the other?

If it should be saved to form the what should I do

I would be fine if it only worked for sf::String, but it would be a nice addition if it would also save the contents if it contained an std::string. So I would check if it is convertible in the save function (tgui::Any has in is() function).

@1aam2am1

This comment has been minimized.

Copy link
Contributor Author

1aam2am1 commented Oct 6, 2019

I mean we have onSizeChange event? We could internally connect thus signal in panel. As panel has form value then it could load form200x200.txt or bigger with size change event and replace it's content. With signaenager widgets would connect to event functions automatically.
We could create ExternalPanel to do that with expected logic in program but also added method to change it from user point.

@texus

This comment has been minimized.

Copy link
Owner

texus commented Oct 7, 2019

I don't fully understand the details of how you are going to do it all, but as long as the logic happens in your own code it is fine.
The internal TGUI code (which would include the SignalManager class itself) should not contain anything special code for your use case. It just loads widgets from a file, potentially calling some of your user-defined callback to handle loading the signals defined in the text file (which can include a SizeChanged signal), and what you do with these widgets afterwards is up to you. And the widgets that have been loaded won't store how they were loaded (there should be no variable indicating that a panel was created manually or loaded from a text file).

@1aam2am1 1aam2am1 force-pushed the 1aam2am1:signals branch from 085d343 to 0ec53c1 Oct 8, 2019
@1aam2am1

This comment has been minimized.

Copy link
Contributor Author

1aam2am1 commented Oct 8, 2019

New idea for signalManager but it isn't working because I don't understand how to connect and hold unconnected function in signalManager.

@1aam2am1 1aam2am1 force-pushed the 1aam2am1:signals branch from 0ec53c1 to b50d606 Oct 8, 2019
@1aam2am1 1aam2am1 force-pushed the 1aam2am1:signals branch from b50d606 to 28e7db2 Jan 9, 2020
@1aam2am1 1aam2am1 closed this Jan 10, 2020
@1aam2am1

This comment has been minimized.

Copy link
Contributor Author

1aam2am1 commented Jan 10, 2020

Reimplemented implementation.
Added tests.

@1aam2am1 1aam2am1 reopened this Jan 10, 2020
@1aam2am1 1aam2am1 force-pushed the 1aam2am1:signals branch from 28e7db2 to b4ff47b Jan 10, 2020
Copy link
Owner

texus left a comment

I spend less time than expected on the other stuff that I was working on so I already found the time to review this today. I haven't actually tested the implementation yet, but it will probably be fine based on what I see in the test you included.

Thanks for the effort you have already put into this. I'll try to test it soon so that this can be merged once the minor changes are made.

/cmake-build-debug
/cmake-build-release
/.idea
Comment on lines +16 to +18

This comment has been minimized.

Copy link
@texus

texus Jan 11, 2020

Owner

"cmake-build-debug" and "cmake-build-release" shouldn't be in .gitignore since they are specific to your build. The .idea can remain in .gitignore as this might benefit more people.

This comment has been minimized.

Copy link
@1aam2am1

1aam2am1 Jan 11, 2020

Author Contributor

I use clion ide and it copies files in this folders. GitHub is olweys chalking the folders if it could add then to commit and so on. Therefore in gitignore I ignored builds folder for clion for better stability and working. It shouldn't do any bad think to anyone through because I doubt tgui would use them.

This comment has been minimized.

Copy link
@texus

texus Jan 11, 2020

Owner

I didn't realize that the ide created those debug and release folders too, I though you created those and the ide only created the .idea folder.
Since those folder names seem to be the default setting for clion, you can keep these folders in .gitignore too.

- Added UserData property to widgets
- Changed method of storing names of widgets
- Fixed adding the same widget to multiple containers
- Added possibility to connect signals before creating widget
Comment on lines 10 to 13

This comment has been minimized.

Copy link
@texus

texus Jan 11, 2020

Owner

You don't need to change the changelog, I will take care of it later. The "Added UserData property to widgets" isn't really accurate as it already existed for a while, it just couldn't be saved/loaded.

This comment has been minimized.

Copy link
@1aam2am1

1aam2am1 Jan 11, 2020

Author Contributor

Delete this commit?

This comment has been minimized.

Copy link
@texus

texus Jan 11, 2020

Owner

Yeah, the changes to this file should be removed.

/////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
/// @brief Returns a list of the names of all the widgets in this container
///
/// @return Vector of all widget names
///
/////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
const std::vector<sf::String>& getWidgetNames() const
TGUI_DEPRECATED("Use for(getWidgets())->getWidgetName() instead") const std::vector<sf::String> getWidgetNames() const

This comment has been minimized.

Copy link
@texus

texus Jan 11, 2020

Owner

for(getWidgets())->getWidgetName() might not be very clear. Maybe something like Use getWidgets() and Widget::getWidgetName instead as description?

/////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
/// @brief Returns a list of the names of all the widgets
///
/// @return Vector of all widget names
///
/////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
const std::vector<sf::String>& getWidgetNames() const;

TGUI_DEPRECATED("Use for(getWidgets())->getWidgetName() instead") const std::vector<sf::String> getWidgetNames() const;

This comment has been minimized.

Copy link
@texus

texus Jan 11, 2020

Owner

Same comment as before, TGUI_DEPRECATED description could be more clear.

TGUI_API bool isWhitespace(char32_t character);
TGUI_API bool isWhitespace(uint32_t character);
Comment on lines -50 to +50

This comment has been minimized.

Copy link
@texus

texus Jan 11, 2020

Owner

TGUI_NEXT code shouldn't really be touched. That code was added for myself as a reference where things should change in 0.9. Originally the code did compile, but I guess that by now you should probably never define it.

This comment has been minimized.

Copy link
@1aam2am1

1aam2am1 Jan 11, 2020

Author Contributor

Well I declared TGUI_NEXT and checked if it would copile if I deleted deprecated code. But it didn't because in my gcc char and char32 were the same and I have thus some problem with that.

This comment has been minimized.

Copy link
@texus

texus Jan 11, 2020

Owner

TGUI_NEXT is an undocumented define that should actually never be used (and as you noticed doesn't even compile). I appreciate that you tried to check with different defines though.
It actually doesn't matter what the code says, it won't be compiled anyway. So you don't have to bother undoing your change, we can keep the "uint32_t" version.

@@ -428,7 +428,7 @@ namespace tgui
///
/// Examples:
/// @code
/// widget->setUserData("Data");
/// widget->setUserData("Data"); ///Warning: const char*

This comment has been minimized.

Copy link
@texus

texus Jan 11, 2020

Owner

The warning is a good idea. But maybe something slightly more verbose like // Note: type to retrieve with getUserData is 'const char*' here?

/////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
/// @brief Returns the name of a widget
///
/// @return Name of the widget or an empty string when the widget didn't exist or wasn't given a name

This comment has been minimized.

Copy link
@texus

texus Jan 11, 2020

Owner

Copy-paste issue: the widget always exists if this function can be called. So it's only empty if it wasn't given a name

bool isWhitespace(char32_t character)
bool isWhitespace(uint32_t character)
Comment on lines -49 to +49

This comment has been minimized.

Copy link
@texus

texus Jan 11, 2020

Owner

Shouldn't be changed

This comment has been minimized.

Copy link
@1aam2am1

1aam2am1 Jan 11, 2020

Author Contributor

Error as described before. On compile time.

@@ -503,7 +503,7 @@ TEST_CASE("[Sprite]")

SECTION("getTexture has a version to change the texture and a const version")
{
sprite.getTexture().setSmooth(false);
//sprite.getTexture().setSmooth(false);

This comment has been minimized.

Copy link
@texus

texus Jan 11, 2020

Owner

Was this commented for a reason?

This comment has been minimized.

Copy link
@1aam2am1

1aam2am1 Jan 11, 2020

Author Contributor

TGUI_NEXT deleted procedure that could return non const value and build was impossible in deprecated code removal.

This comment has been minimized.

Copy link
@texus

texus Jan 11, 2020

Owner

Removing deprecated code isn't documented so its normal that you didn't know, but to remove deprecated code you should define TGUI_REMOVE_DEPRECATED_CODE. The TGUI_NEXT define had a different goal (and it is no longer relevant). Once I've merged your changes I'm going to remove the TGUI_NEXT stuff to avoid further confusion.

So this line should be uncommented.

@1aam2am1

This comment has been minimized.

Copy link
Contributor Author

1aam2am1 commented Jan 11, 2020

I will change this and when path will be ready squash all commits.

@texus
texus approved these changes Jan 12, 2020
@texus

This comment has been minimized.

Copy link
Owner

texus commented Jan 12, 2020

You can squash the commits.
I'll try to merge the PR in the next few days.

@1aam2am1 1aam2am1 force-pushed the 1aam2am1:signals branch 2 times, most recently from 72caf05 to 8d6815d Jan 12, 2020
@1aam2am1

This comment has been minimized.

Copy link
Contributor Author

1aam2am1 commented Jan 12, 2020

Some errors when squashing.

@1aam2am1 1aam2am1 force-pushed the 1aam2am1:signals branch from 8d6815d to 5db158a Jan 12, 2020
@1aam2am1

This comment has been minimized.

Copy link
Contributor Author

1aam2am1 commented Jan 12, 2020

There were some errors. Name of widget was changing when added to container as add(widget, name) and tests wont pass. This should be changed to add(widget) in all widgets but there is to much dependency as add is virtual method. It should be changed in next major version.

Fixed adding the same widget to multiple containers
Added possibility to connect signals before creating widget
Update .gitignore
Now UserData property will be saved as string if possible
@texus

This comment has been minimized.

Copy link
Owner

texus commented Jan 12, 2020

Why were there errors? The code before you squashed it worked fine, so what did you change?
And what did you change now to make it work again?

@texus

This comment has been minimized.

Copy link
Owner

texus commented Jan 12, 2020

I think I know what you mean. I didn't notice that you were using it like that, but setWidgetName should be private and the user should never call it. In 0.9 we could indeed change the design so that the add function no longer takes the name as parameter and where the user should set a name via setWidgetName, but for 0.8 the code should continue to function like before. This means that in 0.8 the name should still only be set via the container.add method, you should only be moving the location where the name is stored. The setWidgetName can be private because Container is a friend class of Widget. The getter function should remain public.

@1aam2am1

This comment has been minimized.

Copy link
Contributor Author

1aam2am1 commented Jan 12, 2020

Well there was error in deleting signals because I didn't disconnected them in remove(Widget*) function. We could let name setting method be like it is but there should be disclaimer that adding it to container will reset it name and name should be added after Adding it or to change name when adding to contner. Adding virtual add(Widget) method is a option on its own. Or changing std::string to optional string then checking if string was settled and only then changing the name.

add(widget, "name") -> setname
add(widget, "") ->setnullname
add(widget)->don't set name

@texus

This comment has been minimized.

Copy link
Owner

texus commented Jan 13, 2020

This has been manually merged in ac2bc92

I've added a warning to setWidgetName that it will be overwritten on parent.add(widget)

@texus texus closed this Jan 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.