Last Comment Bug 671605 - Search window does not allow to open second message from result list
: Search window does not allow to open second message from result list
Status: RESOLVED FIXED
: regression
Product: SeaMonkey
Classification: Client Software
Component: MailNews: Message Display (show other bugs)
: Trunk
: All All
: -- major (vote)
: seamonkey2.5
Assigned To: Karsten Düsterloh
:
Mentors:
Depends on: 660882
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-14 10:58 PDT by Jens Hatlak (:InvisibleSmiley)
Modified: 2011-09-25 15:44 PDT (History)
13 users (show)
iann_bugzilla: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
+
fixed
fixed


Attachments
patch (2.77 KB, patch)
2011-07-14 10:58 PDT, Jens Hatlak (:InvisibleSmiley)
no flags Details | Diff | Splinter Review
patch v1a (2.73 KB, patch)
2011-07-17 06:11 PDT, Jens Hatlak (:InvisibleSmiley)
mnyromyr: review-
neil: superreview+
Details | Diff | Splinter Review
fix reusing message window from search (878 bytes, patch)
2011-07-23 16:40 PDT, Karsten Düsterloh
neil: review+
Details | Diff | Splinter Review
fix reusing message window from search, remove gWindowReuse (3.10 KB, patch)
2011-07-24 06:41 PDT, Karsten Düsterloh
mnyromyr: review+
iann_bugzilla: approval‑comm‑aurora+
iann_bugzilla: approval‑comm‑beta+
Details | Diff | Splinter Review
AdvancedSearch (177.73 KB, image/jpeg)
2011-08-19 03:07 PDT, Glutton.Vidal
no flags Details

Description Jens Hatlak (:InvisibleSmiley) 2011-07-14 10:58:28 PDT
Created attachment 545952 [details] [diff] [review]
patch

+++ This bug was initially created as a clone of Bug #660882 +++

When performing a search via the search window (Tools -> Search Messages) it's impossible to open a second message by double-clicking or selecting it and hitting the open button, unless mailnews.reuse_message_window is set to false. This is a regression from SeaMonkey 2.0.

This should make SM 2.3 (currently in Beta) if possible.

Here's what happens:
1. MsgOpenSelectedMessages (mailWindowOverlay.js) is called
2. MsgOpenSelectedMessageInExistingWindow is called from there

The difference between 2.0 and later is that with 2.0, gDBView.URIForFirstSelectedMessage in MsgOpenSelectedMessageInExistingWindow was not defined so the catch block was called and the function returned false, which made MsgOpenSelectedMessages not return early.

With 2.1 and 2.2, gDBView.URIForFirstSelectedMessage is now defined so the try block is executed until the end. Further down it calls windowID.LoadMessageByMsgKey which seems to have a different gDBView; in any case it doesn't do the right thing.

Instead of trying to investigate what LoadMessageByMsgKey is supposed to do I took the short track: Don't go there in the first place! We really want to open new windows for each message opened from Advanced Search, no matter what the pref says.

While I was at it I took the liberty to kill the useless gWindowReuse variable. Contrary to its name, it's a local variable that is not used anywhere else.

I hope the checking for the dbView search type is correct. ;-)
Comment 1 neil@parkwaycc.co.uk 2011-07-16 16:46:06 PDT
Comment on attachment 545952 [details] [diff] [review]
patch

>+  var reuse_win = gPrefBranch.getBoolPref("mailnews.reuse_message_window");
>+  if (gDBView.viewType == nsMsgViewType.eShowSearch)
>+    reuse_win = false;
>+
>+  if (reuse_win && numMessages == 1 && MsgOpenSelectedMessageInExistingWindow())
>     return;
I would be tempted to write this in one statement i.e.
if (numMessages == 1 &&
    gDBView.viewType != nsMsgViewType.eShowSearch &&
    Services.prefs.getBoolPref("mailnews.reuse_message_window") &&
    MsgOpenSelectedMessageInExistingWindow())
  return;
