511131 - Advanced search uses constrained online search scope, but is actually searching offline

Status: RESOLVED FIXED

(MailNews Core :: Search, defect)

Description

[Kent James (:rkent)]

The new implementation logic for determining which search scope to use in advanced search from bug 474701, as I understand it, tries to figure out the scope after the user has selected the search terms, looking at whether body search is enabled to choose whether to search online. The search validity in the search term editor presented to the user, which determines which search terms they are allowed to select, is the constrained online search validity table. While this allows selection of body search, there are many other search terms and operators that are not available in online search, but are available offline. The advanced search dialog is the primary entry point for virtual folders, so this presents a small subset of TB's true search capability for virtual folders to the user. While there is a workaround available (save a search, then edit it) that is not obvious to most uses.

I really think that the advanced search dialog is the place where all of the traditional search capability of TB (which AFAIK is still intended as being the primary creation path for virtual search folders) is presented to the user.

This bug may turn out to be a dup of 441531. As an alternative, the proper search validity table should be chosen based on the offline capability of the server, particularly after bug 127250 is landed which restores body search capability to IMAP in the default configuration (download messages for offline use).

Comment 1

[Kent James (:rkent)]

My current thought on this is that we should detect whether the user has enabled offline storage of messages (is there a shorter phrase for that?). Then we should set the search scope to offlineMail if yes, otherwise onlineMail. That's pretty much what the downstream code is doing, right? This still leaves users who don't enable offline messages without the benefit of the additional non-body search capabilities of the onlineManual validity table, but fixing that is really bug 441531.

But I am still quite nervous about the new code that tries to decide on the search scope after the user has input search attributes and operators. I really don't think we should be changing the search scope at that point for situations where the user has explicitly set search attributes and operators.

I'm going to request blocking on this just to express my view of the importance of this issue, even though I expect that if I don't fix it, then it probably won't be fixed.

Comments?

Comment 2

[David :Bienvenu]

auto-detect is fine, and we should do it (and we can tell on a per-server basis, I believe, if autosync has been turned on, and on a per-folder basis if a  folder has been configured for offline use).

Having a simple check box for the user to override where the search happens wouldn't hurt either...

Comment 3

[Kent James (:rkent)]

I'm not familiar with the issue with autosync. How does a disabled autosync affect the offline download? Where is the UI to set autosync?

Comment 4

[David :Bienvenu]

tools | account settings | synchronization and disk space, keep messages for this account on this computer. Only applies to imap servers.

Comment 5

[Kent James (:rkent)]

The question of which search attributes and operators to allow is not easy because of the possibility of cross-server searches, where we are trying to use one set of search terms to search folders in servers that might have vastly different search capabilities. This is different from the case in filters, where filters apply to a single server that in most cases has a single set of search capabilities. (But is similar to the problem we would face if we tried to supply a global filter search possibility).

Comment 0 described the problems with the currently allowed search attributes and operators in the advanced search dialog. There is a similar type of issue for the virtual folder editing dialog as well. Even though the virtual folder dialog is often initialized from search terms in the search dialog, virtual folders take a very different approach to presentation of allowed search attributes and operators. Virtual folder editing always uses the offlineMail search scope, which is the scope with the most capability. So users are getting little guidance as to what are valid search attributes and operators when creating or editing virtual folders, and can easily add search terms that are invalid in a particular search scope. Some examples of this would be trying to use body search terms in IMAP or news folders that do not have the bodies offline, or adding To In Addressbook to a News search.

In the interest of keeping this bug size manageable, I won't deal with the problems with the virtual folder dialog here. But the result will be a continuation of the mismatch that is presented between search capabilities shown in the advanced search dialog, and in the virtual folder dialog.

For the advanced search dialog, I think that the correct thing to do will be to select a single folder as the primary folder for a search, and use its search validity. This will match fairly well cases where the user selects a folder, and tries to search it and its subfolders. That will still result in the possibility of adding bogus search terms when the user adds additional folders to the search, but in the normal case where the user selects a folder and searches it, should allow us to show fairly precisely what search attributes and operators are valid. I also want to include the offline/online choice from bug 441531 in this bug.

Comment 6

[Kent James (:rkent)]

Attachment: WIP but basically works

Here's first pass at this. Since I now specify online or offline for the search, plus am being careful to use the correct validity table, I wanted the actual search code to honor my choice when possible, yet just make it work otherwise. So I scan through the validity tables and decide whether a combination of virtual, user, and view terms can be done with the user's choice. If so, I use the user's choice. If not, I use whatever will work.

