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 > [email protected] > https://mail.gnome.org/mailman/listinfo/ekiga-devel-list -- Damien SANDRAS Ekiga Project http://www.ekiga.org
_______________________________________________ ekiga-devel-list mailing list [email protected] https://mail.gnome.org/mailman/listinfo/ekiga-devel-list