Comment 2 Jens Hatlak (:InvisibleSmiley) 2011-07-17 06:11:56 PDT
Created attachment 546415 [details] [diff] [review]
patch v1a

Sure, why not. The extended comment still explains the logic.
Comment 3 Karsten Düsterloh 2011-07-19 13:51:07 PDT
Comment on attachment 546415 [details] [diff] [review]
patch v1a

(In reply to comment #0)
> We really want to open new windows for each message opened from Advanced
> Search, no matter what the pref says.

Actually, no — why would we want to do that?!
That's exactly what the pref is for.
Comment 4 Jens Hatlak (:InvisibleSmiley) 2011-07-19 14:22:31 PDT
(In reply to comment #3)
> Comment on attachment 546415 [details] [diff] [review] [review]
> patch v1a
> 
> (In reply to comment #0)
> > We really want to open new windows for each message opened from Advanced
> > Search, no matter what the pref says.
> 
> Actually, no — why would we want to do that?!

Please go back to a previous version and check. That's what we have (always?) been doing before it regressed.

> That's exactly what the pref is for.

Debatable. I'd say opening a message from the main window or from search results is not the same.

Anyway, if you're serious about respecting the pref in any case (which would actually be a change rather than a fix IMO) then I'd appreciate some help on how to get this right. As I explained in previous comments, the code in question wasn't reached in former times, and additionally we're dealing with different gDBViews now (maybe we'll have to change some functions to accept a dbView parameter; what a mess).
Comment 5 Jens Hatlak (:InvisibleSmiley) 2011-07-22 06:30:26 PDT
(In reply to comment #3)
> > We really want to open new windows for each message opened from Advanced
> > Search, no matter what the pref says.
> 
> Actually, no — why would we want to do that?!

If I open search results, I might want to compare different results side by side or leave some open longer than others. For that I need multiple standalone windows. This is in contrast to the normal usage of the standalone window (opened from the main MailNews window) which you open just once and then use navigation keys like N or Space to proceed (by default; IMO the point of having the pref is to allow changing this behavior of a message flow reading window).
Comment 6 Glutton.Vidal 2011-07-22 12:58:28 PDT
(In reply to comment #5)
> (In reply to comment #3)
> > > We really want to open new windows for each message opened from Advanced
> > > Search, no matter what the pref says.
> > 
I want to open several search result for different reason, as used to do it in former SM versions
Comment 7 Karsten Düsterloh 2011-07-23 16:40:50 PDT
Created attachment 547971 [details] [diff] [review]
fix reusing message window from search

(In reply to comment #4)
> > > We really want to open new windows for each message opened from Advanced
> > > Search, no matter what the pref says.
> > 
> > Actually, no — why would we want to do that?!
> 
> Please go back to a previous version and check. That's what we have
> (always?) been doing before it regressed.

It may have been broken for quite some time now, but that was surely not intended, see <http://mxr.mozilla.org/comm-central/source/suite/mailnews/mailWindowOverlay.js?mark=1678-1679#1675>.

> As I explained in previous comments, the code in
> question wasn't reached in former times, and additionally we're dealing with
> different gDBViews now (maybe we'll have to change some functions to accept
> a dbView parameter; what a mess).

Actually, the problem is a bit less odd:
- we clone the gDBView from the window which requests the opening
- nsMsgDBView::LoadMessageByViewIndex checks mSuppressMsgDisplay, which is true for the search window gDBView (we never have a message pane there) and false for e.g. the main message window

We just need to make sure that <search>.gDBView.suppressMsgDisplay is overridden before requesting a message load.

This patch is doing that.

(In reply to comment #5)
> If I open search results, I might want to compare different results side by
> side or leave some open longer than others. For that I need multiple
> standalone windows. This is in contrast to the normal usage of the
> standalone window

Well, that may be an interesting enhancement, but it's not the actual bug here.
The standard thread pane context menu (which includes eg. "Open Message in New Window") is missing in the search window anyway …
Comment 8 Jens Hatlak (:InvisibleSmiley) 2011-07-24 02:53:44 PDT
(In reply to comment #7)
> We just need to make sure that <search>.gDBView.suppressMsgDisplay is
> overridden before requesting a message load.
> 
> This patch is doing that.

Hmm, I'd still like to get rid of gWindowReuse... Same file.

> (In reply to comment #5)
> > If I open search results, I might want to compare different results side by
> > side or leave some open longer than others. For that I need multiple
> > standalone windows. This is in contrast to the normal usage of the
> > standalone window
> 
> Well, that may be an interesting enhancement, but it's not the actual bug
> here.

I think I'm at a point where I'll just leave that discussion and set the pref to false for me. I don't use the standalone message window for browsing anyway, and this whole discussion seems to be more on personal preferences (and maybe consistency with internal prefs).
Comment 9 Karsten Düsterloh 2011-07-24 06:41:39 PDT
Created attachment 548003 [details] [diff] [review]
fix reusing message window from search, remove gWindowReuse

(In reply to comment #8)
> Hmm, I'd still like to get rid of gWindowReuse... Same file.

Done; carrying over r+.

> I think I'm at a point where I'll just leave that discussion and set the
> pref to false for me. I don't use the standalone message window for browsing
> anyway, and this whole discussion seems to be more on personal preferences
> (and maybe consistency with internal prefs).

JFTR: If you select multiple search results and hit the Open button, multiple message windows will get spawned. The pref only handles the "open one (additional) message" case, and behaviour of that should be consistent.
Comment 10 Karsten Düsterloh 2011-07-24 08:12:48 PDT
Pushed to trunk as <http://hg.mozilla.org/comm-central/rev/ea935b82337f>.
Comment 11 Ian Neal 2011-07-24 08:23:51 PDT
Comment on attachment 548003 [details] [diff] [review]
fix reusing message window from search, remove gWindowReuse

No L10n changes so should be okay to go into both comm-beta and comm-aurora
Comment 12 Karsten Düsterloh 2011-07-24 15:21:48 PDT
Pushed to aurora as <http://hg.mozilla.org/releases/comm-aurora/rev/23c92504abe8>.
Pushed to beta as <http://hg.mozilla.org/releases/comm-beta/rev/45a0b6334810>.
Comment 13 Glutton.Vidal 2011-08-16 22:08:05 PDT
Thanks for the trial to change it ... but to open several messages at once is not that, what we had in former times. I think, that all who have notified for this bug want to open each message after the other till the "one" will be found.
Another great (additional) solution would be, to add a message (preview) area in the Advanced Search Window likewise it exists already for the Inbox/Sent/Trash folder.
Comment 14 Jens Hatlak (:InvisibleSmiley) 2011-08-18 11:01:44 PDT
(In reply to Glutton.Vidal from comment #13)
> but to open several messages at once
> is not that, what we had in former times. I think, that all who have
> notified for this bug want to open each message after the other till the
> "one" will be found.

Well, the module owner made a decision so there is little point to continue this road. However, you can can set mailnews.reuse_message_window to false in about:config and get the old behavior back (though at the cost of also changing the behavior of opening individual messages from the thread pane).

> Another great (additional) solution would be, to add a message (preview)
> area in the Advanced Search Window likewise it exists already for the
> Inbox/Sent/Trash folder.

Huh? I understand the first part but not the second one. :-/
Comment 15 Glutton.Vidal 2011-08-19 03:04:39 PDT
 ... but not the best decision 

maybe this following Screenshot helps to understand
Comment 16 Glutton.Vidal 2011-08-19 03:07:39 PDT
Created attachment 554344 [details]
AdvancedSearch

Frame for message preview
Comment 17 Glutton.Vidal 2011-08-25 09:16:35 PDT
It would be even a compromise to open several messages from Advanced Search Window with right mouse click likewise it exists already for the Inbox/Sent/Trash folder. But so far it isn't possible.

Note You need to log in before you can comment on or make changes to this bug.