So I'm testing online search here. Unfortunately online Imap search, while it works, is generating lots of database errors. News search (on news.mozilla.org) always returns a protocol error, so gives me nothing useful. I'm not sure either of these are my fault, but I need to understand them and possibly fix them before we expose the online search capability more directly.

The choice of online search is also propagated to the virtual folder dialog when the search is saved.

Comment 7

[Kent James (:rkent)]

Attachment: Cleanup for review

This patch allows the user to select online search, and honors that request if it can. But it will silently switch to offline if the validity tables say that's the only way it will work.

I've also adds a fairly complex set of validity tables for news. The goal here is to not show junk in most cases (since it is an experimental feature), and not show body when it won't work. We should probably also take the same approach to RSS at some point (right now it just shows the same search terms as mail).

Note this has a string which could affect the deadlines.

SM has its own version of this. The patch for this bug does not attempt to bring SM in line with TB, but should not regress them either. I think we should probably port this to SM, replacing their method. This patch also implements most of the request in bug 441531 as it affects TB. Perhaps bug 441531 can be morphed into the bug to bring SM in alignment with this patch, as the request of that bug was a common UI.

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 395995 [details] [diff] [review]
Cleanup for review

This should probably have ux review as well.

Comment 9

[Kent James (:rkent)]

Clark, you already commented on this in bug 441531 comment 9. I did not however include the long explanation of "Search online" that the virtual folder dialog has, mostly because I think it is slightly misleading. Instead, we simply default to having "Search online" cleared. Really only people who know what they are doing, and have some legitimate reason for doing so, should select "Search online". The most common reason, I suppose, would be to get a body search done for cases where people are not downloading message bodies locally.

Comment 10

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #9)
> Clark, you already commented on this in bug 441531 comment 9. I did not however
> include the long explanation of "Search online" that the virtual folder dialog
> has, mostly because I think it is slightly misleading. Instead, we simply
> default to having "Search online" cleared. Really only people who know what
> they are doing, and have some legitimate reason for doing so, should select
> "Search online". The most common reason, I suppose, would be to get a body
> search done for cases where people are not downloading message bodies locally.

This sounds reasonable to me, but I haven't tried this out yet though. I'll try to look at it this week.

Comment 11

[David :Bienvenu]

what does this patch do for quick search of an imap folder using locally available terms? Quick search should bias towards offline search whenever possible

Comment 12

[Kent James (:rkent)]

