Closed Bug 66466 Opened 21 years ago Closed 15 years ago

No cross-folder navigation using Next Unread Thread

Categories

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

All
Other
defect

Tracking

(Not tracked)

RESOLVED FIXED
Future

People

(Reporter: laurel, Assigned: mnyromyr)

References

Details

(Keywords: fixed-seamonkey1.1, fixed1.8.1.1)

Attachments

(1 file, 3 obsolete files)

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.
nominating for next release
Keywords: nsbeta1
QA Contact: esther → fenella
marking nsbeta1+
Priority: -- → P3
Whiteboard: [nsbeta1+]
Target Milestone: --- → mozilla0.9
marking nsbeta1- and moving to future milestone.
Keywords: nsbeta1nsbeta1-
Whiteboard: [nsbeta1+] → [nsbeta1+ 2/13]
Target Milestone: mozilla0.9 → Future
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.
With ID 2001072403

I have the following mail layout:

* mail account (japj@oce.nl)
* mail account (...)
* Local Folders
* news account
* news account

I have a message filter  that moves mail from the japj@oce.nl 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.
QA Contact: fenella → laurel
Laurel, can you please check if this is still reproducible (despite Seth's 
message navigation fixes recently) ?
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.
Attached patch proposed fix (obsolete) — Splinter Review
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.
Attached file Here's that IMAP debug (obsolete) —
This is the most current IMAP log.  Hope you find it helpful.
Blocks: 236849
Product: Browser → Seamonkey
Assignee: sspitzer → mail
Attachment #125522 - Attachment is obsolete: true
Attached patch updated patch to trunk (obsolete) — Splinter Review
David, if TB is interested in this fix, I can do that, too...
Attachment #87895 - Attachment is obsolete: true
Attachment #245914 - Flags: superreview?(bienvenu)
Attachment #245914 - Flags: review?(neil)
Assignee: mail → mnyromyr
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 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.
Attachment #245914 - Flags: review?(neil) → review+
Comment on attachment 245914 [details] [diff] [review]
updated patch to trunk

yes, I think this would be nice for TB as well, thx.
Attachment #245914 - Flags: superreview?(bienvenu) → superreview+
Attached patch SM + TB patchSplinter Review
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.
Attachment #245914 - Attachment is obsolete: true
Attachment #246223 - Flags: superreview?(bienvenu)
Attachment #246223 - Flags: review+
Comment on attachment 246223 [details] [diff] [review]
SM + TB patch

thx, Karsten
Attachment #246223 - Flags: superreview?(bienvenu) → superreview+
Landed on trunk.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: [nsbeta1+ 2/13]
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.
Attachment #246223 - Flags: approval1.8.1.1?
Attachment #246223 - Flags: approval-seamonkey1.1?
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
Attachment #246223 - Flags: approval-seamonkey1.1? → approval-seamonkey1.1+
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.
> 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.
 

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

Removing wrong branch flag, see comment #19 for details.
Attachment #246223 - Flags: approval1.8.1.1?
*** Bug 150970 has been marked as a duplicate of this bug. ***
No longer blocks: 236849
You need to log in before you can comment on or make changes to this bug.