503624 - Prefs to mark all RSS or NNTP messages read when exiting the folder no longer work

Status: RESOLVED FIXED

(Thunderbird :: Preferences, defect)

Description

[Dan]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1) Gecko/20090630 Firefox/3.5 (.NET CLR 3.5.30729) (tete009 SSE2 PGO)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1pre) Gecko/20090613 Lightning/1.0pre Shredder/3.0b3pre

For every build after 6/13 the about:config preferences mailnews.mark_message_read.nntp and mailnews.mark_message_read.rss no longer work.

When you exit an RSS field or Newgroup list the unread messages are not automatically marked read.  

This seems to belong as part of the tracking bug for 497199 and is extremely important for me.  I skim many different RSS feeds and NNTP newsgroups and do not read every message and have my views set to only show read messages.  If I now have to manually mark everything as read it is a HUGE usability setback.

Reproducible: Always

Steps to Reproduce:
1. Set prefs to true in about:config editor
2. Go into an RSS feed or NNTP folder
3. Switch to another folder with unread message in the original RSS or NNTP folder
Actual Results:  
Unread messages are NOT marked as read.

Expected Results:  
Unread messages should all be marked as read.

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

The underlying problem is bug 498363.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

I'd like to see this fixed for Tb3, but I'm hesitant to mark it a blocker, since it's only available by hidden preference.

I actually think this behavior might really want to be part of a "triage mode" that's protocol agnostic, since it's likely to be useful for high-traffic mailing lists too.  It would be interesting to put together a "triage mode" extension.

Adding a few folks who might have relevant thoughts here.

Comment 3

[Dan]

I realize it's only a hidden preference, but for me I could not use feeds without it...having to manually mark every single feed as read is incredibly tedious.  My fear is that many users probably DO have it enabled because it's such a time saver.  Naturally if there is an extension to do it that would be fine, but until then I have to stay on the 6/12 build and thus cannot test and comment on some of the more interesting things happening.

On that note I'm also becoming distressed that features are being taken away (this one, the collapsed header view etc.), while new things added that may or may not appeal to new users. 

I strongly feel that no features should be taken away from TB3 unless restoring them would be incredibly time consuming, or there is certainty that most users will not be affected.  

Please -- I love TB and was very happy with the progress until some of the patches landed that made some of my most important features no longer work.

Comment 4

[David :Bienvenu]

I had a patch in some bug that fixed this problem but haven't had a time to come up with a test for it, so I haven't gotten approval for it yet. I need to go find the bug...

Comment 5

[David :Bienvenu]

I believe the patch in bug 498363 should fix this.

Comment 6

[David :Bienvenu]

Attachment: proposed fix with unit test

this fixes the bug, and adds a unit test - I had to tweak the test harness a little (should have used mozmill, but I was pretty far down the road when I realized how the listener stuff worked). Besides calling the wrong method, the mark read on exit code was missing a Ci. before nsMsgViewCommandType.

Comment 8

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 390345 [details] [diff] [review]
proposed fix with unit test

I assume I am the intended reviewer

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 390345 [details] [diff] [review]
proposed fix with unit test

I think doing the test in viewWrapper is correct.  Since the full feature
spills into FolderDisplayWidget, I guess a mozmill test would be nice too, but
it's good to have one here.


on file: mailnews/base/test/unit/test_viewWrapper_realFolder.js line 717
>   assert_true(folder.getNumUnread(false) == expectedUnreadCount);

When using assert_true, please take advantage of the support for the second
argument to be a string that explains went wrong.  (Also, in this case, you
can use assert_equals, which still takes an aWhy).

If you don't, it might be preferable to just use xpcshell's "do_check_eq".  It
does not take an explanation, but it does show the values it compared, which
is better than nothing.


on file: mailnews/base/test/unit/test_viewWrapper_realFolder.js line 718
>   if (viewWrapper.listener.shouldMarkMessagesReadOnLeavingFolder(folder))
>     expectedUnreadCount = 0;

Please lose the conditional here.  The test is intending to force the mark
read logic to be active, so we should assume that it is.  Helps avoid testing
glitches like you caught in my nsMsgDBView tests the other day :)  (Not that
you had a glitch here.)

Comment 10

[David :Bienvenu]

fix checked in, with comments addressed...