Closed
Bug 430614
Opened 17 years ago
Closed 6 years ago
[GSoC] Thunderbird integration into Windows Vista/Windows Search indexer
Categories
(Thunderbird :: Search, enhancement, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 3.0b4
People
(Reporter: rain1, Assigned: rain1)
References
(Depends on 4 open bugs, Blocks 1 open bug, )
Details
(Keywords: uiwanted, Whiteboard: [better UI in bug 467116])
Attachments
(6 files, 6 obsolete files)
(deleted),
text/plain;charset=UTF-16
|
Details | |
(deleted),
patch
|
rain1
:
review+
rain1
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Bienvenu
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
rain1
:
review+
rain1
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Bienvenu
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Bienvenu
:
review+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.1.14) Gecko/20080404 Firefox/2.0.0.14
Build Identifier: version 2.0.0.12 (20080213)
So far, users of Microsoft's Windows Vista search engine and Windows Search product (for older versions of Windows) have had to use a Microsoft email client or an out-of-date closed source third party tool (which works only with old versions of Windows Search, and does not work at all with Windows Vista) [1] to enable their emails to be indexed.
The aim of this project is to develop an open source plugin to allow these search engines to index Thunderbird emails.
[1] http://www.citeknet.com/Products/ProtocolHandlers/WDSThunderbirdMozillaEudoraMailAddin/tabid/70/Default.aspx
Reproducible: Always
Steps to Reproduce:
1.
2.
3.
Assignee | ||
Updated•17 years ago
|
Comment 1•17 years ago
|
||
Confirming due to approved GSoC work.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 2•17 years ago
|
||
The original proposal was to use the mbox files directly and serve them, along with some metadata stored in an SQLite database, to the indexer.
However there are concurrency issues with this approach. (Here COM means the Windows Search COM component)
The SQLite database and mbox file will be accessed from both Tb and COM. The SQLite database should work out to be thread-safe, returning errors if opened concurrently in both TB and COM (and I'll be able to handle the error). How will the mbox be handled, though? The closed source third party addin does have the same issue.
One way might be to check for parent.lock in COM, but that would mean that people who keep Tb open all the time wouldn't ever have their mails indexed. Also, this only resolves the issue in one direction.
Another alternative is the shadow copy feature that Microsoft provides on NTFS in Windows XP and Vista [1]. I'll have to see whether this can work with Windows Search at all. This will also make the code not work on FAT32 partitions. (It actually sounds like overkill)
[1] http://msdn2.microsoft.com/en-us/library/aa384648(VS.85).aspx
So it might be that writing out all mails into individual files is the only workable, failproof solution with the mbox format.
Does anybody have any other ideas how this might be resolved?
Last year's SoC had a project on Tb integration with Beagle (mentored by Beagle). The way chosen was individual files per email. The sources are available at [2].
[2] http://svn.gnome.org/svn/beagle/trunk/beagle/thunderbird-extension/
What I want to do and what is done in the extension are quite similar -- this can definitely be ported over with a few changes. The license is MIT, so not a problem.
If this is the way chosen, I can add support for contacts, and possibly even backport changes to the Beagle extension.
Assignee | ||
Updated•17 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Updated•17 years ago
|
Assignee: nobody → sid1337
Status: ASSIGNED → NEW
Assignee | ||
Updated•17 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•17 years ago
|
||
Unfortunately, it seems as if Tb doesn't use any locks apart from parent.lock. This, coupled with the fact that mbox isn't really designed for concurrent access, means that we just might have to write out mails individually. Does anyone have any idea how else this could be resolved? Could any method using last modification dates be workable?
(Ah, maildir would be a whole lot easier to work with)
Assignee | ||
Comment 4•17 years ago
|
||
The Spotlight plugin by David Bienvenu [1] seems a great choice to port.
[1] https://bugzilla.mozilla.org/show_bug.cgi?id=290057
The messages are first read (in Javascript, using streamMessage) as RFC822, then converted to UTF-8. We can skip the conversion and store the RFC822 form itself.
The way results are handled is also good: the files are named by their message IDs. When a result is selected, the message ID is extracted, and the correct message is then selected.
According to David we should use the same approach for Windows Search, as we don't want any platform-dependent code. I think so too.
My first job will be to port the Spotlight component with all the necessary changes.
Assignee | ||
Comment 5•17 years ago
|
||
I'm posting somewhat regular updates to the bug status to http://sid0.blogspot.com .
Assignee | ||
Updated•17 years ago
|
Assignee | ||
Comment 6•16 years ago
|
||
This is a rough (but working) version of the Windows Search component (derived from the Spotlight component), as part of the midterm evaluation (along with the bugs this one depends on) -- several things still need to be automated and refined.
The code's probably a bit painful to look at right now :)
Assignee | ||
Comment 7•16 years ago
|
||
This will of course be automated for the final evaluation.
Assignee | ||
Updated•16 years ago
|
Attachment #328341 -
Attachment mime type: text/plain → text/plain;charset=UTF-16
Assignee | ||
Comment 8•16 years ago
|
||
This "unforks" come of the common code between nsWinSearchIntegration.js and nsSpotlightIntegration.js, moving it to content/searchCommon.js.
Attachment #328337 -
Attachment is obsolete: true
Assignee | ||
Comment 9•16 years ago
|
||
eh, sorry for that.
Attachment #329061 -
Attachment is obsolete: true
Assignee | ||
Comment 10•16 years ago
|
||
There are several things still to be fixed, but this is in a working state and I'd like to see this landed after a2 so that we can build on it.
Since this is "turned off" by default, it shouldn't have an effect by default. One effect will be for Mac users with mail.spotlight.enable turned on (carried over from Tb2) -- they'll start seeing generated files again.
Attachment #329064 -
Attachment is obsolete: true
Attachment #329499 -
Flags: superreview?(bienvenu)
Attachment #329499 -
Flags: review?(beckley)
Comment 11•16 years ago
|
||
Comment on attachment 329499 [details] [diff] [review]
Code combined between the two components
> mail/components/search/nsSpotlightIntegration.js:
Mode-line should be 2 for tabs and offset.
> const Cc = Components.classes;
> const Ci = Components.interfaces;
Don't need these as they are in searchCommon.js. Also, there's quite a bit of pre-existing code that doesn't use the consts that could benefit in readability by using them. It would also help to better see similarities between this file and nsWinSearchIntegration.js so as to notice when things can be combined in to searchCommon.js.
> Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
>
> var gPrefBranch = Components.classes["@mozilla.org/preferences-service;1"].getService(Components.interfaces.nsIPrefService).getBranch(null);
These are in both files, so can be moved to searchCommon.js.
> var gIndexMsgsToSpotlight;
Why is this a global when it's only used in one function?
> try {
> gIndexMsgsToSpotlight = gPrefBranch.getBoolPref(gPrefBase + ".enable");
> gLastFolderIndexedUri = gPrefBranch.getCharPref(gPrefBase + ".lastFolderIndexedUri");
> } catch (ex) {}
> if (!gIndexMsgsToSpotlight)
> return;
This code could be moved in to the base searchCommon.js just using a local variable in place of gIndexMsgsToSpotlight.
Since there will be nothing left in Init{Spotlight,WinSearch}Integration() but the SIDump() call, you could make the message to dump be a parameter to InitSupportIntegration
> var gStreamListener = {
This object shares about half of its code between the two versions. It would make sense to make a prototype object in searchCommon.js, and then derive a new class in each of the two files.
> /* XPCOM boilerplate code */
This code is almost all shared as well. The only differences are the description, class ID, and contact ID, which could be passed in as parameters to the constructor. The SIDump call could use the description for its output.
All of these changes are pretty simple, except for the gStreamListener one, which is a little more involved. So if you're wanting to get this checked in soon you could wait to do that until later. With the simple changes, r=me.
Attachment #329499 -
Flags: review?(beckley) → review+
Comment 12•16 years ago
|
||
> const Cc = Components.classes;
> const Ci = Components.interfaces;
this actually breaks spotlight because these are redeclared. I'll try removing them and see if spotlight integration starts working.
Assignee | ||
Comment 13•16 years ago
|
||
(In reply to comment #11)
>
> This code could be moved in to the base searchCommon.js just using a local
> variable in place of gIndexMsgsToSpotlight.
>
> Since there will be nothing left in Init{Spotlight,WinSearch}Integration() but
> the SIDump() call, you could make the message to dump be a parameter to
> InitSupportIntegration
I'd like that code to remain as it is, because the plan is to add to InitWinSearchIntegration, adding support for first-run, testing whether the locations have been included, and so on.
>
> > /* XPCOM boilerplate code */
>
> This code is almost all shared as well. The only differences are the
> description, class ID, and contact ID, which could be passed in as parameters
> to the constructor. The SIDump call could use the description for its output.
Well, that is already a shortened version of the usual boilerplate code, and uses XPCOMUtils, so I like to think of that block as one unit. I'm not sure separating the boilerplate code into three parts -- ns{WinSearch,Spotlight}Integration.js, searchCommon.js, and XPCOMUtils.jsm, would be the best idea.
I've fixed the remaining nits locally (sorry about the redeclaration, David).
Comment 14•16 years ago
|
||
Comment on attachment 329499 [details] [diff] [review]
Code combined between the two components
no need for local var here:
+ var msgHdr = gMsgHdrsToIndex[0];
+ GenerateSupportFile(msgHdr);
same here:
+ var msgHdr = aMsgs.queryElementAt(i, Ci.nsIMsgDBHdr);
+ var file = GetSupportFileForMsgHdr(msgHdr);
We're on the trunk now, can you try uncommenting out the code here, and not use encodeURIComponent?
+ // this should work on the trunk, but not in 2.0
+// messageId = netUtils.escapeString(messageId, 3 /* netUtils.ESCAPE_URL_PATH */);
this was a totally arbitrary choice on my part - maybe we should remove it, or increase it. What do you think? :
+ // ignore stuff after the first 20K or so
if (this.message && this.message.length > 20000)
return 0;
again, no need for local var msgHdr.
+ if (gMsgHdrsToIndex.length > 0)
+ {
+ var msgHdr = gMsgHdrsToIndex[0];
+ GenerateSupportFile(msgHdr);
+ }
why isn't the stream listener shared in the common code?
sr=me, with the above addressed, along with the searchCommon folder iteration changes I sent you...
Attachment #329499 -
Flags: superreview?(bienvenu) → superreview+
Assignee | ||
Comment 15•16 years ago
|
||
Thanks for the fixes David.
Carrying forward r/sr.
Attachment #329499 -
Attachment is obsolete: true
Attachment #329912 -
Flags: superreview+
Attachment #329912 -
Flags: review+
Assignee | ||
Updated•16 years ago
|
Keywords: checkin-needed
Comment 16•16 years ago
|
||
Checking in mail/components/Makefile.in;
/cvsroot/mozilla/mail/components/Makefile.in,v <-- Makefile.in
new revision: 1.14; previous revision: 1.13
done
Checking in mail/components/search/Makefile.in;
/cvsroot/mozilla/mail/components/search/Makefile.in,v <-- Makefile.in
new revision: 1.3; previous revision: 1.2
done
Checking in mail/components/search/nsSpotlightIntegration.js;
/cvsroot/mozilla/mail/components/search/nsSpotlightIntegration.js,v <-- nsSpotlightIntegration.js
new revision: 1.3; previous revision: 1.2
done
RCS file: /cvsroot/mozilla/mail/components/search/nsWinSearchIntegration.js,v
done
Checking in mail/components/search/nsWinSearchIntegration.js;
/cvsroot/mozilla/mail/components/search/nsWinSearchIntegration.js,v <-- nsWinSearchIntegration.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/mail/components/search/content/searchCommon.js,v
done
Checking in mail/components/search/content/searchCommon.js;
/cvsroot/mozilla/mail/components/search/content/searchCommon.js,v <-- searchCommon.js
initial revision: 1.1
done
Keywords: checkin-needed
Target Milestone: --- → Thunderbird 3
Assignee | ||
Updated•16 years ago
|
Attachment #329912 -
Attachment description: [to check in] Addressed review comments, merged folder iteration changes → [checked in] Addressed review comments, merged folder iteration changes
Comment 17•16 years ago
|
||
I started using Mozilla Mail (later Tb) instead of Outlook to reduce integration with OS.
So please ensure I can disable this feature when implemented.
Assignee | ||
Comment 18•16 years ago
|
||
(In reply to comment #17)
> I started using Mozilla Mail (later Tb) instead of Outlook to reduce
> integration with OS.
>
> So please ensure I can disable this feature when implemented.
>
Biju: this will probably be an opt-in feature.
Assignee | ||
Comment 19•16 years ago
|
||
There are several advantages to doing so -- this reduces file size, gets only relevant parts, and in case the message fails to download, will at least generate the headers.
Attachment #332525 -
Flags: review?(bienvenu)
Updated•16 years ago
|
Attachment #332525 -
Flags: review?(bienvenu) → review+
Assignee | ||
Updated•16 years ago
|
Attachment #332525 -
Attachment description: Make the file generation code use getMsgTextFromStream → [to check in] Make the file generation code use getMsgTextFromStream
Assignee | ||
Updated•16 years ago
|
Keywords: checkin-needed
Comment 20•16 years ago
|
||
Comment on attachment 332525 [details] [diff] [review]
[checked in] Make the file generation code use getMsgTextFromStream
Checked in, changeset id 64:9d26fe956df7
Attachment #332525 -
Attachment description: [to check in] Make the file generation code use getMsgTextFromStream → [checked in] Make the file generation code use getMsgTextFromStream
Updated•16 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 21•16 years ago
|
||
Most of it's done, I'm just not satisfied with the UI right now. (The first run dialog, that's non-modal.)
Right now it pops up on first run, with the new account dialog stealing focus immediately after. The way to resolve this seems to be to include code in msgMail3PaneWindow.js to initialize the component only after the new account dialog has been closed. I had wanted to make the component not depend on any other code in mail/, but that seems to be inevitable now.
Asking for review for suggestions. David and Bryan, what do you think?
Attachment #333811 -
Flags: ui-review?(clarkbw)
Attachment #333811 -
Flags: review?(bienvenu)
Assignee | ||
Comment 22•16 years ago
|
||
Bryan, what do you think about merging the default client dialog and this one? This will avoid two dialogs. The dialog can be shown either-or, and elements can be hidden by default and made visible as needed.
The default client dialog can now be called the "Operating System Integration" dialog, I guess.
Comment 23•16 years ago
|
||
It might be good to get this into b1, before Sid turns into a student again.
Flags: blocking-thunderbird3.0b1?
Assignee | ||
Comment 24•16 years ago
|
||
(In reply to comment #23)
> It might be good to get this into b1, before Sid turns into a student again.
>
I'm sure most of the code will be ready for the b1 freeze, but I'm not sure bug 447842 will be resolved before then, and I'd like the code to use that.
There's a startup UI issue right now... the startup is really not structured as well as it could be.
Comment 25•16 years ago
|
||
(In reply to comment #22)
> Bryan, what do you think about merging the default client dialog and this one?
> This will avoid two dialogs. The dialog can be shown either-or, and elements
> can be hidden by default and made visible as needed.
>
> The default client dialog can now be called the "Operating System Integration"
> dialog, I guess.
Sid, that sounds like a good plan lets try that as the dialog point. I think we'll just need a little restructuring of the dialog, no need to hide or show elements. We can probably just use checkboxes to show what's on and what's off.
Updated•16 years ago
|
Flags: blocking-thunderbird3.0b1? → wanted-thunderbird3+
Target Milestone: Thunderbird 3 → Thunderbird 3.0b1
Assignee | ||
Comment 26•16 years ago
|
||
This patch combines:
- Several fixes to the indexing code, including a backoff mode, meant to operate when the integration is disabled
- Quite a bit of integration with Windows, including a C++ component to help and a small executable, meant to run elevated
- A basic UI to work at first run -- this is meant to be combined with the default client dialog later
Attachment #333811 -
Attachment is obsolete: true
Attachment #333811 -
Flags: ui-review?(clarkbw)
Attachment #333811 -
Flags: review?(bienvenu)
Assignee | ||
Comment 27•16 years ago
|
||
Comment on attachment 335162 [details] [diff] [review]
Fixes + integration with Windows + UI
Bryan: since the merging of the default client and the search integration is not a minor undertaking, what do you think of having a separate dialog for now, and then building the merged UI on top of this?
Attachment #335162 -
Flags: ui-review?(clarkbw)
Attachment #335162 -
Flags: review?(beckley)
Assignee | ||
Comment 28•16 years ago
|
||
Comment on attachment 335162 [details] [diff] [review]
Fixes + integration with Windows + UI
Jim: Thanks for consenting to review the patch.
To clarify: WSEnable.exe, built by WSEnable.cpp, is the program that runs with elevated privileges to add the integration. The elevation point is when the "Enable Windows Search" button is clicked. The elevation is done only if required (if either the folders are not in the Windows Search indexing scope, or required registry entries are missing). I'm still looking at a way to get the shield icon to display next to the button.
Attachment #335162 -
Flags: superreview?(bienvenu)
Attachment #335162 -
Flags: review?(jmathies)
Comment 29•16 years ago
|
||
(In reply to comment #26)
> - A basic UI to work at first run -- this is meant to be combined with the
> default client dialog later
Couldn't we do without any UI at all (or at most a pref in the advanced options)? As long as we can detect whether Windows Search is installed or we're on Vista, we could just enable integration when that's the case and use a hidden pref for disabling auto-detection/integration.
Assignee | ||
Comment 30•16 years ago
|
||
(In reply to comment #29)
> (In reply to comment #26)
> > - A basic UI to work at first run -- this is meant to be combined with the
> > default client dialog later
>
> Couldn't we do without any UI at all (or at most a pref in the advanced
> options)? As long as we can detect whether Windows Search is installed or we're
> on Vista, we could just enable integration when that's the case and use a
> hidden pref for disabling auto-detection/integration.
>
The trouble with that is that enabling or disabling the integration requires a UAC prompt. (Try adding file locations through the UI on Vista.) It's extremely jarring for the user to be greeted with a UAC prompt on first run without warning.
Windows Mail, which uses a method quite similar to what we're doing here, doesn't have a problem because it doesn't support multiple profiles in custom locations.
Assignee | ||
Updated•16 years ago
|
Whiteboard: [needs review bienvenu][needs review beckley][needs review jmathies][needs review clarkbw][on schedule modulo UI]
Comment 31•16 years ago
|
||
Comment on attachment 335162 [details] [diff] [review]
Fixes + integration with Windows + UI
Looks good. Just some minor comments below. With that r=me.
mail/components/search/content/searchIntegrationDialog.js:
Why have this file at all? Just have searchIntegrationDialog.xul call window.arguments[0].callback() in ondialogaccept and ondialogcancel. You can then also remove the line from mail/components/search/jar.mn referring to searchIntegrationDialog.js.
mail/components/search/nsMailWinSearchHelper.cpp:
> ISearchManager* searchManager;
Make that a nsRefPtr<ISearchManager> and you won't have to worry about calling Release() on it.
> subdir->AppendNative(relativeStr);
> subdir->GetPath(subdirPath);
Need to check for errors here.
>NS_IMETHODIMP nsMailWinSearchHelper::SetFANCIBit(nsIFile* aFile, PRBool aBit, PRBool aRecurse)
...
> aFile->Exists(&exists);
...
> aFile->GetPath(filePath);
...
> aFile->IsDirectory(&isDirectory)
...
> aFile->GetDirectoryEntries(getter_AddRefs(children));
You've got several calls on aFile that you don't check to see if they failed or not.
> nsCOMPtr<nsISupports> childSupports;
> children->GetNext(getter_AddRefs(childSupports));
> nsCOMPtr<nsIFile> childFile(do_QueryInterface(childSupports));
You can actually bypass the indirect ISupports and go straight to the interface you want. You also should check for failure here as well. Would look like this:
nsCOMPtr<nsIFile> childFile;
rv = children->GetNext(getter_AddRefs(childFile));
NS_ENSURE_SUCCESS(rv, rv);
> IApplicationAssociationRegistration *pAAR;
nsRefPtr<> this to ensure release. There's a couple of these in the file. I realize that the usage of these is pretty simple, but it's good to just make a habit of using the wrapper classes, and could come in to play if the code gets updated later and becomes more complicated.
> mCurProcD->Append(NS_LITERAL_STRING("WSEnable.exe"));
Similar here as to above, there's a few calls in mCurProcD and mProfD that don't check for failure.
> HWND hWnd = GetForegroundWindow();
GetForegroundWindow() can return NULL. I can't imagine that would cause a problem for the UAC prompt, since it's global to all windows, but might be worth testing to see what it does.
mail/components/search/nsWinSearchIntegration.js:
> var enabled;
> try {
> enabled = gPrefBranch.getBoolPref(gPrefBase + ".enable");
> gLastFolderIndexedUri = gPrefBranch.getCharPref(gPrefBase + ".lastFolderIndexedUri");
> } catch (ex) {}
...
> if (enabled === undefined)
> // First run has to be handled after the main mail window is open
> return true;
I think that comment needs to be expanded upon. It took me a little bit to figure out what exactly what was going on. enabled will only be undefined if the pref is not defined (exception thrown on the getBoolPref call), and the setting of that pref happens in a different function.
mail/components/search/wsenable/WSEnable.cpp:
>static const int sFolderCount = 3;
>static const int sRegKeyCount = 5;
You can use the _countof () macro instead of putting hand-counted values in there.
> ISearchManager* searchManager;
> ISearchCatalogManager* catalogManager;
I don't know if you want to bring ATL in to this (maybe it is already?), but you could use the CComPtr<T> class and stop worrying/potentially forgetting about releasing.
> if (wcscmp(argv[0], L"1") == 0)
> else if (wcscmp(argv[0], L"0") == 0)
How about the more simple *argv[0] == L'1' and *argv[0] == L'0'?
Attachment #335162 -
Flags: review?(beckley) → review+
Comment 32•16 years ago
|
||
Comment on attachment 335162 [details] [diff] [review]
Fixes + integration with Windows + UI
very nice. sr=bienvenu, with some nits:
+ HRESULT hr = HRESULT_FROM_WIN32(dwRet);
+ if (FAILED(hr))
+ return NS_ERROR_FAILURE;
+
+ return NS_OK;
+}
maybe :
return (FAILED(HRESULT_FROM_WIN32(dwRet)) ? NS_ERROR_FAILURE : NS_OK;
or the more positive SUCCEEDED(...) :-)
searchIntegrationDialog.xul
shouldn't that just be 2008 and initial developer you? Or did you borrow it from somewhere?
+ regKey.close();
+ if (!valuePresent) return false;
the return should be on its own line.
Since I'm not on Vista, I can't really test this out, right?
Attachment #335162 -
Flags: superreview?(bienvenu) → superreview+
Assignee | ||
Comment 33•16 years ago
|
||
(In reply to comment #32)
>
> searchIntegrationDialog.xul
> shouldn't that just be 2008 and initial developer you? Or did you borrow it
> from somewhere?
I did, from the default client dialog XUL file. :)
> Since I'm not on Vista, I can't really test this out, right?
>
Yes, that's right. However it should build successfully, whether you're using the Vista SDK or not (one should have |ac_add_options --disable-vista-sdk-requirements| in mozconfig in case one doesn't have the Vista SDK. I disable the whole search/ directory for Windows in that case.)
I've also asked Jim to review it, to cover the Vista specific parts.
Updated•16 years ago
|
Whiteboard: [needs review bienvenu][needs review beckley][needs review jmathies][needs review clarkbw][on schedule modulo UI] → [needs review jmathies][needs review clarkbw][on schedule modulo UI]
Updated•16 years ago
|
Priority: -- → P2
Comment 34•16 years ago
|
||
Looks like there's a few places in GetFoldersInCrawlScope where you're not cleaning up ISearchManager* searchManager after you're finished. You released it on one failure case but missed it for the rest of the method.
> pAAR->QueryAppIsDefault(L".wdseml", AT_FILEEXTENSION, AL_EFFECTIVE, L"Thunderbird", aResult);
Let's put the app name up on top of the file as we do in the other app registration code so it's easy to find. Please use the same name so it can be found in a search.
Looks pretty tight overall. You're doing a lot of registration of app / file / mime types but only removing a single file association on uninstall. Is there any way we can reset some of that stuff (like the default app) on uninstall?
Updated•16 years ago
|
Attachment #335162 -
Flags: ui-review?(clarkbw) → ui-review+
Comment 35•16 years ago
|
||
Comment on attachment 335162 [details] [diff] [review]
Fixes + integration with Windows + UI
Looks good for now. We've already talked quite a bit about getting the default client and search integration dialog merge after this initial piece gets in.
Assignee | ||
Comment 36•16 years ago
|
||
Thanks for reviewing, Jim.
(In reply to comment #34)
> Looks like there's a few places in GetFoldersInCrawlScope where you're not
> cleaning up ISearchManager* searchManager after you're finished. You released
> it on one failure case but missed it for the rest of the method.
OK, I've switched to nsRefPtr anyway.
>
> > pAAR->QueryAppIsDefault(L".wdseml", AT_FILEEXTENSION, AL_EFFECTIVE, L"Thunderbird", aResult);
>
> Let's put the app name up on top of the file as we do in the other app
> registration code so it's easy to find. Please use the same name so it can be
> found in a search.
OK.
>
> Looks pretty tight overall. You're doing a lot of registration of app / file /
> mime types but only removing a single file association on uninstall. Is there
> any way we can reset some of that stuff (like the default app) on uninstall?
I think the .wdseml extension removal *should* take care of most of the additions. There's one more in KindMap that should be taken care of, but isn't right now.
As for indexed locations, I'm not sure about how to handle them on uninstall. How do you figure out all the profiles for each user and call the function for each of them? The user might just also have another install of Tb (I'm not sure how common this is), in which case he wouldn't like the locations to be de-registered.
Updated•16 years ago
|
Attachment #335162 -
Flags: review?(jmathies) → review+
Comment 37•16 years ago
|
||
If the data sticks around, leaving them in there sounds fine. r+ with those other changes.
Whiteboard: [needs review jmathies][needs review clarkbw][on schedule modulo UI] → [on schedule modulo UI]
Assignee | ||
Comment 38•16 years ago
|
||
Carrying forward reviews by beckley and jmathies, ui-review by clarkbw and sr by bienvenu.
Attachment #335162 -
Attachment is obsolete: true
Attachment #337314 -
Flags: superreview+
Attachment #337314 -
Flags: review+
Assignee | ||
Updated•16 years ago
|
Attachment #337314 -
Attachment description: addressed review comments → [to checkin] addressed review comments
Assignee | ||
Updated•16 years ago
|
Keywords: checkin-needed
Comment 39•16 years ago
|
||
Comment on attachment 337314 [details] [diff] [review]
[checked in] addressed review comments
Checked in, changeset id: 279:e7a685e6761a
Attachment #337314 -
Attachment description: [to checkin] addressed review comments → [checked in] addressed review comments
Updated•16 years ago
|
Keywords: checkin-needed
Comment 40•16 years ago
|
||
I can't see anything related to Windows Search in interface of pre-b1 builds I'm using; currently this:
Mozilla/5.0 (Windows; U; Windows NT 5.1; lt; rv:1.9.1b1pre) Gecko/20080916030709 Shredder/3.0b1pre
Is this feature enabled in these builds already? I'm running XP with SP3 and WS4.0...
Assignee | ||
Comment 41•16 years ago
|
||
(In reply to comment #40)
> I can't see anything related to Windows Search in interface of pre-b1 builds
> I'm using; currently this:
> Mozilla/5.0 (Windows; U; Windows NT 5.1; lt; rv:1.9.1b1pre)
> Gecko/20080916030709 Shredder/3.0b1pre
>
> Is this feature enabled in these builds already? I'm running XP with SP3 and
> WS4.0...
It's Vista only for now.
Comment 42•16 years ago
|
||
Then when will it work in XP? I hope it will make it into the final version, or maybe even one of the betas?
Assignee | ||
Comment 43•16 years ago
|
||
(In reply to comment #42)
> Then when will it work in XP? I hope it will make it into the final version, or
> maybe even one of the betas?
The problem with the XP version of Windows Search is that it lacks a property handler for MIME messages. On Vista, you can search and sort by sender, subject, date received, etc, and have them displayed neatly as columns in your search. (We use the Microsoft provided handler for this.) The XP version doesn't actually have one. Notice that on XP, you can't use Windows Search to properly search through Windows Live Mail messages either. You have to do it from within the WLM interface.
Comment 44•16 years ago
|
||
I think that even searching through just body would benefit.
On the other hand, I just searched for my name, and between the results came out a few Word documents that only contain my name in metadata (the Author tag). Is it possible in XP to at least index those tags, even if they wouldn't be displayed?
Assignee | ||
Comment 45•16 years ago
|
||
(In reply to comment #44)
> I think that even searching through just body would benefit.
>
> On the other hand, I just searched for my name, and between the results came
> out a few Word documents that only contain my name in metadata (the Author
> tag).
That's because the XP version has a property handler for Word documents. It doesn't have one for MIME messages.
> Is it possible in XP to at least index those tags, even if they wouldn't
> be displayed?
Yes, that's what currently happens on XP. (It's slightly confusing, but XP has a filter for MIME messages, but not a property handler. The filter does the indexing, the property handler handles the display of results.) The check for Vista is hardcoded at the moment, but I guess it can be changed to possibly use a hidden pref instead to make it work on XP.
Comment 46•16 years ago
|
||
Well, like I said, I think that even if we index/display only the body part, it would still be a benefit over not indexing at all for XP users.
Perhaps that check could be removed at all, with an appropriate note added in release notes and/or the UI that enables this feature?
Comment 47•16 years ago
|
||
> with an appropriate note added in
> release notes and/or the UI that enables this feature?
If we haven't already, we should probably make a clear note on the release notes that this is Vista only.
Assignee | ||
Comment 48•16 years ago
|
||
As discussed with Bryan, a better UI is needed. Pushing to beta 2 for the UI.
Keywords: relnote
Whiteboard: [on schedule modulo UI] → [relnote at comment 47][better UI needed]
Target Milestone: Thunderbird 3.0b1 → Thunderbird 3.0b2
Comment 49•16 years ago
|
||
Is this implementation valid for Copernic Desktop Search?
Assignee | ||
Comment 50•16 years ago
|
||
(In reply to comment #49)
> Is this implementation valid for Copernic Desktop Search?
No.
Assignee | ||
Comment 51•16 years ago
|
||
- I'd forgot to include the xpt
- Doh, need to unset the File Attribute Not Content Indexed bit for the profile directory and all subdirectories.
Assignee | ||
Updated•16 years ago
|
Attachment #339997 -
Flags: review?(bienvenu)
Updated•16 years ago
|
Attachment #339997 -
Flags: review?(bienvenu) → review+
Assignee | ||
Updated•16 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•16 years ago
|
Attachment #339997 -
Attachment description: A couple more fixes → [to check in] A couple more fixes
Comment 52•16 years ago
|
||
Comment on attachment 339997 [details] [diff] [review]
[checked in] A couple more fixes
Checked in, changeset id: 406:1f2f13351b1f
Attachment #339997 -
Attachment description: [to check in] A couple more fixes → [checked in] A couple more fixes
Updated•16 years ago
|
Keywords: checkin-needed
Comment 53•16 years ago
|
||
Can someone who has used vista search write one or more tests for litmus? If you don't have admin access to litmus I'll gladly take your text and create the test.
Nothing verbose, just list a few steps of what testers should do and what they should see as a result. Perhaps 4 distinct tests?
1. enable search (and/or what they see on first startup after install)
2. <a test to verify that it's working correctly>
3. <a test checking for a problem that has already fixed by development, i.e. should NOT be seen>
4. disable search
Flags: in-litmus?
Assignee | ||
Comment 54•16 years ago
|
||
(In reply to comment #53)
> Can someone who has used vista search write one or more tests for litmus? If
> you don't have admin access to litmus I'll gladly take your text and create the
> test.
>
> Nothing verbose, just list a few steps of what testers should do and what they
> should see as a result. Perhaps 4 distinct tests?
> 1. enable search (and/or what they see on first startup after install)
> 2. <a test to verify that it's working correctly>
There will probably be 4-5 tests that will be needed to verify that it is working correctly, since there are several related parts to the integration.
> 4. disable search
This hasn't been really implemented yet.
Comment 55•16 years ago
|
||
Hmm, it's not quite working properly for me. It does appear to have created the .wdseml files properly, but they're not all showing up in the start menu. This is on Vista SP1 - 64-bit, UAC turned off :)
When I search from the start menu, I can see messages from newsgroups but not messages from my IMAP folders. Additionally, the results show only the message ID (presumably from the file name), not the Subject or anything else.
When I search from explorer (Windows+F), I see messages from both IMAP folders and Newsgroups, but the From/Subject/Date Received/To Names are all blank!
Looking in the .wdseml files, they do look valid. I can attach one if needed, though. The indexing control panel shows that wdseml files are being processed with the MIME filter. It's currently rebuilding the index at my behest, but it looks like the problem is still occurring. After copying one of the results to another of my indexed folders and starting rebuilding the index, it also shows up with blank properties.
This is starting to look like it's Windows's fault, but I should probably mention it here anyway in case it's useful.
Assignee | ||
Comment 56•16 years ago
|
||
Interesting -- could you confirm if HKEY_LOCAL_MACHINE\\SOFTWARE\\Microsoft\\Windows\\CurrentVersion\\PropertySystem\\PropertyHandlers\\.wdseml (those should be single backslashes, of course) has its "default" value {5FA29220-36A1-40f9-89C6-F4B384B7642E}? It'd also be great if you could ensure that ..\\PropertyHandlers\\.eml has the same default value. It seems like the property handler isn't kicking in.
Please do attach a .wdseml file which isn't working for you.
Assignee | ||
Comment 57•16 years ago
|
||
OK, now I'm thinking it's a Windows 64-bit problem, and that some registry locations have been virtualized because the Windows Search enabler is a 32-bit program. Thanks for the bug report!
Assignee | ||
Comment 58•16 years ago
|
||
per http://msdn.microsoft.com/en-us/library/aa384129(VS.85).aspx
We also disable CheckRegistryKeys as that won't work on 64-bit Windows right now. As a result, we'll just harmlessly elevate every time we're enabled.
Assignee | ||
Updated•16 years ago
|
Attachment #341893 -
Flags: review?(bienvenu)
Comment 59•16 years ago
|
||
Comment on attachment 341893 [details] [diff] [review]
[checked in] Fix for 64-bit Windows
can we file a follow up bug to handle the ToDo items and mark it in b1?
Attachment #341893 -
Flags: review?(bienvenu) → review+
Updated•16 years ago
|
Attachment #341893 -
Attachment description: Fix for 64-bit Windows → [checked in] Fix for 64-bit Windows
Comment 60•16 years ago
|
||
Comment on attachment 341893 [details] [diff] [review]
[checked in] Fix for 64-bit Windows
Checked in, changeset id: 526:e1a4229ff535
Also checked in for Shredder alpha 3 respin (a=Standard8,bienvenu), changeset id: 490:504bc8871087
Assignee | ||
Comment 61•16 years ago
|
||
(In reply to comment #59)
> (From update of attachment 341893 [details] [diff] [review])
> can we file a follow up bug to handle the ToDo items and mark it in b1?
Yes, that's a good idea. I'll get to this, possibly today.
Comment 62•16 years ago
|
||
(In reply to comment #56)
> Interesting -- could you confirm if
> HKEY_LOCAL_MACHINE\\SOFTWARE\\Microsoft\\Windows\\CurrentVersion\\PropertySystem\\PropertyHandlers\\.wdseml
> (those should be single backslashes, of course) has its "default" value
> {5FA29220-36A1-40f9-89C6-F4B384B7642E}? It'd also be great if you could ensure
> that ..\\PropertyHandlers\\.eml has the same default value. It seems like the
> property handler isn't kicking in.
>
> Please do attach a .wdseml file which isn't working for you.
This key didn't exist. You guessed right, though, it was there under
Wow6432Node. It's working with the key added, thanks!
I'm not sure how to disable the indexing so that I can enable it again and verify that the patch works. (It would be nice if it showed up in the Indexing Options control panel, too :)
Assignee | ||
Comment 63•16 years ago
|
||
(In reply to comment #62)
>
> This key didn't exist. You guessed right, though, it was there under
> Wow6432Node. It's working with the key added, thanks!
>
> I'm not sure how to disable the indexing so that I can enable it again and
> verify that the patch works. (It would be nice if it showed up in the Indexing
> Options control panel, too :)
build3's currently being respun. It has the fix.
http://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/3.0a3-candidates/build3/
Once it's ready, please download and install it, then in the config editor, reset mail.winsearch.enable. You should get the prompt again on restarting Tb. It's best if you also remove the key you just added before doing this, and ensure that the key's added back when you click Enable, to verify that the fix is working correctly.
Thanks for catching this.
Assignee | ||
Updated•16 years ago
|
Keywords: relnote
Whiteboard: [relnote at comment 47][better UI needed] → [better UI needed]
Assignee | ||
Comment 64•16 years ago
|
||
Unfortunately I can't see myself having time to improve this for beta 1. I think I should have time for beta 2.
Target Milestone: Thunderbird 3.0b1 → Thunderbird 3.0b2
Assignee | ||
Updated•16 years ago
|
Flags: in-litmus? → in-litmus+
Assignee | ||
Updated•16 years ago
|
Target Milestone: Thunderbird 3.0b2 → Thunderbird 3.0b3
Assignee | ||
Updated•16 years ago
|
Whiteboard: [better UI needed] → [better UI in bug 467116]
Assignee | ||
Updated•16 years ago
|
Target Milestone: Thunderbird 3.0b3 → Thunderbird 3.0b4
Comment 66•15 years ago
|
||
The Thunderbird drivers wish to release Thunderbird 3 as soon as possible. As a
result, we feel that this bug shouldn't stand in the way of all the other good
work getting into the hands of users sooner rather than later. Therefore we are
retargeting it for 3.1. See http://ccgi.standard8.plus.com/blog/archives/242
for more details. The 3.1 release is expected to be a quick release soon after
Thunderbird 3.
Flags: blocking-thunderbird3.1+
Flags: blocking-thunderbird3-
Flags: blocking-thunderbird3+
Updated•15 years ago
|
Component: General → Search
QA Contact: general → search
Comment 67•15 years ago
|
||
Just FYI information, Thunderbird 3 RC2 do integrate well with Windows Desktop Search 4. Integration have been tested on Windows XP Professional SP3 32-bit, on Windows Vista Ultimate SP2 32-bit and on Windows 7 Ultimate 32-bit.
Comment 68•15 years ago
|
||
Given that 3.1 is mostly about fixing stuff that blocks folks from upgrading from Thunderbird 2, I suspect this doesn't want to block 3.1 at all, so I'm removing the blocking-thunderbird3.1+ flag for now. That said, it's hard for me to tell exactly what work remains to be done here just from skimming recent comments. Sid0, can you summarize the end-user visible changes, the work that's still needed, and what the risk is? For now, I'm going to set this as wanted-thunderbird+. If you think it should really block Thunderbird 3.1, you're welcome to renominate.
Flags: blocking-thunderbird3.1+ → wanted-thunderbird+
Comment 69•15 years ago
|
||
Please make dependent on 567073.
Comment 70•15 years ago
|
||
Please add 567212.
Comment 71•6 years ago
|
||
Time to close this. What was done was done.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•