On Mi, 2011-06-15 at 12:39 +0100, Salvatore Iovene wrote:
> There is a NotificationManagerFactory object that creates a
> NotificationManager that uses the right backend (MLite, libnotify, or a
> dummy no-op backend) and syncevo-dbus-server.cpp is now agnostic to
> what's happening behind the scene.
>
> This architecture might seem overly complicated, but it would be really
> easy to expand in the future, should the need arise.
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.
> $ git log --pretty=oneline 9a849f6588052ef093aaed038bddc23a10bf952b..HEAD
> b16777e603daa259d67332a2f0f401927527d296 syncevo-dbus-server: use a "factory"
> to create the appropriate NotificationManager.
+ /* 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.
> b7bcad6c01c9364a39b5b5730b02fc66dcf0d207 syncevo-dbus-server: update
> MNotification parameters to correct ones.
Seems like a candidate for squashing.
> 507e5a308b288faf400bf0f09bfd4143d7fbad43 syncevo-dbus-server: use #defines
> for the dbus parameters.
> fe2e6d17335aa8884f0252784c8148202737b503 syncevo-dbus-server: initial
> implementation of mlite backend.
> 94abeee4f52c278ef3cdadf2e8e1964d8169c5dc syncevo-dbus-server: added inclusion
> for Noop notification backend.
Same here: did the previous patches compile without this? If not, then I
suggest squashing into the one that introduced the header dependency.
> 011c750c6c91c735fd2f0bfa52d141f884af92bc syncevo-dbus-server: semi-working
> stub for mlite notifications.
- 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.
> 9978e870f532cb0787959c32fa4ae8a9e1ffcc17 syncevo-dbus-server: notifications
> system made more generic.
+ /** 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.
_______________________________________________
SyncEvolution mailing list
[email protected]
http://lists.syncevolution.org/listinfo/syncevolution
