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

["João Paulo Rechi Vita" <jprvita@gmail.com>]

On Tue, Oct 20, 2009 at 7:05 AM, Zhao Forrest <forrest.zhao@gmail.com> 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/