(In reply to comment #11)
> what does this patch do for quick search of an imap folder using locally
> available terms? Quick search should bias towards offline search whenever
> possible

The patch will do its darndest to use whatever search type is specified, and if you don't specify, or if it thinks everything will fail, then it defaults to offline. So it should try its darndest to search offline with quick search of a normal folder since that never specifies an online search mode. That is no different from the previous behavior. The only thing that is different is that the online/offline decision uses the validity tables, which are more accurate (and more error prone) than the previous approach.

Things get more interesting when you combine quick search with a virtual folder. In this patch, we try to continue to honor the offline/online search choice from the virtual folder. So quick search of a virtual folder that is specified as online would be done online if possible. That is a change from the previous behavior, and I suppose is open for discussion. I don't have strong feelings here. I would imagine that most ordinary users would only switch the virtual folder to online use if they can't use the default, which probably means they are doing an imap body search without offline folders. In that case, the quick search would also be forced to search online, and the discussion is moot. But for those who unwisely set online with no good reason, this patch decides to honor their request even in quick search, while the previous behavior would switch that to offline search if possible.

Comment 13

[Kent James (:rkent)]

Attachment: Fixed missing interface in fixIterator

Testing a little more the case where online virtual searches are combined with custom views, I discovered an issue (small but serious). Fix attached.

I still think that this bug is important for b4, since without it the contentional search dialogs show a much reduced search capability than actually exists. It would be good to get it in before the last minute.

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 397951 [details] [diff] [review]
Fixed missing interface in fixIterator

>+  let checkbox = document.getElementById('searchOnline');
>+  if (typeof arguments.searchOnline != "undefined")
>+    document.getElementById('searchOnline').checked = arguments.searchOnline;

You don't actually use checkbox here, so just loose it.

>+  const nsMsgSearchScopeValue localNews = 4; // offline news, base table, no body or junk
...
>+  const nsMsgSearchScopeValue localNewsJunk = 14; // local news + junk
>+  const nsMsgSearchScopeValue localNewsBody = 15; // local news + body
>+  const nsMsgSearchScopeValue localNewsJunkBody = 16; // local news + junk + body

nit: prefer comments on the line previous, using the /// notation so that doxygen can process them.

>+  NS_ASSERTION (nsnull == m_localNewsTable, "already have local news validity table");
>+  nsresult rv = NewTable(getter_AddRefs(m_localNewsTable));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  rv = SetLocalNews(m_localNewsTable);
>+  return rv;

nit: just return the function result direct and save the intermediate assignment to rv (several places in this file).

r=Standard8 with those fixed.

Comment 15

[David :Bienvenu]

Isn't this:

+      // Anything greater than 0 is an online server like IMAP or news.
+      let enableCheckbox = gCurrentFolder.server.offlineSupportLevel;
+
+      if (enableCheckbox)
+        // check if we are offline
+        enableCheckbox = !Components.classes["@mozilla.org/network/io-service;1"]
+                                    .getService(Components.interfaces.nsIIOService)
+                                    .offline;
+      if (enableCheckbox)
+        searchOnline.disabled = false;

just

  if (gCurrentFolder.server.offlineSupportLevel &&
      !Components.classes["@mozilla.org/network/io-service;1"]
    searchOnline.disabled = false;
  else...

?

Other than that, and Standard8's comments, this looks OK - I'll go mark sr.

Comment 16

[David :Bienvenu]

Comment on attachment 397951 [details] [diff] [review]
Fixed missing interface in fixIterator

sr=me, with comments addressed, thx for fixing this!

Comment 17

[Kent James (:rkent)]

Attachment: Fixed nits [Checkin: Comment 23]

This fixes the nits from the r & sr. Would be good to get the UI review soon as requested by Standard8.

Comment 18

[David :Bienvenu]

marking as blocking tb 3 since it's a regression that has a fix...

Comment 19

[Kent James (:rkent)]

Attachment: Screen shot

David A suggested that I add a screen shot for a UI review. This is it, but it is not really all that revealing.

Most of this bug deals with what is really an error, that the existing search dialog does not expose a correct set of capabilities of the search. That should now be fixed. It also did not enable searching online, even though that capability exists, and is exposed in the virtual folder dialog. Normally we would not encourage online searches, but at least now they can if they want.

There are interesting issues of the interaction between the online search choice, and the views and quick search features that a user selects. I chose to honor the user's choice of online/offline whenever possible, even though it may result in a slower search, but to silently override it if a search can be done locally but not online. I don't think this is a core issue though, because online search is generally not encouraged, so this is really a fringe issue.

The other interesting issue is the complete set of validity tables for news, including properly enabling body and junk terms if the news folder supports it. That puts junk now way ahead of rss in correctness (rss just allows anything that mail can do, whether it makes sense or not).

So while there are important user experience issues in this bug, they are not really easily seen in the graphics, and tend to affect more advanced users.

Comment 20

[Andrew Sutherland [:asuth] (he/him)]

After a super brief discussion with davida, we have the modest proposal of changing "search online" to "Search on server" or something that better conveys what online search actually means.

Until I actually worked with the code in the search engine, I had no idea what "online" actually meant in this case.  It could have meant "on server" or it could have meant "realtime" (versus "cached").  Additionally, in the internet age "online" sounds good, even though offline search is much faster.

Given that online generally equals slow, it might be good to even put that in the label: "Search on server (slow)".  Of course, that raises the question of why have the option at all, so maybe: "Search on server (slow, only for experts)".

Comment 21

[Kent James (:rkent)]

Your points are fine, and I'm not really trying to push online search here. I think that one use though is for body searches for people who do not keep their messages offline. And talk about terrible words, we use "Offline" to mean both "you have everything stored locally now" and "you've lost your internet connection".

The virtual folder has "Search Online (Gives up-to-date results for IMAP and News folders but increases time to open the folder)" which I think is way too long.

I see your point about "online" as a word. I would suggest "Search on server (may be slower)" but I will happily defer to all of those UI experts in the audience.

Comment 22

[Kent James (:rkent)]

The patch as-is could be checked in, but there still seems to be discussion about the wording. Standard8, can you make the call on checkin or not, and do the checkin if appropriate? As I said in IRC, I an concerned about some of the risks of the code for this patch, so the extra testing time would be good, and the wording can be changed easily later.

Comment 23

[Andrew Sutherland [:asuth] (he/him)]

pushed:
http://hg.mozilla.org/comm-central/rev/f83b9d5dddd1

Yes, 'twas not my intent to suggest the patch block on wording changes.  Just fodder for Bryan.

Comment 24

[Andrew Sutherland [:asuth] (he/him)]

(commit authority from IRC: <Standard8> rkent: release driver says land it as is and let clarkbw comment on it when he gets back if he wants to)

Comment 25

[David :Bienvenu]

I suspect this may have broken smart folders - someone is calling getAvailable with an attrib of -1 when I click on a smart folder, so we crash.  This seems like a likely culprit...

Comment 26

[David :Bienvenu]

Attachment: fix crash w/ smart folders - checked in.

match all msg searches don't have an attrib to search over...

Comment 27

[Kent James (:rkent)]

Comment on attachment 398940 [details] [diff] [review]
fix crash w/ smart folders - checked in.

This is fine, but we should really fix getAvailable so that it simply returns false with an input of -1.

Comment 28

[David :Bienvenu]

yeah, that would be good as well - as long as that would make matchAll terms be run locally...

Comment 29

[Kent James (:rkent)]

It's important that we do the UI review on this prior to the string freeze for beta 4. I'm sending this message to ping clarkbw on this issue.

Also, the whole concept of search online seems to be in question, since there are very few use cases where it is the best solution, and people will tend to try it anyway. So let be raise the proposal that we consider hiding the UI by default, and leaving it to an extension or hidden preference to enable it for people who know what they are doing, and have a real need for this. If we go this path, we should do that on both search as well as virtual folders UI.

Comment 30

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #29)
> Also, the whole concept of search online seems to be in question, since there
> are very few use cases where it is the best solution, and people will tend to
> try it anyway. So let be raise the proposal that we consider hiding the UI by
> default, and leaving it to an extension or hidden preference to enable it for
> people who know what they are doing, and have a real need for this. If we go
> this path, we should do that on both search as well as virtual folders UI.

That seems like a good balance to strike.  I've been trying text the looks like this, any comments on it?  "Run search on server"

Comment 31

[Magnus Melin [:mkmelin]]

If you're hiding it, can you make that a pref?

No necessarily this bug; but how will searches work for folders where part of the messages are offline, but most only online (like "just keep last 7 days for offline use")?

Comment 32

[Kent James (:rkent)]

How does one configure "just keep last 7 days for offline use"? The account setup screen that I see will delete messages older than a certain date, but then claims that it does that on both the local and server copy. There is an option however to not download messages over a certain size.

There is no attempt to figure out where messages are. You either search offline or online, never both.

Comment 33

[Magnus Melin [:mkmelin]]

Bug 510707 is adding the UI, the backend already exists.

Comment 34

[Kent James (:rkent)]

clarkbw: I like "Run Search on Server" just fine. 

Magnus: Obviously I haven't been following bug 510707. But it seems like that bug is encouraging people to keep only a fraction of their folder offline. If they actually do that, and remember that they are doing that, then it increases the value of allowing the "Run Search on Server" option, as that is the only way to really do a body search of mail that you don't have locally. I wish there was some short way that could be communicated.

I'm leaning against hiding this now.

Comment 35

[Kent James (:rkent)]

Attachment: use "Run search on server"

Let's focus the UI review on a particular patch. I'm happy with "Run search on server".

In doing this patch, I noticed that the button "Open Message Folder" no longer works in the search dialog, which I assume happened earlier and is not related to this bug. I'll open a new bug for that.

Comment 36

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 399249 [details] [diff] [review]
use "Run search on server"

thanks!

Comment 37

[Bryan Clark (DevTools PM) [:clarkbw]]

I do think the "Run search on server" option is a bit confusing for most people but I'd be ok with showing it if we felt the search experience was going to get worse with not having messages locally.

Really I'd like some kind of detection of that case so we can offer a "Continue this search online" option for searches we know ran short and then offer to "[x] Always run this search online"; or something to that effect.

Comment 38

[Magnus Melin [:mkmelin]]

Comment on attachment 399249 [details] [diff] [review]
use "Run search on server"

Unfortunately you need to bump the localization keys too so localizers can keep up.

Comment 39

[Kent James (:rkent)]

"for searches that ran short" does not really make any sense, because in its most important use, for imap body search when messages are not stored locally, we don't show the search attribute "body" at all for folders that are not available locally, so the user cannot select this choice to run short with. So we strive for correctness, and don't show the user what we cannot do correctly, rather than strive to give him partial results.

But I will certainly grant you that the whole array of choices in the search term dialogs, in both search and filters, is bound to be bewildering to the average user.

Comment 40

[Kent James (:rkent)]

Attachment: [checked in] key is onServer, don't change access key for open folder

Comment 41

[Magnus Melin [:mkmelin]]

Comment on attachment 399270 [details] [diff] [review]
[checked in] key is onServer, don't change access key for open folder

Looks good, r=mkmelin

Comment 42

[Mark Banner (:standard8)]

Comment on attachment 399270 [details] [diff] [review]
[checked in] key is onServer, don't change access key for open folder

Thanks for the update Kent.

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

Comment 43

[Mark Banner (:standard8)]

Comment on attachment 398775 [details] [diff] [review]
Fixed nits [Checkin: Comment 23]

I guess Bryan is happy with this now so no need for this request.