Open Bug 377242 Opened 13 years ago Updated 6 months ago

[SoC] Enable Roaming Support in Thunderbird

Categories

(Thunderbird :: General, defect)

defect
Not set

Tracking

(Not tracked)

People

(Reporter: chofmann, Assigned: nick.kreeger)

References

()

Details

(Keywords: helpwanted)

Attachments

(2 files, 14 obsolete files)

Tracking bug for Summer of Code project
OS: Windows XP → All
Hardware: PC → All
Summary: Enable Roaming Support in Thunderbird → [SoC] Enable Roaming Support in Thunderbird
Roaming should also provide support for remote signatures (see Bug 65574).
Note that the code in extensions/sroaming *may* be a useful place to start/for reference. Though it only works with non-toolkit builds (see bug 378647).

In either case, it'd be really nice if this code could be designed/positioned so that other apps could use it as well (e.g. SeaMonkey).
(In reply to comment #2)
> Note that the code in extensions/sroaming *may* be a useful place to start/for
> reference. Though it only works with non-toolkit builds (see bug 378647).
> 
> In either case, it'd be really nice if this code could be designed/positioned
> so that other apps could use it as well (e.g. SeaMonkey).
> 

I agree, the hope is to design the feature so that other mailnews apps (SeaMonkey, Correo, etc) can take advantage as well.
Glad to see some interest in picking this up!
See bug 249323 comment 8.
FYI, I have very limited time, but if I can help with answering a few questions, I'd be glad to help.
Bug 78858: (ability to store filters on IMAP).
Attached patch July 5th Patch (obsolete) — Splinter Review
This is just to post some of my progress to the feature.. It listens to events for adding to the addressbook, sending off the notice, parsing the TOC file, exporting the attachments, and (not done yet) begin to move the new files back in.
Assignee: bienvenu → nick.kreeger
Version: unspecified → Trunk
Depends on: 387712
Attached patch July 19th Patch (obsolete) — Splinter Review
This is a significant upgrade to the roaming service since the last patch. The focus on this version is to enhance the update message sniffing, processing, and logging. Update messages are tracked by their date in seconds time, it provides a easy (and cheap) way of checking to see if a header has been processed. Messages are then downloaded and saved in the OS temporary directory, under a folder with a corresponding name as the date in seconds time. Once all the attachments for a message have been downloaded, then the files get copied into where they need to go (not in this patch, I'm still thinking about timing). Then the next oldest message is processed so that updates do not get overridden.

As with the previous version of the patch posted here, this only works with the address book and IMAP accounts. There are also some prefs that need to be saved as well to turn on roaming. I will post those here later if there is any interest in giving this thing a spin.
Attachment #271152 - Attachment is obsolete: true
Attached patch Aug. 3 Diff (obsolete) — Splinter Review
Continued updates, this time I added a class to sync prefs based off of a file from an attachment. There are also numerous updates, including logic for full and delta update messages. I will update the wiki as appropriate this afternoon.
Attachment #273052 - Attachment is obsolete: true
Cool, Nick, I'm looking forward to trying this!
(In reply to comment #9)
> Created an attachment (id=275149) [details]
> Aug. 3 Diff
> 
> Continued updates, this time I added a class to sync prefs based off of a file
> from an attachment. There are also numerous updates, including logic for full
> and delta update messages. I will update the wiki as appropriate this
> afternoon.
> 

Sorry, I jumped the gun on this patch, the attachment downloading code is broken (again)...
Depends on: 391402
Attached patch Aug 15 Patch (obsolete) — Splinter Review
Here is an update patch. This patch includes the patch I have made to make the composer listener code work.
Attachment #275149 - Attachment is obsolete: true
Attached patch Aug 15 Patch2 (obsolete) — Splinter Review
Oops, I diff'd off a new tree and forgot to su-do CVS add my new files. Disregard the old patch.... Doh!
Attachment #276901 - Attachment is obsolete: true
Attached patch Aug. 19th Diff (obsolete) — Splinter Review
This patch introduces full POP3 support, API's for setting the sync server (and turn it on), and fixes some troubles with the AB sync. 

There are still a couple of issues:
1.) UpdateMessageProcessor instances are leaking (I've been tracking this down all weekend)
2.) IMAP sync folder needs to be opened at startup. 
3.) If the Address Book has been opened, the copied in abook.mab isn't loaded until the app restarts. However, if the AB service hasn't been started, the sync works perfect. 
4.) The update message's aren't getting purged (removed) because I have some logic that needs to be finished (messages are getting removed before they are done processing).
5.) Update messages aren't getting marked as read (or hidden from the user yet, this is for easy development sake).

