468471 - Move Spotlight metadata out of the profile dir so results get shown

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[David Humphrey (:humph)]

spin-off from bug 290057 comment 150.

Spotlight won't show search results for anything under ~/Library/Thunderbird.  Instead it looks like we should be doing what Apple and Microsoft do, and put our .mozeml files in ~/Library/Caches/Metadata.  We need to re-root our code that does .mozeml logic there.  We also need to make sure we keep profile info so we can have this work with multiple profiles.

Comment 1

[David Humphrey (:humph)]

Attachment: Fix paths for Spotlight results

This allows us to deal with Spotlight sending results rooted in ~/Library/Caches/Metadata and convert them so Thunderbird looks in the Profile dir instead.

NOTE: I have hard coded "/Caches/Metadata" because I think this is safe; however it would be possible for me to use the same method I used to get the ~/Library dir and ask the OS for this path--we'll have to hard-code this elsewhere in JS, so it's an assumption I thought I could make here.

Comment 2

[David Humphrey (:humph)]

I forgot to mention above that my plan is to put the .mozeml in a structure that mirrors the profile, for example:

/Users/dave/Library/Caches/Metadata/Thunderbird/Profiles/bdpf5gyq.Shredder/ImapMail/mercury.senecac.on.ca/INBOX.mozmsgs

I need to alter the js bits of Spotlight to write-out to this dir, and will as soon as Sid's refactoring patch lands.

Comment 3

[David Humphrey (:humph)]

Attachment: Fix paths for Spotlight results, uses any profile dir

After discussions with bienvenu and sid on irc, this does a few things:

* Assumes that all Spotlight data will be located in ~/Library/Caches/Metadata/Thunderbird (i.e., I've dropped Profiles/<foo>/ since we can't assume the name of a profile, if random profile dirs are to be used).

* Works with any profile dir, not just those rooted in ~/Library/Thunderbird/Profiles

Comment 4

[David Humphrey (:humph)]

Attachment: Handle Spotlight files passed with -file arg

