154658 - Web search for selected text in mail window

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, enhancement)

Description

[Lasse Marøen]

Since bug 15176 was fixed we have web search for selected text in the browser.
It would be nice to have it in MailNews too.

To reproduce:

1. Read mail
2. Find interesting word in message
3. Select and right click (or whatever for mac)

Expected:

Context menu includes item:
Web search for "interesting word"

Comment 1

[Jon Granrose]

I'd like to see this also.  I finally missed this feature enough times that I
took the time to find the bug...

Comment 2

[Michael kmd]

I use the 'search web' feature in the browser quite often. I would like to see
it in the mail app too. tnx

Comment 3

[Bogdan Stroe]

*** Bug 244194 has been marked as a duplicate of this bug. ***

Comment 4

[Krang]

I have a patch for this bug, but it will need to wait for bug 469498 since it modifies the mailnews context menu.

Comment 5

[Krang]

Attachment: Add context search to mailnews

Here is the patch that enables context menu searching in mailnews.  This is accomplished by accessing the browser window object and executing its search function to avoid having to move a ton of browser-specific search code to a shared location.

I couldn't get this working when there is no browser window open, since attempting to open a browser window and call the OpenSearch() function on it caused an error (no such function).  I assume it is trying to call the function before the window is fully loaded.  To get around this, I simply hid the option when there is no browser window, although another option would be to have the option still there, but disabled.

Comment 6

[Karsten Düsterloh]

You could mimic the way toOpenWindowByType works and do the search instead of the focussing. You may find that navigator.js::Startup isn't called yet, but you could try waiting for it in a loop:
...
if (!window[uri].gNavigatorRegionBundle)
  setTimeout(newWindowLoaded, 0, event);
...

Comment 7

[Ian Neal]

I'm not sure search belongs with the reply/forward context menu items either.

Comment 8

[Philip Chee]

+        var showSearch = Components.classes["@mozilla.org/appshell/window-mediator;1"]
+                                   .getService(Components.interfaces.nsIWindowMediator)
+                                   .getMostRecentWindow("navigator:browser");

Isn't this just reinventing getTopWin()? Why not just do:
  var showSearch = getTopWin();
directly and then in the menu item do:
  oncommand="showSearch.OpenSearch(...);"

Comment 9

[Karsten Düsterloh]

Comment on attachment 362438 [details] [diff] [review]
Add context search to mailnews

> Isn't this just reinventing getTopWin()?

True. But I don't see "you can only search if there's already a browser window open" as a useful precondition anyway.

> I'm not sure search belongs with the reply/forward context menu items either.

Probably better to put it  above mailContext-openNewWindow, where "all" the other selection items are? IanN?

Comment 10

[Ian Neal]

