66466 - No cross-folder navigation using Next Unread Thread

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, defect, P3)

Description

[laurel]

Using jan24 commercial trunk build

Using Next Unread Thread will currently not engage navigation across folders/groups.

1.  Within one mail account, go to a populated user created folder. Put into
threaded mode.  Mark several of the messages unread.
2.  Go to INBOX for that same mail account. There are no unread messages in the
inbox. 
3.  Put the inbox in threaded mode. Select a message in the inbox, then
Go|Next|Unread Thread.

Actual result:  nothing happens

Expected:  Should display a dialog "Advance to next unread message in <folder
name with unread>?"

Note:  gave threaded mode example, should also work with flat sort.

Comment 1

[laurel]

nominating for next release

Comment 2

[scottputterman]

marking nsbeta1+

Comment 3

[scottputterman]

marking nsbeta1- and moving to future milestone.

Comment 4

[Mack Earnhardt]

When in threaded mode, do you have non-zero unread counts for any threads?  If
so, this is one of the effects of bug 59681.

Comment 5

[Jeroen Janssen]

With ID 2001072403

I have the following mail layout:

* mail account ([e-mail])
* mail account (...)
* Local Folders
* news account
* news account

I have a message filter  that moves mail from the [e-mail] mailaccount to a
subdirectory of the "Local Folders".I have unread mails in Local Folders and in
several newsgroups.

If I stand in the Inbox folder of my first email account and press 'n' (Next
Unread Mail), I get a dialog box for unread mail in my *LAST* news account. This
is a bug, since it should go to the unread mails in my Local Folder first.

Comment 6

[Håkan Waara]

Laurel, can you please check if this is still reproducible (despite Seth's 
message navigation fixes recently) ?

Comment 7

[laurel]

Using jan10 commercial build, win98: This still exists exactly as written.  Just
to keep this clear, this is using next unread THREAD, not next unread message.

To Jeroen in comment #5: your comment is addressing a different issue; not for
this bug report.

Comment 8

[Travis Crump]

Attachment: proposed fix

This patch fixes both this bug and bug 150970.	I rearranged the case
nsMsgNavigationType::nextUnreadThread  so that it always falls through
nsMsgNavigationType::nextUnreadMessage since it didn't make sense to me why we
weren't going to the next unread message if a message wasn't currently
selected.  It also didn't make sense to me why we were bailing if
MarkThreadOfMsgRead failed so I removed that error checking.  If the thread
isn't successfully marked as read than the user will probably be taken to
another message in the same thread and will eventually get to a new thread by
repeating using next unread thread whereas if we bail than the user will stay
at the same place and since the error is likely to repeat will never get
anywhere.  Regardless, both behaviors are equally broken so it makes sense to
go with the simpler code.  It may be possible(and better) to intelligently deal
with an error, but the only thing I could think of was to rerun
MarkThreadOfMsgRead, but if it had an error once it is likely to just fail
again.	Since with this change a message will be selected when we do cross
folder navigation I added nextUnreadThread to the types of motion for which we
do cross folder navigation thereby fixing this bug.  

I also discovered that we are marking the thread as read twice which is
redundant so I removed the marking thread as read call from
mail3PaneWindowCommands.js which should result in a negligible performance gain
which is always good.

Comment 9

[Jo Rhett]

Attachment: Here's that IMAP debug

This is the most current IMAP log.  Hope you find it helpful.

Comment 10

[Karsten Düsterloh]

Attachment: updated patch to trunk

David, if TB is interested in this fix, I can do that, too...

Comment 11

[[e-mail]]

Comment on attachment 245914 [details] [diff] [review]
updated patch to trunk

>+  if (type != nsMsgNavigationType.nextUnreadMessage &&
>+      type != nsMsgNavigationType.nextUnreadThread &&
>+      type != nsMsgNavigationType.forward &&
>+      type != nsMsgNavigationType.back)    // currently, only do cross folder navigation for "next unread message"
Comment is out of date ;-)

>             {
>-                nsMsgKeyArray idsMarkedRead;
The { only exists here to scope the variable, so you could outdent the block.

>+                     nsMsgKeyArray idsMarkedRead;
>+                     rv = MarkThreadOfMsgRead(m_keys.GetAt(startIndex), startIndex, idsMarkedRead, PR_TRUE);
>                 }
Worth testing rv?

>+                return NavigateFromPos(nsMsgNavigationType::nextUnreadMessage, startIndex, pResultKey, pResultIndex, pThreadIndex, PR_TRUE);
>                 break;
Useless break after return.

Comment 12

[[e-mail]]

Comment on attachment 245914 [details] [diff] [review]
updated patch to trunk

>+	//First mark the current thread as read already handled.
This comment makes no sense.

r=me with this and my previous comments addressed.

Comment 13

[David :Bienvenu]

Comment on attachment 245914 [details] [diff] [review]
updated patch to trunk

yes, I think this would be nice for TB as well, thx.

Comment 14

[Karsten Düsterloh]

Attachment: SM + TB patch

Updated patch to Neil's comments; see comment #8 why the rv value is ignored.
Added TB changes, but in fact it's just one additional file, since msgViewNavigation.js and nsMsgDBView.cpp are shared anyway.
Carrying over r+ and rerequesting sr.

Comment 15

[David :Bienvenu]

Comment on attachment 246223 [details] [diff] [review]
SM + TB patch

thx, Karsten

Comment 16

[Karsten Düsterloh]

Landed on trunk.

Comment 17

[Karsten Düsterloh]

Comment on attachment 246223 [details] [diff] [review]
SM + TB patch

Asking for branch approval for SM1.1 and TB2 - low risk, fixes a long-standing UI inconsistency.

Comment 18

[Robert Kaiser]

Comment on attachment 246223 [details] [diff] [review]
SM + TB patch

Sounds good to me for SeaMonkey, a=me, I think the additional TB approval counts enough for checkin

Comment 19

[Scott MacGregor]

a=me for the thunderbird changes on the branch. 1.8.1.1 triage team, this has no impact on firefox.

Karsten, I wonder if we should add thunderbird-2 approval flag to these suite bugs? that would allow me to approve mailnews only bugs on the branch for us. I don't have the right privileges for 1.8.1.1.

Comment 20

[Karsten Düsterloh]

> Karsten, I wonder if we should add thunderbird-2 approval flag to these suite
> bugs?

Yeah, having TB flags for SM MailNews components sounds like a good idea.
 

Comment 21

[Karsten Düsterloh]

Landed on MOZILLA_1_8_BRANCH.

Comment 22

[Karsten Düsterloh]

Comment on attachment 246223 [details] [diff] [review]
SM + TB patch

Removing wrong branch flag, see comment #19 for details.

Comment 23

[Mike Cowperthwaite]

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