Re: [PATCH] Implements the OBEX server/SyncML client binding for syncEvolution (http://syncevolution.org/).

On Tue, Oct 20, 2009 at 7:05 AM, Zhao Forrest [off-list ref] wrote:

quoted

quoted

--- a/src/main.c
+++ b/src/main.c

@@ -53,6 +53,7 @@

 #define OPP_CHANNEL    9
 #define FTP_CHANNEL    10
 #define PBAP_CHANNEL   15
+#define SYNCEVOLUTION_CHANNEL  16
 #define PCSUITE_CHANNEL        24

Do we want to call this "SYNCEVOLUTION" or "SYNCML"? Both has pros and
cons. Treating it as generic SyncML capability hides the detail that
it's currently based on SyncEvolution. Exposing SyncEvolution itself
would allow to activate multiple different SyncML implementations at the
same time.

My preference would be to keep it as is, but I wanted to ask anyway.

I have the same idea as you do, and prefer to keep it as "SYNCEVOLUTION".

quoted

Off topic: what is the "PC Suite Services server"?

It's Nokia OBEX PC Suite Services. Johan may know more details about it.

quoted

quoted

+       { "syncevolution", 'e', 0, G_OPTION_ARG_NONE, &option_syncevolution,
+                               "Enable OBEX server for Syncevolution client" },

Mind the spelling: "syncevolution" (lower case) is the command line
tool, "SyncEvolution" (camel case) the project. "Syncevolution" is a
typo ;-)

Will fix it ASAP.

quoted

Is there documentation that should be updated together with introducing
the new feature?

The patch does not introduce any new d-bus API to obexd, so no doc need to
be updated.

quoted

quoted

+       if (option_syncevolution == TRUE) {
+               services |= OBEX_SYNCEVOLUTION;
+               bluetooth_init(OBEX_SYNCEVOLUTION, "OBEX server for"
+                               " Syncevolution client", NULL,
+                               SYNCEVOLUTION_CHANNEL, TRUE, FALSE, FALSE,
+                               NULL);
+       }
+

Which string is going to appear in the service description (the SDP
part)? This one here? Mentioning "SyncML" would be useful. So I suggest
"OBEX server for SyncML, using SyncEvolution". Leave out the
"SyncEvolution client", because the term client is a) overloaded
(SyncML/OBEX/D-Bus) and b) SyncEvolution could be both client and server
in this context (SAN => SyncML client, SyncML message => SyncML server).

OK. Will expose service name as "OBEX server for SyncML, using SyncEvolution"
since ProviderName attribute is not supported by obexd now.

If it's consensus the right way would be exposing SyncEvolution
through ProviderName, please add a comment to the code telling that,
so someone remember to fix it by the time obexd supports it.

-- 
João Paulo Rechi Vita
MSc Computer Science Student
Computer Engineer
IC / Unicamp
http://jprvita.wordpress.com/