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)

x86
Windows Vista
enhancement

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+
Details | Diff | Splinter Review
(deleted), patch
Bienvenu
: review+
Details | Diff | Splinter Review
(deleted), patch
rain1
: review+
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.
Blocks: 430583
Blocks: gsoc
No longer blocks: 430583
Confirming due to approved GSoC work.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Blocks: 369283
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.
Status: NEW → ASSIGNED
Assignee: nobody → sid1337
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
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)
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.
Depends on: 433405
Depends on: 433697
Depends on: 433740
I'm posting somewhat regular updates to the bug status to http://sid0.blogspot.com .
Depends on: 435290
Depends on: 436792, 436793
Depends on: 436880
Depends on: 437848
Depends on: 438335
Depends on: 439225
Depends on: 439494
Depends on: 441043
Depends on: 441048
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 :)
This will of course be automated for the final evaluation.
Attachment #328341 - Attachment mime type: text/plain → text/plain;charset=UTF-16
Attached patch Snapshot of work so far (obsolete) (deleted) — Splinter Review
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
Attached patch fixed license block (obsolete) (deleted) — Splinter Review
eh, sorry for that.
Attachment #329061 - Attachment is obsolete: true
Depends on: 444950
Attached patch Code combined between the two components (obsolete) (deleted) — Splinter Review
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)
Depends on: 445199
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+
> 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.
(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).
Depends on: 445554
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+
Thanks for the fixes David. Carrying forward r/sr.
Attachment #329499 - Attachment is obsolete: true
Attachment #329912 - Flags: superreview+
Attachment #329912 - Flags: review+
Keywords: checkin-needed
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
Attachment #329912 - Attachment description: [to check in] Addressed review comments, merged folder iteration changes → [checked in] Addressed review comments, merged folder iteration changes
Blocks: TB2SM
Depends on: 446047
Depends on: 447860
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.
(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.
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)
Attachment #332525 - Flags: review?(bienvenu) → review+
Attachment #332525 - Attachment description: Make the file generation code use getMsgTextFromStream → [to check in] Make the file generation code use getMsgTextFromStream
Keywords: checkin-needed
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
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)
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.
It might be good to get this into b1, before Sid turns into a student again.
Flags: blocking-thunderbird3.0b1?
Depends on: 447842
(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.
Keywords: uiwanted
Depends on: 432920
(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.
Flags: blocking-thunderbird3.0b1? → wanted-thunderbird3+
Target Milestone: Thunderbird 3 → Thunderbird 3.0b1
Attached patch Fixes + integration with Windows + UI (obsolete) (deleted) — Splinter Review
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)
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)
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)
(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.
(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.
Whiteboard: [needs review bienvenu][needs review beckley][needs review jmathies][needs review clarkbw][on schedule modulo UI]
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 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+
(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.
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]
Priority: -- → P2
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?
Attachment #335162 - Flags: ui-review?(clarkbw) → ui-review+
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.
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.
Attachment #335162 - Flags: review?(jmathies) → review+
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]
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+
Attachment #337314 - Attachment description: addressed review comments → [to checkin] addressed review comments
Keywords: checkin-needed
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
Depends on: 454399
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...
(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.
Then when will it work in XP? I hope it will make it into the final version, or maybe even one of the betas?
(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.
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?
(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.
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?
> 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.
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
Depends on: 455689
Depends on: 455932
Is this implementation valid for Copernic Desktop Search?
(In reply to comment #49) > Is this implementation valid for Copernic Desktop Search? No.
Depends on: 456247
Depends on: 456440
Depends on: 456548
- 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.
Attachment #339997 - Flags: review?(bienvenu)
Attachment #339997 - Flags: review?(bienvenu) → review+
Keywords: checkin-needed
Attachment #339997 - Attachment description: A couple more fixes → [to check in] A couple more fixes
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
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?
(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.
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.
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.
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!
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.
Attachment #341893 - Flags: review?(bienvenu)
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+
Attachment #341893 - Attachment description: Fix for 64-bit Windows → [checked in] Fix for 64-bit Windows
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
(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.
(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 :)
(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.
Depends on: 458935
Keywords: relnote
Whiteboard: [relnote at comment 47][better UI needed] → [better UI needed]
Depends on: 459419
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
Depends on: 466732
Depends on: 467111
Depends on: 467116
Depends on: 467117
Depends on: 467118
Flags: in-litmus? → in-litmus+
Depends on: 467275
Depends on: 470145
Target Milestone: Thunderbird 3.0b2 → Thunderbird 3.0b3
If this blocks a blocking bug, it blocks.
Flags: blocking-thunderbird3+
Whiteboard: [better UI needed] → [better UI in bug 467116]
Target Milestone: Thunderbird 3.0b3 → Thunderbird 3.0b4
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+
Component: General → Search
QA Contact: general → search
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.
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+
Please make dependent on 567073.
Please add 567212.
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.

Attachment

General

Created:
Updated:
Size: