Return-Path: MIME-Version: 1.0 In-Reply-To: References: <1256007499-19250-1-git-send-email-forrest.zhao@intel.com> <1256023119.9142.19.camel@pohly-mobl1.ikn.intel.com> Date: Tue, 20 Oct 2009 11:18:33 -0200 Message-ID: Subject: Re: [PATCH] Implements the OBEX server/SyncML client binding for syncEvolution (http://syncevolution.org/). From: =?UTF-8?Q?Jo=C3=A3o_Paulo_Rechi_Vita?= To: Zhao Forrest Cc: Patrick Ohly , "Zhao, Forrest" , "linux-bluetooth@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-bluetooth-owner@vger.kernel.org List-ID: On Tue, Oct 20, 2009 at 7:05 AM, Zhao Forrest wrote: >>> --- 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". > >> >> Off topic: what is the "PC Suite Services server"? >> > It's Nokia OBEX PC Suite Services. Johan may know more details about it. > >>> +       { "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. > >> 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. > >> >>> +       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/