Re: [SyncEvolution] [REVIEW REQUEST] Use MNotification in syncevo-dbus-server

On Mi, 2011-06-15 at 12:39 +0100, Salvatore Iovene wrote: Makes sense to me. Note that your changes conflict with the re-architecting of the syncevo-dbus-server that Chris Kühl is working on. Your changes will be merged first, I just wanted to warn Chris... Note: oldest patch at the bottom, read from bottom to top. + /* Detect what kind of manager we need: if /usr/bin/sync-ui + * exists, we shall use the MLite backend; otherwise, if + * libnotify is enabled, we shall use the libnotify backend; + * if everything fails, then we'll use the no-op backend. + */ + std::ifstream path(SYNC_UI_PATH); + if(path) { + mgr = new NotificationManager<NotificationBackendMLite>(); Isn't the logic reversed here? If the binary exists, then we are probably on a Netbook/Desktop, and thus should use libnotify. libnotify also should be the default when it is the only enabled backend. The access() system call is a faster way of checking for a file. + /** Creates the appropriate NotificationManager for the current + * platform. + * Note: NotificationManagerFactory does not take ownership of + * the returned pointer: the user must delete it when done. + */ + static NotificationManagerBase* createManager(); I'd prefer to not have plain pointers passed around like this, in particular in combination with explicit delete's. SyncEvolution uses the "Resource Acquisition Is Initialization" idiom, so if you get a plain pointer, it should be immediately stored in a smart pointer. In this case I would change the createManager() to return a boost::shared_ptr<NotificationManagerBase>. That way a caller can't get it wrong and the code which creates the instance can also be converted to using a shared pointer. Seems like a candidate for squashing. Same here: did the previous patches compile without this? If not, then I suggest squashing into the one that introduced the header dependency. - const std::string& viewParams = 0); + const std::string& viewParams = std::string()); This cleanup belongs into the previous patch, doesn't it? Not terribly important, just saying. + /** Publishes the notification. */ + void publish(const std::string& summary, const std::string& body, + const std::string& viewParams = 0); It would be useful to document the meaning of the various strings. In the classes which implement them I'd avoid duplicating the documentation. Such cut-and-paste docs tend to get out-of-sync over time. -- Best Regards, Patrick Ohly The content of this message is my personal opinion only and although I am an employee of Intel, the statements I make here in no way represent Intel's position on the issue, nor am I authorized to speak on behalf of Intel on this matter.