377622 - Remove palm sync time stamp and category id from ab code.

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[Mark Banner (:standard8)]

Comment 1

[Mark Banner (:standard8)]

Opps committed too early.

I'd like to remove the palm sync category id and time stamp from the nsIAbDirectory interface (http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/mailnews/addrbook/public/nsIAbDirectory.idl&rev=1.45&mark=70-71,111-115#61) and the associated address book core code.

The "plan" is to change the palm sync extension code so that it initially uses something like extensions.palmsync.<name of ab in dirprefs>.categoryId and extensions.palmsync.<name of ab in dirprefs>.syncTimeStamp, falling back to ldap_2.servers.<ab>.whatever if it can't find the extension versions (and if they exist, then moving the values to the extensions.palmysync versions).

This will reduce the footprint of the address book core code, as well as simiplifying nsDirPrefs and the nsIAbDirectory/nsIAbDirectoryProperties interface.

Other sync type extensions shouldn't be using category id & time stamp anyway because that could conflict with palm sync.

Comment 2

[Wayne Mery (:wsmwk)]

First concern that comes to mind is maintaining the integrity of (AB data + palmsync state) for a given backup and restore of the AB.  

Second is more general case:
* what advantages might be gained from a separate palmsync datastore - for example would more palmsync improvements be possible without being tied to a specific release of AB?  
* likewise, what might suffer with less tight integration?

Comment 3

[Mark Banner (:standard8)]

(In reply to comment #2)
> First concern that comes to mind is maintaining the integrity of (AB data +
> palmsync state) for a given backup and restore of the AB.  

The palmsync state should be maintained as I won't be getting the core AB code to remove the old ldap_2.server category id/time stamp prefs. What will happen is the ldap_2.server prefs will be left alone, until the next time a sync is performed at which stage it'll be migrated to the new ones. If the AB gets deleted, then we'll delete the ldap_2.server prefs but that' won't matter.

> Second is more general case:
> * what advantages might be gained from a separate palmsync datastore - for
> example would more palmsync improvements be possible without being tied to a
> specific release of AB?

a) It'd be easier for other sync extensions to hook in to the AB (if palmsync can do it, they should be able to ;-) ).
b) In theory because you're not relying so much on the AB interfaces then you'd be able to make improvements easier - though I'm not sure what they would be in this case.
c) You're not going to get other extensions trying to use category id/time stamp at the same time as you.
d) It'd be faster for core address book with smaller memory footprint and simpler code (it'd also be easier to rework ab prefs which is one thing that'd be nice to do).

> * likewise, what might suffer with less tight integration?

You could loose some backwards compatibility. For example if you synced with latest trunk/extension with this bug fixed, and then went back to Thunderbird 2.0 & its extension version and tried to sync, you'd effectively have no category id / time stamp information. Although this could be seen as a problem, I think it would be best to break it now whilst big changes are going on, rather than later. Though this is really an effect of a bug and not due to less tight integration.

Otherwise I can't really think of any other problems due to less tight integration.

Comment 4

[Mark Banner (:standard8)]

Attachment: Patch v1

This patch removes the Palm Sync Category Id & Time Stamp pref setting from the core address book code and moves it into the extension code. I decided to keep the current ldap_2.servers.<ab>.<prefname> location via the Get/Set pref interfaces in nsIAbDirectory, at some stage in the future I may change my mind on this, but this should work correctly for now.

I've tested this on linux to make sure there's no problems with the removals, but I haven't tested this on windows to check palmsync builds/work.

Comment 5

[David :Bienvenu]

Comment on attachment 261712 [details] [diff] [review]
Patch v1

I like this - looks good, but I haven't had a chance to build it yet on Windows.

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 261712 [details] [diff] [review]
Patch v1

Requesting sr early - David's had a brief look at it but hasn't built it yet, so I'll fix any problems David finds if necessary.

Comment 7

[Scott MacGregor]

Comment on attachment 261712 [details] [diff] [review]
Patch v1

looks good to me Mark.

Comment 8

[Mark Banner (:standard8)]

Patch checked in -> fixed.