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
_______________________________________________
ekiga-devel-list mailing list
ekiga-devel-list@gnome.org
https://mail.gnome.org/mailman/listinfo/ekiga-devel-list
