It seems that actionlistener events are just the string of the name of the component that fired the event. wouldn’t it make more sense for it to pass a poitner to the object itself and either provide a way for querying for its type or just creating a family of different actionlistener interfaces for various patterns?
its hard to do a lot with a string name.
i was wondering the same thing…
Got a good example of a case when you’d really need more than a string?
I see your point, but whenever I’ve thought it needed more info for something I was doing, I always then realised that it was because I was writing sloppy event code. In good design, you use different classes to handle different types of event, not switch statements that do different things based on the type of event…
so the best way to handle this would be to write our own custom listener?
[quote=“jules”]Got a good example of a case when you’d really need more than a string?
I see your point, but whenever I’ve thought it needed more info for something I was doing, I always then realised that it was because I was writing sloppy event code. In good design, you use different classes to handle different types of event, not switch statements that do different things based on the type of event…[/quote]
I appreciate the thought behind what you’re doing, but using the string as that sole piece of information has a number of problems. One is that if you were to localize an application, the string will probably be different in different cultures and or languages.
Another reason is that it doesn’t make a lot of sense to pass only the string because if I am handling the same event for n instances of the same widget, you are telling me the name of the widget, but forcing my event handler have first hand knowledge of how to get that widget (storing its own pointer to it and querying for it each time based on the string). It may be the case (and often is) that the event handler does not own these widgets, and needs to discover them dynamicaly. Since you are passing the string already, its clear the underlying event handling plumbing has the pointer, so why not just pass the whole pointer to the type of widget or the type of event.
The way the .NET Framework does it in my opinion is very well done. There are various event handler signatures (delegates, but we can use interfaces for C++). So you can have a heterogeneous collection of widgets which all invoke the same pattern event, and have them all handled by the same event handler.
An example is toolbar buttons and menu items, their event signature is the same for invokation, enabling the developer to only write one handler. If the handler needs to know more about the component itself, it can query the type and then perform actions on it or query more information on its state.
These are just a few off the top of my head.
Sure - I didn’t design it like that to deliberately force people to use strings, and there’s a theoretical argument for adding pointers (like ChangeListener), but it’s interesting that in years of using these things I’ve never actually needed a pointer…
localization wouldn’t really be an issue if you check like this:
if (m == myButton->getName()) {
//…
}
but i do agree that in some situations having a reference to the object would be very useful… (especially when working with custom components)
well here’s an example of one of problems i’ve had. i have a component which contains several child component of the same type. when you click on a button in one of the child components i want to have the parent do something with that component. how would you suggest i implement this? with the current actionlistener system i would have to make sure that each child component has a different name, and then to handle the action i would have to check the name against all the children to see which one it is.
[quote=“jtxx000”]localization wouldn’t really be an issue if you check like this:
if (m == myButton->getName()) {
//…
}
but i do agree that in some situations having a reference to the object would be very useful… (especially when working with custom components)[/quote]
I am not following you. The problem as I see it is that you are comparing the name of the button (locale/culture specific) to a value determined in code, right? So when you are coding which value do you use? The one you probably speak, english in our case. What happens when you change the text for the button?
I just think that pivoting off of a culture specific variable for code logic is just a bad practice. The purpose of a text string in this case is for display, and by doing this you are going to have worlds coliding so to speak, between code logic and display information.
[quote=“jtxx000”]localization wouldn’t really be an issue if you check like this:
if (m == myButton->getName()) {
//…
}
but i do agree that in some situations having a reference to the object would be very useful… (especially when working with custom components)[/quote]
Well I don’t want to start a design war, obviously you have done a tramendous job with JUCE. At the same time, its possible the GUI domains you’ve written apps for just never needed this capability.
Having worked at MS during the start to finish phase of the 1.0 and 1.1 versions of the .NET Framework, I know that a lot of debate and thought went into the event handling patterns, and a lot of really big smartypants helped shape it. It needed to be useful to the widest domain for GUIs possible and I think it would be hindered not to have to have the ability to capture the event origin (sender). Especially when its “free” so to speak, there’s not much point in hiding it.
I’m not 100% sure, but I think the button name and its text are 2 different things in Juce (setButtonText to set its text, and setName for the component name).
So, the above code seem ok to me concerning language issues.
Anyway, I think, for performance reasons, the string approach is not very good as it requires a string comparison for every event n (which can be called 1000+ time) and every component m (it is a O(m * n) algorithm). A pointer is not good either (as you will still have to perform a O(n * m) search).
For me the best (quickliest) solution, is to use an index (or a hash), so you could downsize the code to a switch() (with is O(m) when optimized with increasing indexes) statement. You can add this functionnality by deriving every component with a getHash or getIndex method.
Another approach (and I think it’s the best around), would be to store the child component pointers and their names in a binary map (stored as a binary tree with the name (or index) as the key instead of the current array). This could even preserve current code, as it could be added in the main component class. So, you will then just call a getChildComponentFromKey(the_key_type_either_int_or_string)
to get a component pointer with a O(n*log(m)) with string or O(n) with int.
Then your code would simply be a
MyComponent * menuItem = dynamic_cast<MyComponent*>(getChildComponentFromKey(key));
if (menuItem) // Do whatever you want with menu itemThis is require no (or few) modifications, while changing the ActionListener for pointers, would mean changing them everywhere in JUCE.
It is still possible to add a bool actionListenerCallbackForPointer that would do nothing in default implementation and let the user override it for his/her need with few modifications.
With the current implementation, a simple way of preventing 2 child components to share a common name would be to override the addAndMakeVisible method (and then check for already named component), or use my dialog editor/creator, so you are sure the code will never generate colliding names.
I don’t want to flame Microsoft but I’m sure that having too much information about an event is not developper friendly, as it often lead to complexity (reinventing the wheel) while there is already a simple way for doing the same stuff.
To summarize, I agree with Jules about the current implementation. Even if it is not the best, it is very easy to understand, the code is easier to maintain (because of its simplicity) and, most of time is enough for user experience (bottleneck is rarely GUI).
Hope it helps.
[quote=“X-Ryl669”]A pointer is not good either (as you will still have to perform a O(n * m) search).
[/quote]
I’m not following you. Why do you need to search anything with a pointer? With the pointer to the component that fired the event, you can perform whatever actions you want on that event through the pointer. There is no searching involved.
But to your point about the text and name being different, that is good to know, I was unclear on that.
[quote=“X-Ryl669”]
I don’t want to flame Microsoft but I’m sure that having too much information about an event is not developper friendly, as it often lead to complexity (reinventing the wheel) while there is already a simple way for doing the same stuff. [/quote]
I guess I’m not communicating this very well. There is not “too much information”, there’s just a reference/pointer to the component that fired the event (“sender”), and a reference/pointer to the event, i.e. MenuEvent me.
That’s it. If you want to work off the name of the component, feel free to use (menu)sender->getName(), or whatever.
I don’t understand why this pattern is being perceived amongst the participants of this thread as being too much information? You simply use what you want.
The problem with the current pattern is that in cases where the event handler has no knolwedge of the components, and wants to “affect” the component that fired the event, how is it supposed to do this? It has to now keep its own set of pointers to all these components, which now needs to be syncronized.
The pattern I am suggesting gives you the current functionality and expands it to enable a looser coupling of code.
You are right, the pointer allow more functionnalities while it makes the code more opaque some time.
For example let’s say you have an array of buttons (OK APPLY HELP CANCEL)
Most people will have then a buttonArray[4] declared to store the pointers.
Then the code with pointers will be :
if (sender == buttonArray[0])
{ // Code for OK
} else
if (sender == buttonArray[1])
{ // Code for CANCEL
} else
if (sender == buttonArray[2])
{ // Code for APPLY
} else
if (sender == buttonArray[3])
{ // Code for HELP
} else
{ // Error
}while with string it will look like (same convention) :
if (sender == "OK")
{ // Code for OK
} else
if (sender == "CANCEL")
{ // Code for CANCEL
} etc...I think the second option it is more obvious than the first (especially if you remove the comments, like many (bad?) programmers do)
With an ID (index key), it could become :
switch(sender)
{
case idOK:
{ // Code for OK
} break;
case idCANCEL:
{ // Code for CANCEL
} break;
etc...
};It is still easy to write & read, and at the same time, it is quicklier (because it is now a O(1) approach (switch are optimized to a call array)) while the pointer approach is still O(n).
Of course, this requires declaring idOK, idCANCEL, etc (maybe as an enum) and use them as keys.
If you still need a pointer (but then I don’t know why) you will have it in the case statement (“OK” is buttonArray[0], etc…)
For different components, the idea is the same.
Most of time, a pointer confuse the developper because:
A reference lead to the same issues.
The current implementation will fit in 99% of all cases. I’m sure it doesn’t worth being changed.
But anyway, you could simply modify ActionListener implementation (very easy) to add a virtual method called actionListenerCallbackPtr which will be called before actionListenerCallback (but with the component pointer). If that method return false (like would do the default implementation), you let ActionListener call the (usual) actionListenerCallback. Else, you just don’t call it at all. Then, you have what you’re asking for.
Ok, your example helped me understand that I am not communicating things right. Here is the problem I am having with using the string, based on your example:
if (sender == "OK")
{ // Code for OK
// Here I want to set the OK button to be a red color (arbitrary example, but you get the point. How can I do that? I don't know where this button is do I?
} else
if (sender == "CANCEL")
{ // Code for CANCEL
}
But here I can do that:
if (sender->getName() == "OK")
{ // Code for OK
sender.setColor(green)
} else
if (sender->getName() == "CANCEL")
{ // Code for CANCEL
sender.setColor(red)
} See what I mean? In this generic handler using string, I would have to keep a list or collection of pointers that a duplicate of the real thing, or have knowledge of where the real collection is in order to modify that component.
This is easy when I have a pointer to sender because I can effect that component at will without having to know anything about who created it and where it resides. That is the power of having a pointer of sender.
I hope this clears things up.
Okay, I understand your point. In that case, it’s clearly quicklier to have the pointer. However, as the method is almost generic, it would use a Component * parameters, and not all component share the same interface (like setColor), so in reality your code will look like :
TextButton * converted_sender = dynamic_cast<TextButton*>(sender);
converted_sender->setColor();
Then, isn’t it easier to directly use the correct object once you’ve identified it (like this) :
if (sender->getName() == “OK”) buttonArray[0]->setColor();
(And then, there is no need to have a pointer here).
[quote=“X-Ryl669”]Okay, I understand your point. In that case, it’s clearly quicklier to have the pointer. However, as the method is almost generic, it would use a Component * parameters, and not all component share the same interface (like setColor), so in reality your code will look like :
TextButton * converted_sender = dynamic_cast<TextButton*>(sender);
converted_sender->setColor();
Then, isn’t it easier to directly use the correct object once you’ve identified it (like this) :
if (sender->getName() == “OK”) buttonArray[0]->setColor();
(And then, there is no need to have a pointer here).[/quote]
In cases where the handler simply has no possible knowledge of where to find the component, then a cast is not a big deal in my opinion. Meanwhile, since its typed as Component*, you can continue to get its name and pivot off of that, which is what is essentialy done now.
Basically, passing the pointer offers the most flexibility at absolutely not cost. You only pay for what you use, which is in keeping with the C++ mantra. By hiding this from you, the library becomes limited to a very specific type of handling pattern.
As it stands now, my generic handlers cannot be generic, they need to have knowledge of all sorts of collections of components that should be none of their business.
[quote=“X-Ryl669”]Okay, I understand your point. In that case, it’s clearly quicklier to have the pointer. However, as the method is almost generic, it would use a Component * parameters, and not all component share the same interface (like setColor), so in reality your code will look like :
TextButton * converted_sender = dynamic_cast<TextButton*>(sender);
converted_sender->setColor();
Then, isn’t it easier to directly use the correct object once you’ve identified it (like this) :
if (sender->getName() == “OK”) buttonArray[0]->setColor();
(And then, there is no need to have a pointer here).[/quote]
what if you have 100 buttons? you would have to search through all of them, which would be inefficient… a pointer would be a lot cleaner in this case.
I think I wasn’t clear enough. I think you are both right, a pointer make handling much more simple.
If you read my previous posts, I was saying that the string solution make the code easier to write (less code to write), and is still easy to read. However, this implies searching which is a pain, and limit the possible action you can do without a lot of code.
The pointer solution is less easier to write (as you should check the pointer first, then cast it to the right type), but is easier to deal with. Pointer doesn’t prevent you from searching, as long as you have different component types (which is the case 99 out of 100 times), because if you just cast the pointer to a button while it is a text editor, it is going to crash as soon as you use it (or you will have to check if the cast worked, and then, it is exactly like the string approach).
Ok, searching is a lot faster because it is using integer instead of strings (of course, if you use a dynamic_cast then, the compiler generate RTTI code to check the conversion and then you directly loose the gain you expected).
The solution I am advertising is using an ID so you will still be able to keep the code readable and easy to write (with a correct enum), it is possible to check for ID ranges (so this solves the different component type issue, if you want to apply the same code to all your button, simple put a if (ID >= firstButtonID && ID <= lastButtonID).
With a switch, the search could be O(1) (instead of O(n)).
Then, I agree that no solution is better than another. If your handler has no possible knowledge of the component, like “addAndMakeVisible(new textButton(…)) in the constructor”, then the pointer is the only solution that would work. At the same time, it is not with “no cost” like you are saying, because you have to cast it (which could lead to RTTI code if you use dynamic_cast or crash if you don’t), you have to take care of the usual pointer issues (if I delete it, it is going to crash when leaving the method), add documentation for the pointer issues in order to avoid them, all of these being a pain when maintaining such code.
A good solution, like I said earlier, would be to have both system (default handler with string (or better, ID), only called if you don’t override the non default handler with pointers).
Even better, the component could use a template argument for switching from both cases at compile time at no cost.
Angrycat, I don’t understand your example. Could you show me a simple code so I could see what you mean ?
Hmm.
I think you might both be missing the point here…
In an example like you’re talking about where clicking a button does something to that particular button, it’d be crazy to use a listener at all - a special class of button that handles its own click event would be a much better, more OO solution.
