[Ekiga-devel-list] Design of the engine : LiveObject and MenuBuilder aren't bad

[Julien Puydt <jpuydt free fr>]

Hi,


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...

Snark