(In reply to comment #9)
> (From update of attachment 362438 [details] [diff] [review])
> > I'm not sure search belongs with the reply/forward context menu items either.
> 
> Probably better to put it  above mailContext-openNewWindow, where "all" the
> other selection items are? IanN?
Yes, that is one possible option.

Comment 11

[Krang]

Attachment: Add context search to mailnews v2

Here is a new patch that implements the suggestions/fixes in the comments above.  The search now fully works even if there are no open browser windows.

Comment 12

[Karsten Düsterloh]

Comment on attachment 365378 [details] [diff] [review]
Add context search to mailnews v2

Nice, almost there...
Some of the following comments may seem odd at first (and were violated in certain parts of our code in the past), but we strive towards a common, highly readable coding style.

>diff --git a/mailnews/base/resources/content/mailWindowOverlay.js 
>+function MsgOpenSearch(searchStr, reverseBackgroundPref)

Function parameters should have the 'a'-prefix, i.e. aSearchStr, aReverseBackgroundPref.

>+{
>+    var uri = getBrowserURL();
>+
>+    // don't do several loads in parallel
>+    if (uri in window)
>+        return;

Move the var line between comment and if.
And please use a two space indent throughout your whole new code.

>+    var topWindow = getTopWin();
>+

Drop the empty line.

>+    if (topWindow)
>+        topWindow.OpenSearch('internet', searchStr, true, reverseBackgroundPref);
>+    else
>+    {

If one 'if'-branch has braces, the other one must have them, too.

>+        // open the requested window, but block it until it's fully loaded
>+        function newWindowLoaded(event)

Although the actual function name doesn't matter in this case, please:
- make it start with an uppercase letter
- use a different name than in toOpenWindowByType, e.g. NewSearchWindowLoaded
- use 'a'-prefix for the parameter

>+        {
>+            if (!window[uri].gNavigatorRegionBundle) {

Please use 'brace on its own line' style.

>+                setTimeout(newWindowLoaded, 0, event);
>+                return;
>+            }
>+
>+            window[uri].OpenSearch('internet', searchStr, false, reverseBackgroundPref);

Opening a new search when the window is about to pass away doesn't make much sense. 
(You'll get here from the unload handler as well!)

Comment 13

[Krang]

Attachment: Add context search to mailnews v2.1

Here is an updated patch that addresses all review comments above.

Comment 14

[Karsten Düsterloh]

Comment on attachment 366148 [details] [diff] [review]
Add context search to mailnews v2.1

>diff --git a/mailnews/base/resources/content/mailWindowOverlay.js 
>+
>+        window[uri].OpenSearch("internet", aSearchStr, false, aReverseBackgroundPref);

r=me with that empty line dropped on check-in.

Comment 15

[Krang]

Comment on attachment 366148 [details] [diff] [review]
Add context search to mailnews v2.1

Thanks for the review.  Re-requesting sr due to the changes made since the first patch.

Comment 16

[[e-mail]]

Comment on attachment 366148 [details] [diff] [review]
Add context search to mailnews v2.1

>+  // don't do several loads in parallel
This is unlikely to be a problem; the original code was guarding against pressing window-opening keys (e.g. Ctrl+H) multiple times.

>+        if (!window[uri].gNavigatorRegionBundle)
>+        {
>+          setTimeout(NewSearchWindowLoaded, 0, aEvent);
>+          return;
>+        }
>+
>+        window[uri].OpenSearch("internet", aSearchStr, false, aReverseBackgroundPref);
So, your load handler is attached first, and therefore runs before Startup, but Startup is guaranteed to run next, so you should need exactly one timeout. In other words, you could call w.setTimeout(w.OpenSearch, 0, "internet", ...);

>+    // or until the current window passes away
What do you want to happen when the current window "passes away"? Currently the new browser window still opens, but with no content. But then, given the first comment, perhaps you don't need to bother with an unload handler.

>+    window[uri] = window.openDialog(uri, "_blank", "chrome,all,dialog=no");
[I had to read the code to remind myself that this is the magic for "open a window that I can then load something in", as distinct for "null" which is the magic for "open a window using the new window preference" and "about:blank" which is the magic for "open a blank window ready for URL entry".]

Comment 17

[Krang]

Attachment: Add context search to mailnews v2.2

Here is an updated patch.  Carrying over Mnyromyr's r+ from v2.1.

(In reply to comment #16)
> This is unlikely to be a problem; the original code was guarding against
> pressing window-opening keys (e.g. Ctrl+H) multiple times.

Fixed by removing that part.

> So, your load handler is attached first, and therefore runs before Startup, but
> Startup is guaranteed to run next, so you should need exactly one timeout. In
> other words, you could call w.setTimeout(w.OpenSearch, 0, "internet", ...);

Good idea, fixed.

> What do you want to happen when the current window "passes away"? Currently the
> new browser window still opens, but with no content. But then, given the first
> comment, perhaps you don't need to bother with an unload handler.

Fixed by removing the unload handler.

> [I had to read the code to remind myself that this is the magic for "open a
> window that I can then load something in", as distinct for "null" which is the
> magic for "open a window using the new window preference" and "about:blank"
> which is the magic for "open a blank window ready for URL entry".]

I adjusted the comment to clarify.

Comment 18

[[e-mail]]

Comment on attachment 366967 [details] [diff] [review]
Add context search to mailnews v2.2

>+      delete window[uri];
Nit: if you use topWindow (or a new variable) you don't need to delete it.
sr=me with that fixed.

Comment 19

[Krang]

Attachment: Add context search to mailnews v2.3 [Checkin: Comment 20]

(In reply to comment #18)
> Nit: if you use topWindow (or a new variable) you don't need to delete it.
> sr=me with that fixed.

Thanks, here is the patch for checkin.  I reused topWindow for the new search window.

Comment 20

[Serge Gautherie (:sgautherie)]

Comment on attachment 367160 [details] [diff] [review]
Add context search to mailnews v2.3
[Checkin: Comment 20]


http://hg.mozilla.org/comm-central/rev/4964819a0618