Search window does not allow to open second message from result list

RESOLVED FIXED in seamonkey2.5

Status

SeaMonkey
MailNews: Message Display
--
major
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: InvisibleSmiley, Assigned: Karsten Düsterloh)

Tracking

({regression})

Trunk
seamonkey2.5
regression
Bug Flags:
in-testsuite -

SeaMonkey Tracking Flags

(seamonkey2.3+ fixed, seamonkey2.4 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

6 years ago
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. ;-)
Attachment #545952 - Flags: superreview?(neil)
Attachment #545952 - Flags: review?(mnyromyr)

Comment 1

6 years ago
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;
(Reporter)

Comment 2

6 years ago
Created attachment 546415 [details] [diff] [review]
patch v1a

Sure, why not. The extended comment still explains the logic.
Attachment #545952 - Attachment is obsolete: true
Attachment #546415 - Flags: superreview?(neil)
Attachment #546415 - Flags: review?(mnyromyr)
Attachment #545952 - Flags: superreview?(neil)
Attachment #545952 - Flags: review?(mnyromyr)

Updated

6 years ago
Attachment #546415 - Flags: superreview?(neil) → superreview+
(Assignee)

Comment 3

6 years ago
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.
Attachment #546415 - Flags: review?(mnyromyr) → review-
(Reporter)

Comment 4

6 years ago
(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).
(Reporter)

Comment 5

6 years ago
(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

6 years ago
(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
(Assignee)

Comment 7

6 years ago
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 …
Attachment #547971 - Flags: review?(neil)

Updated

6 years ago
Attachment #547971 - Flags: review?(neil) → review+
(Reporter)

Comment 8

6 years ago
(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).
Assignee: jh → mnyromyr
(Assignee)

Comment 9

6 years ago
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.
Attachment #546415 - Attachment is obsolete: true
Attachment #547971 - Attachment is obsolete: true
Attachment #548003 - Flags: review+
(Assignee)

Comment 10

6 years ago
Pushed to trunk as <http://hg.mozilla.org/comm-central/rev/ea935b82337f>.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [relnote note: actual problem was broken since at least SM 1.0]
(Assignee)

Updated

6 years ago
Whiteboard: [relnote note: actual problem was broken since at least SM 1.0] → [relnote note: actual problem existed since at least SM 1.0]

Comment 11

6 years ago
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
Attachment #548003 - Flags: approval-comm-beta+
Attachment #548003 - Flags: approval-comm-aurora+
(Assignee)

Comment 12

6 years ago
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>.

Updated

6 years ago
Flags: in-testsuite-
Target Milestone: --- → seamonkey2.3

Updated

6 years ago
tracking-seamonkey2.3: ? → +
status-seamonkey2.3: --- → fixed
status-seamonkey2.4: --- → fixed
Target Milestone: seamonkey2.3 → seamonkey2.5

Comment 13

6 years ago
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.
(Reporter)

Comment 14

6 years ago
(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

6 years ago
 ... but not the best decision 

maybe this following Screenshot helps to understand

Comment 16

6 years ago
Created attachment 554344 [details]
AdvancedSearch

Frame for message preview

Comment 17

6 years ago
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.
(Reporter)

Updated

6 years ago
Keywords: relnote
Whiteboard: [relnote note: actual problem existed since at least SM 1.0]
You need to log in before you can comment on or make changes to this bug.