Closed Bug 537820 Opened 10 years ago Closed 10 years ago

quick search in smart folders/saved searches should prefer offline when it has a choice

Categories

(Thunderbird :: Search, defect)

x86
Windows 7
defect
Not set

Tracking

(blocking-thunderbird3.0 needed, thunderbird3.0 .1-fixed)

RESOLVED FIXED
Thunderbird 3.1a1
Tracking Status
blocking-thunderbird3.0 --- needed
thunderbird3.0 --- .1-fixed

People

(Reporter: Bienvenu, Assigned: Bienvenu)

References

(Depends on 1 open bug)

Details

(Keywords: perf, regression)

Attachments

(2 files)

quick search shouldn't run online searches, when possible. The code in searchSpec.js defers to the online search property of the folder when it has a choice between online and offline search; this is not the right choice for quick searches. I may need to better understand why this change was made in order to fix this w/o regressing the original bug. It's also unclear to me that I can tell in searchSpec.js if I'm doing a quick search or not (though views should also happen offline, if possible).
bug 511131 is what caused this regression, so I need to make sure that advanced search still does the right thing after I come up with a patch for this.
(In reply to comment #1)
> bug 511131 is what caused this regression, so I need to make sure that advanced
> search still does the right thing after I come up with a patch for this.

The intention of bug 511131 was that it deferred to offline search unless the search criteria were only valid for online search.
blocking-thunderbird3.0: ? → needed
Attached patch proposed fixSplinter Review
I'd really like to get this in for 3.01...This restores a bit of the old logic, though I think the old code was a bit broken anyway. Basically, if we're doing a quick search or have a view applied, we prefer to do offline search, if possible. I checked, and both single and multiple folder saved searched use virtualFolderTerms, and quick search uses _userTerms, and views use _viewTerms.

I've requested sr from asuth, just so it shows up on his rader, but I think he's going to be out for a few more days.
Attachment #420089 - Flags: superreview?(bugmail)
Attachment #420089 - Flags: review?(kent)
I'm not sure that I understand the premise of this particular bug.

The original code, as implemented in bug 511131, had to decide what to do in cases where a virtual folder was defined that uses an online search, and the user did a quick search on top of that. In that case, it honored the user request and also used online search in the combination saved/quick search.

Is this bug asking to change that decision? If so, why is this so urgent, as it must be fairly rare for users to define saved searches as online.

Or is the belief that all quick searches are now online? If so, what is the evidence for that?

So I need to know what this bug is purporting to fix before I can effectively review it.
smart mailboxes are by default online, so any quick search or view in a smart folder gets executed online. There's no reason to do that, since we've already got all the data, as long as all terms are searchable offline.

I've got patches to make smart mailboxes offline by default, but the core problem should be fixed as well.
It seems to me that the "core problem" here is that smart folders are defined to be online, not that quick searches honor that request.

But I understand that your desire is that quick searches prefer offline, and I'll review the code with that objective.
Comment on attachment 420089 [details] [diff] [review]
proposed fix

I tested this patch using Wireshark to see the real IMAP calls. Before the patch, an "Unread" view of a smart folder did an online search, but that no longer happens with the patch applied. If I take the folder offline, and add a custom view that searches the body, then the view still does an online search.

For quick search, I did a similar test using a body filter, and a subject filter. Both worked as expected.

All of these tests worked as expected with the patch applied.

>+        dump("scope = " + scope + "\n");

I assume you did not intend to leave this dump. Please remove.

r=me with this dump removed.
Attachment #420089 - Flags: review?(kent) → review+
thx, Kent, I'll remove the dump.
Attached patch fix for 3.01Splinter Review
patch without the dump, for if and when we can land this for 3.01
Attachment #420143 - Flags: approval-thunderbird3.0.1?
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.1a1
Duplicate of this bug: 535358
Comment on attachment 420089 [details] [diff] [review]
proposed fix

clearing sr request...
Attachment #420089 - Flags: superreview?(bugmail)
Attachment #420143 - Flags: approval-thunderbird3.0.1? → approval-thunderbird3.0.1+
Duplicate of this bug: 504419
You need to log in before you can comment on or make changes to this bug.