I'm suppose to have a "evaluation" ready patch by 12 PM Pacific today. This is what I'm turning in. I will continue hacking away for the August 31st code submission deadline.
Attachment #276902 - Attachment is obsolete: true
cool, I'll look through this...
Attached patch Patch V1 (obsolete) — Splinter Review
Review ready patch. If you want me to, I can #ifdef the code that touches existing modules in the interim.
Attachment #277407 - Attachment is obsolete: true
Attachment #279759 - Flags: review?(bienvenu)
very cool!

+    PRBool useSyncService;
+    nsresult rv = prefBranch->GetBoolPref(MSG_PREF_ENABLESYNC, &useSyncService);
+    if (NS_SUCCEEDED(rv) && useSyncService)

don't need to redeclare rv; it's declared above.

same thing here:

+    PRBool useSyncService;
+    nsresult rv = prefBranch->GetBoolPref(MSG_PREF_ENABLESYNC, &useSyncService);

+ *   Nick Kreeger <nick.kreeger@park.edu>
+ * Portions created by the Initial Developer are Copyright (C) 1998
+ * the Initial Developer. All Rights Reserved.

should be 2007 (this occurs more than once)

+  [noscript] boolean IsUpdateMessageHdr(in nsIMsgDBHdr aMsgHdr);
+  [noscript] void FoundUpdateMessage(in nsIMsgDBHdr aMsgHdr);

why [noscript]?

+    nsresult rv = prefBranch->GetBoolPref(MSG_PREF_ENABLESYNC, &useSyncService);
+    if (NS_SUCCEEDED(rv) && useSyncService)
+      nsCOMPtr<nsIMsgRoamingService> roamingService = do_GetService(NS_MSGROAMINGSERVICE_CONTRACTID);


maybe a comment why we're causing the roaming service to get instantiated here?

+nsresult GetContentTypeForFileName(nsACString &inFileName, nsACString &aFileType)
+{
+  nsCAutoString fileType;
+  nsCAutoString fileName(inFileName);
+  
+  if (fileName.Equals("abook.mab"))
+    fileType.AppendLiteral("application/x-rdf");
+  
+  return NS_OK;
+

this doesn't look right - do we want to be filling in aFileType here?
more later - just checkpointing...
+  void SetRoamingServer(in nsIMsgIncomingServer inIncomingServer);
+  
+  /**
+   * Get the incoming server that the roaming service is using for bootstrapping the
+   * update messages with.
+   * @return The incoming server the roaming service is using. If one is not set, then
+   *         nsnull will be returned.
+   */
+  nsIMsgIncomingServer GetRoamingServer();


why not just attribute nsIMsgIncomingServer roamingServer?

same here:

+  /**
+   * Toggle the roaming service on or off.
+   * @param inShouldSync State to set the roaming service.
+   */
+  void SetShouldSync(in boolean inShouldSync); 
+  
+  /**
+   * Get the current roaming sync state.
+   * @return The state of the roaming service.
+   */
+  boolean GetShouldSync();
+

attribute boolean shouldSync;



+  nsMsgRoamingService   *mMsgRoamingServiceRef;  // weak ref
...
+  NS_IF_ADDREF(mMsgRoamingServiceRef = aMsgRoamingService);

this looks like a strong ref, and I don't see it getting released anywhere...

+// Pick one or the other listener
+NS_IMPL_ISUPPORTS5(nsMsgRoamingService, nsIMsgRoamingService, nsITimerCallback, 
+                   nsIObserver, nsIUrlListener, nsIMsgSendListener)
+

Is that comment still valid, or do you need both?

+  if (mCreateFullUpdateMsg)
+    rv = GetFullUpdateMsgSubject(subjectStr);
+  else
+    rv = GetDeltaUpdateMsgSubject(subjectStr);

this can be rv = mCreateFullUpdateMsg ? GetFullUpdateMsgSubject(subjectStr) : GetDeltaUpdateMsgSubject(subjectStr);

why does base/src/Makefile.in need these two include dirs? (especially profile)
+		  profile \
+		  intl \

So if the sync pref is turned off, we won't init the roaming service, and it will be pretty much passive?

I'd like to give this a try, but I'm thinking applying a patch with these comments addressed would avoid a round of backing out the patch and applying a new one.
Comment on attachment 279759 [details] [diff] [review]
Patch V1

minusing in favor of patch with comments addressed.
Attachment #279759 - Flags: review?(bienvenu) → review-
Attached patch Patch V2 (obsolete) — Splinter Review
Here's a followup patch. Sorry for the delay, got stuck moving the past week or so.

> why does base/src/Makefile.in need these two include dirs? (especially profile)
> +                 profile \
> +                 intl \
> 

We need 'profile' for access to the profile name (used in the update message subject line).
We also need 'intl' for access to the string bundle service, to access the branded name of the application (also used in the subject line of an update message).

> So if the sync pref is turned off, we won't init the roaming service, and it
> will be pretty much passive?
> 

In the current implementation, yes.

If we want to be able to prompt the user that roaming information exists on an account, we can remove the pref-checks in the storage classes. However, before we can do this, I think I'm going to need to add some additional information to the full-update message.
Attachment #279759 - Attachment is obsolete: true
Attachment #281243 - Flags: review?(bienvenu)
Attached file Roaming Setup Extension (obsolete) —
Here is the XUL roaming setup extension for Thunderbird. It adds a menu item to the "Tools" menu called "Roaming Settings...".

You can turn on the roaming service, choose the account to use as a bootstrapper for update messages, and pick what type of data to sync (currently just prefs and addressbook).

This is my first attempt at making a XUL extension, so if something is broke, please be sure to note it here. I think it turned out pretty decent considering my crash-course on XUL over the past couple of days ;-)

P.S. You will have to install this one by hand in Tbird (sorry).
> P.S. You will have to install this one by hand in Tbird (sorry).
>

Using the supplied Add-Ons manager of course... 

there's a small case problem with the extension - curServerUri vs curServerURI, or something like that, but it breaks the account picker because of the js exception.
In nsresult nsMsgRoamingService::Init()

it should be

  nsCOMPtr<nsIMsgFolder> rootFolder;
  rv = mSyncServer->GetRootFolder(getter_AddRefs(rootFolder));
  NS_ASSERTION(NS_SUCCEEDED(rv), "### Could not get the specified sync folder..!");


instead of

  nsCOMPtr<nsIMsgFolder> rootFolder;
  rv = mSyncServer->GetRootFolder(getter_AddRefs(rootFolder));
  NS_ASSERTION(rv, "### Could not get the specified sync folder..!");

because rv == 0 is success.

I'm having some problems getting this working. I set up two profiles to use roaming, and to sync both prefs and address books, using the same pop3 account. Then I added a card to one address book and shut down. It looks like we try to send the update notification on shutdown, which is problematic unless we block shutdown from happening while we're sending, because components get torn down from under us while the send is happening. Doing things on shutdown is problematic - one approach is to open a window, perhaps hidden, and leave it open until we've finished the operation, which should prevent the app from exiting. I'll try to dig a little deeper into that.


I did get a couple full update messages to get sent,  but I don't see them get applied to the profile they're downloaded in... should this scenario work?
Attached file Roaming Setup Extension V2 (obsolete) —
Here is an updated extension package that fixes the JS bustage.
Attachment #281613 - Attachment is obsolete: true
> Doing things on shutdown is problematic

Yes. I hit the same problem with my "session roaming", which inherently relies on that concept. Better is (both for the user and to sovle this problem) to sync immediately when things are changed, e.g. directly after the address in the abook has been added or edited, similar with other data types you sync (which is the hard part).
See bug 397498 - while I agree that doing too much at shutdown could be annoying for the user, it's pretty much unavoidable to have to sometimes do things at shutdown, so Nick is creating a general mechanism that mailnews components can use to do asynchronous things at shutdown.
Depends on: 397498
Attached file Roaming Setup Extension V3 (obsolete) —
Here is a new extension that makes the dialog a real dialog!
Attachment #281774 - Attachment is obsolete: true
Attached patch Patch V3 (obsolete) — Splinter Review
Here is the next go-round of the roaming patch. There isn't any architectural changes in this patch with the exception of wiring up the roaming service to the shiny new shutdown service.
Attachment #281243 - Attachment is obsolete: true
Attachment #287421 - Flags: review?(bienvenu)
Attachment #281243 - Flags: review?(bienvenu)
+  rv = directoryService->Get(NS_OS_TEMP_DIR, NS_GET_IID(nsIFile), getter_AddRefs(tempDirFile));
+  NS_ENSURE_SUCCESS(rv, rv);
+  
+  nsCOMPtr<nsIFile> syncFileDir;
+  rv = tempDirFile->Clone(getter_AddRefs(syncFileDir));
+  NS_ENSURE_SUCCESS(rv, rv);
+  syncFileDir->AppendNative(NS_LITERAL_CSTRING(MSG_ROAMINGSYNCFILEFOLDER));
+  
+  PRBool exists;
+  syncFileDir->Exists(&exists);
+  if (!exists)
+    syncFileDir->Create(nsIFile::DIRECTORY_TYPE, 0600);

