9:52 a.m.
Hello Congwu!
Is the "phone" branch ready for merging? It seems that at least the
"initiate slow sync" part still needs some work or cleaning up.
I'd can merge this after fixing the compiler warnings on the current
master later today, if you want.
I've looked at the code. Some comments:
Server Alerted Sync: SAN generation
ContentType in the 'Get' command during the SAN should be XML or WBXML
instead
of SAN Notification.
Why that? Perhaps I misunderstand the code, but doesn't it ensure that
the content type for the SAN message is
TransportAgent::m_contentTypeServerAlertedNotificationDS and *not* XML
or WBXML?
FileSyncSource: use x-vcalendar instead of x-calendar
This changes user-visible behavior, and not just in the FileSyncSource,
doesn't it? I agree that x-vcalendar is the better choice, but
x-calendar should still be accepted for users who have it in their
configs.
Join/dejoin Mutiple SyncSources, MB#4611
Can you move the escape code for the new virtual backend's
evolutionsource property into util.cpp? It's likely that we'll also need
it for multiple syncURLs.
+ "If the backend is a virtual data source, \n"
+ "this field points a comma seperated list of \n"
+ "sub datasources actually used for syncing.\n"
+ "If your sub datastore has comma in name, you\n"
+ "can separated by preceding it with '\\' \n"
Extra white space - evil! ;-) Spelling improvement:
"If the backend is a virtual data source,\n"
"this field must contain a comma separated list of\n"
"sub datasources actually used to store data.\n"
"If your sub datasource has a comma in its name, you\n"
"must prevent that comma from being mistaken as the\n"
"separator by preceding it with a backslash, like this:\n"
" evolutionsource = Source1PartA\,PartB, Source2\\Backslash\n"
--
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.
10:30 a.m.
I've looked at the code. Some comments:
Server Alerted Sync: SAN generation
ContentType in the 'Get' command during the SAN should be XML or
WBXML instead
of SAN Notification.
Why that? Perhaps I misunderstand the code, but doesn't it ensure that
the content type for the SAN message is
TransportAgent::m_contentTypeServerAlertedNotificationDS and *not* XML
or WBXML?
It means the request sent for SAN must have a san content type but when you use
"Get" to pull response you must use XML/WBXML.
This is found by Nokia phone and it is reasonable.
FileSyncSource: use x-vcalendar instead of x-calendar
This changes user-visible behavior, and not just in the FileSyncSource,
doesn't it? I agree that x-vcalendar is the better choice, but
x-calendar should still be accepted for users who have it in their
configs.
Yes, I just thought this was a typo.
Agree, anyway it breaks FileSyncSource with vcal2.1, I believe. No time to check today, I
may come out a patch for FileSyncSource later.
Join/dejoin Mutiple SyncSources, MB#4611
Can you move the escape code for the new virtual backend's
evolutionsource property into util.cpp? It's likely that we'll also need
it for multiple syncURLs.
+ "If the backend is a virtual data source, \n"
+ "this field points a comma seperated list of \n"
+ "sub datasources actually used for syncing.\n"
+ "If your sub datastore has comma in name, you\n"
+ "can separated by preceding it with '\\' \n"
Extra white space - evil! ;-) Spelling improvement:
"If the backend is a virtual data source,\n"
"this field must contain a comma separated list of\n"
"sub datasources actually used to store data.\n"
"If your sub datasource has a comma in its name, you\n"
"must prevent that comma from being mistaken as the\n"
"separator by preceding it with a backslash, like this:\n"
" evolutionsource = Source1PartA\,PartB, Source2\\Backslash\n"
Yes, done.
Best Regards,
Congwu
11 a.m.
Hello!
Merged. I was wondering a bit about the OBEX event leak (m_obexEvent):
The solution is to check explictly whether m_obexEvent already wraps a resource
before assigning a new value to it.
Setting a new value for m_obexEvent, one of our cxxptr smart pointers,
will automatically free any existing resource pointed to by it. Why was
it possible to leak something owed by it?
Or does the commit fix the problem that the event was kept around too
long?
--
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.
2:20 a.m.
Patrick wrote:
Hello!
Merged. I was wondering a bit about the OBEX event leak (m_obexEvent):
The solution is to check explictly whether m_obexEvent already wraps a
resource
before assigning a new value to it.
Setting a new value for m_obexEvent, one of our cxxptr smart pointers,
will automatically free any existing resource pointed to by it. Why was
it possible to leak something owed by it?
No, the assignment operator for eptr does not release the resource it already
owns,
see:
eptr & operator = (eptr &other) {
m_pointer = other.m_pointer;
other.m_pointer = NULL;
return *this;
}
Maybe we should change it to:
eptr & operator = (eptr &other) {
set (other.m_pointer);
other.m_pointer = NULL;
return *this;
}
Best Regards,
Congwu
2:22 a.m.
Chen,Congwu wrote:
Patrick wrote:
>Hello!
>
>Merged. I was wondering a bit about the OBEX event leak (m_obexEvent):
> The solution is to check explictly whether m_obexEvent already wraps a
>resource
> before assigning a new value to it.
>
>Setting a new value for m_obexEvent, one of our cxxptr smart pointers,
>will automatically free any existing resource pointed to by it. Why was
>it possible to leak something owed by it?
No, the assignment operator for eptr does not release the resource it already
owns,
see:
eptr & operator = (eptr &other) {
m_pointer = other.m_pointer;
other.m_pointer = NULL;
return *this;
}
Maybe we should change it to:
eptr & operator = (eptr &other) {
set (other.m_pointer);
other.m_pointer = NULL;
return *this;
}
Best Regards,
Congwu
It looks also not safe for self-assignment, does it?
Best Regards,
Congwu
7:21 a.m.
On Tue, 2009-12-01 at 02:22 +0000, Chen, Congwu wrote:
Chen,Congwu wrote:
>Patrick wrote:
>>Hello!
>>
>>Merged. I was wondering a bit about the OBEX event leak (m_obexEvent):
>> The solution is to check explictly whether m_obexEvent already wraps a
>>resource
>> before assigning a new value to it.
>>
>>Setting a new value for m_obexEvent, one of our cxxptr smart pointers,
>>will automatically free any existing resource pointed to by it. Why was
>>it possible to leak something owed by it?
>No, the assignment operator for eptr does not release the resource it already
>owns,
>see:
>eptr & operator = (eptr &other) {
> m_pointer = other.m_pointer;
> other.m_pointer = NULL;
> return *this;
> }
>
>Maybe we should change it to:
>eptr & operator = (eptr &other) {
> set (other.m_pointer);
> other.m_pointer = NULL;
> return *this;
> }
>
>Best Regards,
>Congwu
It looks also not safe for self-assignment, does it?
Both are indeed aspects that should be fixed instead of working around
them. Can you do that and once done, remove the then redundant code for
m_obexEvent?
--
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.
9:29 a.m.
Both are indeed aspects that should be fixed instead of working
around
them. Can you do that and once done, remove the then redundant code for
m_obexEvent?
See updated phone branch. There is also another commit so that
EvolutionCalendarBackend can accept vcalendar 1.0 type.
Best Regards,
Congwu
10:26 a.m.
On Thu, 2009-12-03 at 09:29 +0000, Chen, Congwu wrote:
>Both are indeed aspects that should be fixed instead of working
around
>them. Can you do that and once done, remove the then redundant code for
>m_obexEvent?
>
See updated phone branch. There is also another commit so that
EvolutionCalendarBackend can accept vcalendar 1.0 type.
Thanks, merged.
--
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.
3:09 p.m.
On Thu, 2009-11-26 at 10:30 +0000, Chen, Congwu wrote:
>I've looked at the code. Some comments:
>
> Server Alerted Sync: SAN generation
>
> ContentType in the 'Get' command during the SAN should be XML or
>WBXML instead
> of SAN Notification.
>
>Why that? Perhaps I misunderstand the code, but doesn't it ensure that
>the content type for the SAN message is
>TransportAgent::m_contentTypeServerAlertedNotificationDS and *not* XML
>or WBXML?
It means the request sent for SAN must have a san content type but when you use
"Get" to pull response you must use XML/WBXML.
This is found by Nokia phone and it is reasonable.
Hmm, this (or some of the other changes) breaks "test-dbus.py". In
TestConnection.testStartSync a client sends a SyncML message as XML and
expects a XML message back, but it gets
'application/vnd.syncml.ds.notification'.
[a bit later]
No, it's the "if m_serverMode, then generate SAN" part. That check is a
bit too simplistic, because server mode might already have been entered
via initServer(). That's what syncevo-dbus-server does when it receives
a connection from a client together with the first SyncML message.
I've changed the if clause to:
if (m_serverMode && !m_initialMessage.size())
In other words, a SAN will be generated only if there is no message to
process already. Reasonable?
--
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.
1:01 a.m.
On Tue, 01 Dec 2009, Ohly, Patrick wrote:
On Thu, 2009-11-26 at 10:30 +0000, Chen, Congwu wrote:
> >I've looked at the code. Some comments:
> >
> > Server Alerted Sync: SAN generation
> >
> > ContentType in the 'Get' command during the SAN should be XML or
> >WBXML instead
> > of SAN Notification.
> >
> >Why that? Perhaps I misunderstand the code, but doesn't it ensure that
> >the content type for the SAN message is
> >TransportAgent::m_contentTypeServerAlertedNotificationDS and *not* XML
> >or WBXML?
> It means the request sent for SAN must have a san content type but when you use
> "Get" to pull response you must use XML/WBXML.
> This is found by Nokia phone and it is reasonable.
Hmm, this (or some of the other changes) breaks "test-dbus.py". In
TestConnection.testStartSync a client sends a SyncML message as XML and
expects a XML message back, but it gets
'application/vnd.syncml.ds.notification'.
[a bit later]
No, it's the "if m_serverMode, then generate SAN" part. That check is a
bit too simplistic, because server mode might already have been entered
via initServer(). That's what syncevo-dbus-server does when it receives
a connection from a client together with the first SyncML message.
I've changed the if clause to:
if (m_serverMode && !m_initialMessage.size())
In other words, a SAN will be generated only if there is no message to
process already. Reasonable?
Yes, I think you are right
--
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.
--
Regards,
Chen Congwu
Moblin China Development
4434
days inactive
4441
days old
syncevolution@synevolution.org
9 comments
3 participants
participants (3)
-
Chen Congwu -
Chen, Congwu
-
Patrick Ohly