this mail starts by explaining why I chose to make Ekiga::LiveObject have a single populate_menu taking an Ekiga::MenuBuilder method, instead of having the action system as now found in lib/engine/action, and comments (and questions and ideas) on that new system.

When I wrote the menu builder code, one of the things I wanted was make sure that providing actions would be *simple*, and indeed :

1. if you only need to add a few actions, it is mostly trivial (look at evolution-book.cpp's populate_menu : for a single action, there are very few lines!) ;

2. if you don't provide the actions yourself, it is also easy to accumulate the actions of various sources : just pass the builder around.

This is why the live object pushes what can be done (calls add_action, etc on the menu builder), rather than the gui pulling it (calls some get_action). That way most of the api is in Ekiga::MenuBuilder (which has few implementations, but rich ones) and not Ekiga::LiveObject (which has many, and shouldn't be rich : when you write an evolution book, you want to spend your time writing how to interact with it, not writing boilerplate to create objects).

Another goal was to make it easy to extend ; the first version of Ekiga::MenuBuilder had only the add_action method : add_separator and add_ghost were added later. And adding them was rather trivial! I just added them to the base class, which of course directly broke the MenuBuilderGtk class (the compiler complained and made a todo-list)... but *nothing else*. Again, because the api was in Ekiga::MenuBuilder and not Ekiga::LiveObject.

I could have gone with a framework similar to other parts of the engine with an abstract Ekiga::Actionnable and an Ekiga::ActionnableImpl duality, which would have had quite nice properties too (with respect to using the compiler to detect problems, for exemple -- see my other mail), but that would have made things too complex for something which I wanted as dynamic as possible. Indeed, I thought we could have objects which would provide some actions in some cases, and others in other cases -- and switching would happen in the snap of fingers during runtime! So with a several-methods framework, that would have given complex code, while the menu builder trick makes it easy : check the state at the start of your populate_menu implementation, and build your menu accordingly!

There is still something in common with the abstract+Impl I would like to mention : the idea that the base doesn't provide anything and hence doesn't get in the way, but that other things can come in and allow one to be lazy. For example, if you want to trigger a "call" action on an Ekiga::LiveObject, but nothing if it doesn't, you can use the Ekiga::Activator implementation of Ekiga::MenuBuilder. See menu-builder-tools.h for other examples. You don't have to use them, but they're available. And it's easy to come up with other such tools, and trivial to *combine* them.

Now, here is what I notice when reading the lib/engine/action/ framework code :

1. The Ekiga::ActionProvider class definitely looks like the Ekiga::LiveObject : it doesn't have a populate_menu getting an Ekiga::MenuBuilder, but a pull_actions getting an Ekiga::ActionStore, but really it's the same idea ! So the menu builder idea is built right inside the new action framework...

2. The Ekiga::ActionStore class is dumb (a dead list) when Ekiga::MenuBuilder is smart (either directly or by composition, see above), and I fear that will limit the features at one point.

3. The Ekiga::Action class is both too smart (as everything is already implemented and fixed) and too dumb (it's a struct turned into a class with direct accessors added to make up for the change). Again, that might hinder us at one point. I would suggest making Ekiga::Action purely abstract and providing an Ekiga::SimpleAction for a trivial implementation.

4. Where are the separators ? Ekiga::ActionStore will not allow us to add those... and if we do, we'll end up with an Ekiga::MenuBuilder under another name...

5. I guess the ghosts actions of Ekiga::MenuBuilder are just disabled actions (but they still need a callback -- Ekiga::Action has a single ctor!)...

6. I don't get the point of the 'activated' signal of actions. An action asks an object to do something, and actually doing something should trigger a signal : the gui should react to changes (the contact was removed) not to would-be-changes (the user clicked to remove the contact).

7. Ekiga::Actor has add_action methods... that is very wrong : the Foo::Bar objects knows what it's able to do, you don't dictate its conduct.

8. Ekiga::Actor has signals for when an action is enabled/disabled which just transmit what happens to the actions, and action_added/action_removed to know how the list evolves, and that's good.

9. Ekiga::Actor should also have an 'updated' signal which is triggered when any of the above happens. The idea is that a dumb gui which just regenerates itself when anything changes will only listen to that one signal. And if we add new signals, which also trigger 'updated', it will still work. That is a problem I have noticed with signals when designing the engine : because they don't go through the inheritance hierarchy, you can't use the compiler to notify you about all the places where you probably should handle a new signal you just added in the base class... In fact, that's why the observer design pattern exists ; but I find it makes the code too complex for what it brings to the table : better have as few signals as possible, and have one of them a catch-all.

10. Looking in ldap-book.cpp, I see that instead of a fire-and-forget situation where populate_menu creates actions and they may disappear at any point, a live object has a set of actions which are long-lived, created in the constructor and kept afterwards. That's an interesting take, but : how do we make clear that some actions are related to others (in the loudmouth presentity, there are actions to manage the presence subscription, they should be shown together), and how do we point to some actions as more important (for a loudmouth presentity, the action to start/continue a chat is more important than the presence subscriptions) ? Since we don't push things around (they get pulled), I don't see how to put a hierarchy in the actions!

11. URIActionProvider is just another name and implementation for PresentityDecorator... just like in point 1, we get the same idea under a different name...

So basically (emacs' org mode shows it better) :
| Before | After | Variation |
| LiveObject | Actor | not abstract (fragile) | | MenuBuilder | ActionStore | lost hierarchy of actions (separators | | (nothing) | Action | more code (struct with getters and setters!) | | URIActionProvider | PresentityDecorator | none |

I think I can claim the new code is not simpler, quite the contrary...

ekiga-devel-list mailing list

Reply via email to