I'm noticing that trying to create a directory in the designated temporary directory of my Ubuntu Linux distribution won't give me the correct permissions to do the work that needs to be done by the roaming service. I've tried playing with the permissions octal - still no luck. I'm thinking about doing all the sync work in the profile directory (maybe just for Linux..).
(In reply to comment #31)
> +  rv = directoryService->Get(NS_OS_TEMP_DIR, NS_GET_IID(nsIFile),
> getter_AddRefs(tempDirFile));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  
> +  nsCOMPtr<nsIFile> syncFileDir;
> +  rv = tempDirFile->Clone(getter_AddRefs(syncFileDir));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  syncFileDir->AppendNative(NS_LITERAL_CSTRING(MSG_ROAMINGSYNCFILEFOLDER));
> +  
> +  PRBool exists;
> +  syncFileDir->Exists(&exists);
> +  if (!exists)
> +    syncFileDir->Create(nsIFile::DIRECTORY_TYPE, 0600);
> 
> I'm noticing that trying to create a directory in the designated temporary
> directory of my Ubuntu Linux distribution won't give me the correct permissions
> to do the work that needs to be done by the roaming service. I've tried playing
> with the permissions octal - still no luck. I'm thinking about doing all the
> sync work in the profile directory (maybe just for Linux..).
> 

Disregard.. I'm an idiot ;-)

Attached patch Patch V4 (obsolete) — Splinter Review
Here is another roaming update patch. This time, I tried a different class for listening to AB changes - so data can be refreshed at real time.

The roaming service also has start/stop methods in this patch to work with the extension better.

I'm also thinking about syncing conflicts at this stage - 'cause I know they are going to happen...
Attachment #287421 - Attachment is obsolete: true
Attachment #288167 - Flags: review?(bienvenu)
Attachment #287421 - Flags: review?(bienvenu)
Attached patch Patch V5 (obsolete) — Splinter Review
This is another upgrade patch to V4. This time, I fixed most of the address book syncing issues by implementing a basic sync-conflict prompt to the roaming service. A listening class can inform the roaming service that there is a conflict in the data and the user can choose to either override, discard, or merge (optional to the calling class).

The only issue that remains is the ability to refresh the AB view when the address book window is open. For testing, make your changes - then be sure to close the address book window. I'm working on finding a way to inform the XUL tree to refresh.....
Attachment #288167 - Attachment is obsolete: true
Attachment #288621 - Flags: review?(bienvenu)
Attachment #288167 - Flags: review?(bienvenu)
 nsresult nsAbMDBDirectory::NotifyItemChanged(nsISupports *item)
 {
   nsresult rv;
   nsCOMPtr<nsIAddrBookSession> abSession = do_GetService(NS_ADDRBOOKSESSION_CONTRACTID, &rv);
   NS_ENSURE_SUCCESS(rv,rv);
 
   rv = abSession->NotifyItemPropertyChanged(item, nsnull, nsnull, nsnull);
   NS_ENSURE_SUCCESS(rv,rv);
+  
+  NotifyRoamingService();
   return rv;
 }

So, why can't the roaming service just hook into the nsIAddrBookSession notifications if its enabled? At the moment this has a perf impact on the current notification methods even if the roaming service isn't enabled. Its also another notification method that we need to maintain and not forget (which if it was hooked into nsIAddrBookSession would be easier to manage).

It also means that it is more difficult for extensions to provide an address book type and have that synced via the roaming service as well (I don't know if any do yet, but even if there aren't any, we'd have to reimplement this bit on each ab type we want to support e.g. migrating away from mork).

(In reply to comment #34)
> The only issue that remains is the ability to refresh the AB view when the
> address book window is open. For testing, make your changes - then be sure to
> close the address book window. I'm working on finding a way to inform the XUL
> tree to refresh.....

My instinct here would be to add another nsIAddrBookSession based notification that is called when the database is reloaded. nsAbView can then hook onto this notification (easily because it already hooks onto nsIAddrBookSession). You can then check if the nsAbView is currently displaying the updated directory.

You probably need to be slightly careful with mailing list changes (i.e. add/remove of lists). You'd need to get the RDF trees to refresh somehow.
> So, why can't the roaming service just hook into the nsIAddrBookSession
> notifications if its enabled? At the moment this has a perf impact on the
> current notification methods even if the roaming service isn't enabled. Its
> also another notification method that we need to maintain and not forget (which
> if it was hooked into nsIAddrBookSession would be easier to manage).

When we originally started to plan the service, we wanted to try and keep as many things generic as possible to allow for new data types to be added to the sync queue without having to throughly change nsMsgRoamingService. While there are exceptions right now, we don't really want to have the roaming service listen for AB changes...

Instead, I have been rethinking the communication system between the roaming service and the data classes. Rather than having the data classes inform the roaming service of changes, whenever the roaming service decides its time to think about sending an update message - it asks the listening data classes through the |nsIObserver| interface. This removes the problems that have been pointed out above. Since this version of AB syncing is meant to only sync mork-databases - we decided to keep the code in that class since it gives us easy access to the actual |nsIAddrDatabase| and makes it easy to use the fancy file APIs.

> It also means that it is more difficult for extensions to provide an address
> book type and have that synced via the roaming service as well (I don't know if
> any do yet, but even if there aren't any, we'd have to reimplement this bit on
> each ab type we want to support e.g. migrating away from mork).

I'm not aware of any address book extensions.. Do you have a link to one so that I can look into this?
Attached patch Patch V6Splinter Review
This patch reworks the notification system of the roaming service. The service will ask the callers if they have any changes via the |nsIObserver| interface. I've also reworked the signature of the "add-listener" function for the roaming service to take in information about the data-types sync file. This way the roaming service can be kept much more generic, rather than having statically typed definitions and file information in the service itself.

I also have a new version of the extension, which I will post shortly.
Attachment #288621 - Attachment is obsolete: true
Attachment #291749 - Flags: review?(bienvenu)
Attachment #288621 - Flags: review?(bienvenu)
Attached file Roaming Extension V4
Attachment #287349 - Attachment is obsolete: true
I haven't had a chance to run this, but some initial comments:

Wow, this is a lot of work!

wrong indentation here:
+    if (shouldSync)
+    StartListening();

Does this leak memory? Does GetCharPref allocate memory? If so, I think you want prefValue.Adopt(...)

+      rv = prefBranch->GetCharPref(curPrefName.get(), &prefStr);
+      if (NS_SUCCEEDED(rv))
+        prefValue.Assign(prefStr);
+      break;



Here, I think you can just use a break statement instead of the doSearch var
+  PRBool doSearch = PR_TRUE;
+  PRInt32 arrSize = mRoamingListeners.Count();
+  for (PRInt32 i = 0; i < arrSize && doSearch; i++)
+  {
+    RoamingListener *roamingListener = mRoamingListeners[i];
+    if (roamingListener->observer == aObserver && roamingListener->syncTopic.Equals(aTopic))
+    {
+      mRoamingListeners.RemoveObject(roamingListener);
+      doSearch = PR_FALSE;
+    }
+  }


If you want native line endings (and I think you do), you could use MSG_LINEBREAK or something like that...

+    changedFileContent.Append('\n'); // Use a OS-independent call here.


+  nsString fullMessage, message1, message2;
+  roamingStrBundle->GetStringFromID(SYNC_CONFLICT_DESC1, getter_Copies(message1));
+  roamingStrBundle->GetStringFromID(SYNC_CONFLICT_DESC2, getter_Copies(message2));
+  fullMessage.Append(message1);
+  fullMessage.Append(' ');
+  fullMessage.Append(inDataName);
+  fullMessage.Append(message2);

You can't assume that all languages will want inDataName at the end of the sentence. You should do something like:

+1001=There has been a sync conflict with the %S

and use a string formatting routine to generate the string...
Bienvenu: With all the work done here, will you put it in your todo after 3.0?
Attachment #291749 - Flags: review?(bienvenu)
Comment on attachment 291749 [details] [diff] [review]
Patch V6

clearing obsolete review request. This patch has a some cool stuff in it, but I don't know if it's going to get picked up.
Keywords: helpwanted
David, what exactly would the next steps be for someone who is interested in respoonding to the helpwanted keyword?
de-bitrotting the patch, and addressing my review comments, for a start.
Does/would Weave not do the same job/be a better direction to take things in for "roaming profile"-type support?

Gerv
Weave integration is probably a better way to go - but one advantage of the direction of this patch is that it doesn't require any server other than your mail server, which you already have...
See Also: → 403837
You need to log in before you can comment on or make changes to this bug.