Re: [PATCH v3 5/8] obexd: Add RegisterNotifications function

[Christian Fetzer <christian.fetzer@oss.bmw-carit.de>]

[-- Attachment #1: Type: text/plain, Size: 2370 bytes --]

Hi Luiz,

On 03/13/2013 08:57 PM, Luiz Augusto von Dentz wrote:
> Hi Christian,
>
>
> On Wed, Mar 13, 2013 at 1:27 PM, Marcel Holtmann <marcel@holtmann.org 
> <mailto:marcel@holtmann.org>> wrote:
>
>     Hi Christian,
>
>     >>> This allows applications to register for all different types
>     >>> of MAP event reports.
>     >>>
>     >>> In response to this call, the MSE should connect to the local MNS
>     >>> instance.
>     >>> ---
>     >>> doc/obex-api.txt   |  6 ++++
>     >>> obexd/client/map.c | 86
>     ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>     >>> 2 files changed, 92 insertions(+)
>     >> never ever intermix doc/ changes with actual code. You really
>     need to learn on how to split patches properly. There is no reason
>     at all that this are not two patches.
>     >>
>     >>
>     >>> diff --git a/doc/obex-api.txt b/doc/obex-api.txt
>     >>> index 759c4d8..ef5d85e 100644
>     >>> --- a/doc/obex-api.txt
>     >>> +++ b/doc/obex-api.txt
>     >>> @@ -651,6 +651,12 @@ Methods    void SetFolder(string name)
>     >>>                     Possible errors:
>     org.bluez.obex.Error.InvalidArguments
>     >>>  org.bluez.obex.Error.Failed
>     >>>
>     >>> +           void RegisterNotifications(boolean)
>     >>> +
>     >>> +                   Register / unregister reception of
>     notifications.
>     >>> +
>     >>> +                   Possible errors:
>     org.bluez.obex.Error.InvalidArguments
>     >>> +  org.bluez.obex.Error.Failed
>
>
> Why we are not doing this automatically? The application can opt out 
> by not connecting to signals, but I see no reason why not to be 
> connected to register the notification channel automatically when 
> connecting the MAP session if the remote support it.
>
> -- 
> Luiz Augusto von Dentz

as I tried to explain, the purpose of the function in the current patch 
is to select the one device for that you will receive notifications.
I agree, it would be better to hide the notification connection setup 
and rather do it implicitly.
That needs to go hand in hand with notification support for multiple 
devices; with removing the function, you loose the possibility to select 
a device.

I'll send a suggestion for multiple device notification support in a 
separate email and try to explain the relevant spec details.
Then we can discuss it.

Br,
Christian

[-- Attachment #2: Type: text/html, Size: 4863 bytes --]