Hi Julien,

I do not have the time to read, understand and debate about your
proposal. And actually, I don't want to. (Even though I could also write
a very long mail explaining why the current code is better than the old
one and allows many more things like a global call button for example).

If you disagree with the current code and approach, you will have to
live with it, because I do not want any change and I am the maintainer.
I have the final word.

Sorry if I look rude or if I act as a dictator because I don't want to,
but it is for my own sanity.

Damien

Le mercredi 14 janvier 2015 à 11:56 +0100, Julien Puydt a écrit :

> 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



-- 
Damien SANDRAS 

Ekiga Project 
http://www.ekiga.org
_______________________________________________
ekiga-devel-list mailing list
ekiga-devel-list@gnome.org
https://mail.gnome.org/mailman/listinfo/ekiga-devel-list

Reply via email to