New patch with fix to deal with -file args vs. straight path names (see https://bugzilla.mozilla.org/attachment.cgi?id=352606&action=edit).

Huge thanks to Sid for testing and debugging on Windows.

Comment 5

[David :Bienvenu]

trying this now...

Comment 6

[David :Bienvenu]

I guess you're waiting for sid's other stuff to land before putting up a patch that changes where we put the .mozeml files, so that this code will work?

Comment 7

[David Humphrey (:humph)]

Yeah, that stuff was in such flux I figured it was more prudent to wait.  It won't take long once he gets it landed.

To test you can (obviously) copy some of your .mozeml files over manually.

Comment 8

[David :Bienvenu]

Comment on attachment 352608 [details] [diff] [review]
Handle Spotlight files passed with -file arg

thx for the patch, humph. We should wait for the patch that changes where we store the .mozeml files to land before landing this patch, I think.

Comment 9

[David Humphrey (:humph)]

Attachment: Decouple profile dir from search path logic

This changes searchCommon.js to use a platform-specific function for finding the proper .mozmsg location.  I have tested on Mac, but not on Vista directly (i.e., I did test the logic of what I did with Vista, which simply maintains the current behaviour, and this worked).

NOTE: this patch depends on a bunch of other stuff (i.e., command line handling code for -file here and in bug 290057), so shouldn't be landed before those get in.

Comment 10

[Rain [:rain1] (inactive)]

Comment on attachment 357805 [details] [diff] [review]
Decouple profile dir from search path logic



>+    if (!this._profileDir)
>+      this._profileDir = Components.classes["@mozilla.org/file/directory_service;1"]
>+                                   .getService(Components.interfaces.nsIProperties)
>+                                   .get("ProfD", Components.interfaces.nsIFile);

You can use Cc and Ci here and elsewhere, and please align the .s as such:

Cc[...]
  .getService(...)
  .get(...);

>
>+
>+    // Swap the metadata dir for the profile dir prefix in the folder's path
>+    var folderPath = aFolder.filePath.path

Nit: semicolon.

>+
>+  /// Use the folder's path (i.e., in profile dir) as is
>+  _getSearchPathForFolder: function winsearch_get_search_path(aFolder)
>+  {
>+    return aFolder.filePath;
>+  }

You forgot a comma here.

I tested it on Windows and it works fine after that comma is added.

Comment 11

[David Humphrey (:humph)]

Attachment: Decouple profile dir from search path logic v2

Fixed review issues.  Thanks, Sid.

Comment 12

[Rain [:rain1] (inactive)]

(In reply to comment #11)
> Created an attachment (id=357818) [details]
> Decouple profile dir from search path logic v2

Oh, one more thing, please change the vars to lets as that is the prevailing style of the file now.

Comment 13

[David Humphrey (:humph)]

Attachment: Decouple profile dir from search path logic v3

s/var/let/

Comment 14

[David Humphrey (:humph)]

Now that the -file handling stuff has made it in (see http://hg.mozilla.org/mozilla-central/rev/c56446e2e61d), can I get you to take a look at this other patch, Standard8?  I think I can pretty much close-up this spotlight bug once that's done.

Comment 15

[Mark Banner (:standard8)]

Comment on attachment 352608 [details] [diff] [review]
Handle Spotlight files passed with -file arg

>+
>+  PRInt32 numArgs;
>+  aCmdLine->GetLength(&numArgs);
>+  if (numArgs == 0)
>+    return NS_OK;
>+
>+  nsAutoString arg0;
>+  rv = aCmdLine->GetArgument(0, arg0);
>+  NS_ENSURE_SUCCESS(rv, rv);
>   
>-  nsAutoString mailUrl; // -mail or -mail <some url> 
>-  PRBool flag = PR_FALSE;
>-  rv = aCmdLine->HandleFlagWithParam(NS_LITERAL_STRING("mail"), PR_FALSE, mailUrl);
>-  if (NS_SUCCEEDED(rv))
>-    flag = !mailUrl.IsVoid();
>-  else 
>-    aCmdLine->HandleFlag(NS_LITERAL_STRING("mail"), PR_FALSE, &flag);
>-  if (flag)
>+  if (NS_LITERAL_STRING("-mail") == arg0)

This change looks wrong. It looks like you're requiring that -mail be the first argument. What happens if I want to do ./thunderbird -P example -mail imap-message://is/here/ ? I believe that could fail (or maybe it would have to be -file xyz -mail ...)

>-  // Get the nsILocalFile for this file:// URI.
>-  rv = MsgGetLocalFileFromURI(nativeArg, getter_AddRefs(folderPath));
>+  nsCOMPtr<nsILocalFile> localFile;
>+  NS_NewLocalFile(EmptyString(), PR_TRUE, getter_AddRefs(localFile));
>+  if (!localFile) 
>+    return NS_ERROR_FAILURE;

Its usually better to use NS_ENSURE_SUCCESS(rv, rv) because then we can find out the actual error rather than a generic one.

>+  nsCOMPtr<nsILocalFileMac> macLibraryDir(do_QueryInterface(localFile));
>+  rv = macLibraryDir->InitWithFSRef(&fsRef);
>+  if (NS_FAILED(rv)) 
>+    return rv;
>+
>+  // TB Spotlight data is in ~/Library/Caches/Metadata/Thunderbird
>+  macLibraryDir->Append(NS_LITERAL_STRING("Thunderbird"));
>+  nsAutoString metadataDirStr;
>+  macLibraryDir->GetPath(metadataDirStr);

Could this call fail? (and the other one to GetPath below it)

>+  folderPath = do_CreateInstance("@mozilla.org/file/local;1");

You should be checking this succeeded (preferably with rv).

Comment 16

[David Humphrey (:humph)]

Attachment: Handle Spotlight files passed with -file arg (v4)

Thanks for the review.  Here's a new patch that fixes your issues and stops using the 10.5 only call.  I've tested with both patches in this bug on Mac (10.5.6, I don't have access to 10.4), and Sid on Windows--both work.

Comment 17

[Mark Banner (:standard8)]

Comment on attachment 360531 [details] [diff] [review]
Handle Spotlight files passed with -file arg (v4)

sr=me. As discussed on irc, this needs the patch in bug 290057 to land on 1.9.1 before we land this.

Comment 18

[Mark Banner (:standard8)]

Comment on attachment 357819 [details] [diff] [review]
Decouple profile dir from search path logic v3

>diff --git a/mail/components/search/SpotlightIntegration.js b/mail/components/search/SpotlightIntegration.js

>+
>+  /// The user's profile dir, which we'll cache and use a lot for path clean-up
>+  _profileDir: null,
>+
>+  /// Spotlight won't index files in the profile dir, but will use ~/Library/Caches/Metadata
>+  _getSearchPathForFolder: function spotlight_get_search_path(aFolder) 
>+  {
>+    if (!this._profileDir)
>+      this._profileDir = Cc["@mozilla.org/file/directory_service;1"]
>+                           .getService(Ci.nsIProperties)
>+                           .get("ProfD", Ci.nsIFile);

Why not store just the path of the profile directory?

Also, if you want to cache this value, but don't want to init it until first use, you can do:

get _profileDir() {
  delete this._profileDir;
  return this._profileDir = Cc["@mozilla.org/file/directory_service;1"]
                              .getService(Ci.nsIProperties)
                              .get("ProfD", Ci.nsIFile);
}

>+    // Swap the metadata dir for the profile dir prefix in the folder's path
>+    let folderPath = aFolder.filePath.path;
>+    let fixedPath = folderPath.replace(this._profileDir.path, 
>+                                       "~/Library/Caches/Metadata/Thunderbird");

Does this work for the sort of paths on mac that get encoded due to non-ascii characters?

Comment 19

[Rain [:rain1] (inactive)]

Attachment: [checked in] Handle Spotlight files passed with -file arg (v4, merged to trunk)

The patch had bitrotted -- simple merge to trunk. Please credit the patch to humph. :)

Comment 20

[David Humphrey (:humph)]

(In reply to comment #18)
> (From update of attachment 357819 [details] [diff] [review])
> Why not store just the path of the profile directory?

I'll do it if you think it necessary.

> >+    // Swap the metadata dir for the profile dir prefix in the folder's path
> >+    let folderPath = aFolder.filePath.path;
> >+    let fixedPath = folderPath.replace(this._profileDir.path, 
> >+                                       "~/Library/Caches/Metadata/Thunderbird");
> 
> Does this work for the sort of paths on mac that get encoded due to non-ascii
> characters?

I'm struggling to find a way to test this.  If you have thoughts, please let me know, as this is all that holds us up from landing the entire Spotlight patch set.

Comment 21

[David Humphrey (:humph)]

Attachment: [checked in] Decouple profile dir from search path logic v4

Adds getter for _profileDir.

Comment 22

[Mark Banner (:standard8)]

Comment on attachment 367092 [details] [diff] [review]
[checked in] Decouple profile dir from search path logic v4

I inspected the diffs and this is fine.

Comment 23

[Mark Banner (:standard8)]

Comment on attachment 366110 [details] [diff] [review]
[checked in] Handle Spotlight files passed with -file arg (v4, merged to trunk)

Checked in: http://hg.mozilla.org/comm-central/rev/d3ef8cbd815e

and bustage fix:
http://hg.mozilla.org/comm-central/rev/4a3e80b5b8ab

Comment 24

[Mark Banner (:standard8)]

Comment on attachment 367092 [details] [diff] [review]
[checked in] Decouple profile dir from search path logic v4

Checked in: http://hg.mozilla.org/comm-central/rev/a0d3c5785eb1

Comment 25

[Mark Banner (:standard8)]

I guess this is fixed now.

Comment 26

[Nomis101]

In the error console I now see the error:

> Error: missing } after property list
> Source File: file:///Volumes/Shredder/Shredder.app/Contents/MacOS/modules/SpotlightIntegration.js
> Line: 709, Column: 2
> Source Code:
>  _getSearchPathForFolder: function spotlight_get_search_path(aFolder) 


Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090314 Shredder/3.0b3pre

Comment 27

[Rain [:rain1] (inactive)]

Attachment: bustage fix [Checkin: Comment 28]

Comment 28

[Serge Gautherie (:sgautherie)]

Comment on attachment 367402 [details] [diff] [review]
bustage fix
[Checkin: Comment 28]


http://hg.mozilla.org/comm-central/rev/5b546df58905

Comment 29

[Saul Tannenbaum]

With respect to comment 3 this produces some unwanted side effects.

For example, the general rule of thumb for testing new versions of Thunderbird is "create a fresh profile". But, with a common store for message texts across profiles, just creating a new profile just uses the same common store.

At a minimum, any documentation about starting fresh with a new profile should note this directory and provide advice about starting it fresh, as well. As this store is shared among profiles, it's not at all clear what the right thing to do is. (And, I'm not sure that Thunderbird gracefully recovers from having this directory deleted. That is, it doesn't seem to know it's gone and just continues to incrementally add messages. I'll file a separate bug on this once I can fully confirm this behavior.)