156930 - Views: Need better handling of Next in this case for QuickSearch,Watched View

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[laurel]

Using current branch or trunk

When in a QuickSearch results view, Next unread does indeed function (good) --
if there are unread items in the view you can navigate or if there are no unread
items in the view and none in the folder, Next will invoke cross-folder/account
navigation.  However, in the case where there are unread items in the folder
view and no unread items in the results view, Next is enabled but doesn't do
anything.

We should handle this better, by either disabling the button or providing
cross-folder navigation or maybe an alert asking if the user wants to go to the
folder view for further unread item navigation.  I'm not sure which is best, but
it is confusing the way it stands now (catches me off-guard quite often).

Note:  this also applies if using the Watched Threads with Unread message view,
since there can be no unread in the view while still unread items in the folder.
 In this case, 4.x (Windows) engages cross-folder navigation.

Steps:
1.  In the 3-pane mail window, select a folder with many unread items.
2.  Type text in the QuickSearch bar which will yield results containing some,
but not all, of the unread items in the folder.
3.  Select a result, then click Next button... goes to next unread in the search
view.  Continue until there aren't any unread items in the search view.
4.  Next is still enabled -- click it.  Nothing happens.

Result:  Nothing happens when clicking Next when in a Search/Watched view having
no unread items while there are still some unread items in the folder.

Expected:  Something should happen. We should either invoke cross-folder
navigation (4.x did this with Watched view), provide an option/clue to the user
to go to folder view (?), or disable the button in this situation.

Comment 1

[laurel]

nominating for next release 

Comment 2

[laurel]

I think this will become more important since the new Views dropdown will
increase possibilities of people having unread items which are in the folder,
but not in the view.  We need a way for them to handle navigation across
folders/accounts when in views without being too confusing or cumbersome.

Comment 3

[laurel]

To add to the last comment... even if the user has junk mail controls on, but
doesn't choose to have junk mail moved to a junk folder, if they view mail
folders as Not Junk, many junk messages will be unread and appear outside of the
view.  This is a prefab problem, unless we decide to mark junk as read as part
of the controls...

Comment 4

[laurel]

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

Comment 5

[Samir Gehani]

Mail triage team: nsbeta1+/adt2

Comment 6

[jglick]

In this situation (user selects Next and no Unread msgs remain in folder), if no
Views or QS results are being displayed, we invoke cross-folder navigation
correct? If so, we should do the same in this case as well.

Comment 7

[jglick]

If thats not doable, we should disable the Next button.

Comment 8

[Samir Gehani]

-> Seth

Comment 9

[[e-mail]]

Attachment: Proposed patch

I've changed the meaning of FindNextChildFolder slightly.
Firstly, it doesn't search the current folder. This means that if unread
messages are hidden by quick search or mail views, then cross folder navigation
won't get confused if you have any unread messages in other folders.
Secondly, I've added a parameter indicating the child folder after which to
begin searching, which simplifies the code that searches parent folders.
Servers are not affected as they have no messages.

Comment 10

[(not reading, please use [e-mail] instead)]

laurel points out some issues, but I highly I doubt we'll ever address them
completely.

persistent mail views (views persist across folders) and per folder remembering
of the sort / view will need to some issues with next unread cross folder
navigation.

imagine a folder view for "is not unread".  while other folders might have
unread messages, since views persist, when you land on the folder, you won't
have anything to load.

to really address this problem, we'd have to build a view up for potential
folders before landing on them, to verify there was unread.  (I doubt we're
going to ever do that, it's costly)

I think for now, landing on a folder with unread, even if there are no unread
messages (due to mailviews or sort/view) is reasonable, and given the defaults.

I'll go review neils patch...

Comment 11

[(not reading, please use [e-mail] instead)]

Comment on attachment 114095 [details] [diff] [review]
Proposed patch

I'm not a big fan of changing the values of "in" arguments to a function inside
the function.
that can have unintended side effects. 

instead of

+      if (current) {
+	 if (current == folder)
+	   current = null;
+      } else {
+	 // if there is unread mail in the trash, sent, drafts, unsent messages

why not:

+	 if (current != folder) {
+	 // if there is unread mail in the trash, sent, drafts, unsent messages

performance isn't a issue here, and that check isn't that expensive.

also, since you are fixing up this code, can you make a patch with a switch
from:

+function FindNextChildFolder(folder, current)

to

+function FindNextChildFolder(aFolder, aCurrent)

the aFoo notation really helps readability.

I'll continue my review tomorrow.

Comment 12

[(not reading, please use [e-mail] instead)]

Comment on attachment 114095 [details] [diff] [review]
Proposed patch

neil answered my questions over email.

r/sr=sspitzer, nice work.

but can you address a few nits before checking in?

1)

can you add a comments to explain exactly what FindNextChildFolder() does?

It's non-trival, and I fear someone will tweak it and cause a regression.

2)

neil explained over email that what this does is skips over folders up to and
including the folder you pass in.

+      if (current) {
+	 if (current == folder)
+	   current = null;
+      } else {

Can you include that as a comment in the code?


3)

I'm not a big fan of changing the values of "in" arguments to a function inside
the function, so instead of assigning to aCurrent, can you use a local boolean
instead?

4)

FindNextChildFolder(folder, current)

since you are rewriting this function, can you do aFolder and aCurrent (for
readability?)

5)

please add comments to FindNextFolder() as well, to make sure it will be clear
how we expect that code to behave.

Again, and I fear someone will tweak it and cause a regression.

6)

please make sure to test well before checking in.  as you know, this code is
fragile.

once you attach a final patch with cleanup and comments, I'll note the r/sr.

I think this can wait until 1.4 alpha.

Comment 13

[[e-mail]]

Attachment: Fixed patch

1. Done.
2. Done.
3. I avoided an extra variable by adding an extra loop.
4. Done.
5. I looked at the existing comments, and they seem ok.
6. Retested and found a bug handling special folders.

Comment 14

[(not reading, please use [e-mail] instead)]

Comment on attachment 114429 [details] [diff] [review]
Fixed patch

r/sr=sspitzer

thanks for making those extra changes.

Comment 15

[[e-mail]]

Fix checked in.

Comment 16

[(not reading, please use [e-mail] instead)]

fyi, this caused bug #194725, which has been fixed (by neil)

Comment 17

[Ninoschka Baca]

Trunk build 2003-03-06: Mac 10.1.5
Trunk build 2003-03-07: WinXP
Verified Fixed. Cross-folder navigation is working using the scenario originally
reported.