768025 - Mark multiple messages as read/unread needs improving (Mark -> As Read)

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, enhancement)

Description

[Philip Chee]

From Thunderbird Bug 575732:
> When selecting multiple messages, the "As Read" is either marked or not marked 
> based on the first selected message. This results in the need to sometimes 
> repeat the action twice for the desired effect. A split into two menu options 
> would resolve the issue.
> 
> Reproducible: Sometimes
> 
> Steps to Reproduce:
> 1. in a list of unread mail, select the first, and wait for it to become "read"
> 2. shift-click on the last unread mail to select them all.
> 3. mark -> as read
> Actual Results:  
> Since the "As read" will be checked, as the first select item has been changed 
> to "read", the result will be a change of the first message into "un-read" 
> state, 
> 
> Expected Results:  
> The desired effect would be in the above example to mark them all as read.

Also please note followup regression fixes to Bug 575732:
  TB Bug 628768 - mark as read toggle broken.
  TB Bug 641253 - Message > Mark menu shouldn't be disabled when no messages are selected

Comment 1

[Liu]

Hi, though I am a newbie here, still I want to contribute my part. Can u give me some clue about how to start dealing with this bug? Thanks.

Comment 2

[Philip Chee]

First you need to be able to build seamonkey. Please read <https://developer.mozilla.org/en-US/docs/Simple_SeaMonkey_build>. Also you can join us on IRC: irc://moznet/seamonkey where it's easier to ask questions and get replies. For help in getting seamonkey to build it's also useful to ask in irc://moznet/introduction .

Comment 3

[Ian Neal]

(In reply to Liu from comment #1)
> Hi, though I am a newbie here, still I want to contribute my part. Can u
> give me some clue about how to start dealing with this bug? Thanks.

Did you get anywhere with this?

Comment 4

[Liu]

Sorry, I was trying to fix another bug. Haven't gotten concrete process for this one.

(In reply to Ian Neal from comment #3)
> (In reply to Liu from comment #1)
> > Hi, though I am a newbie here, still I want to contribute my part. Can u
> > give me some clue about how to start dealing with this bug? Thanks.
> 
> Did you get anywhere with this?

Comment 5

[zephyr]

I wish to work upon this bug.

Comment 6

[Philip Chee]

> zephyr 2012-10-29 13:12:48 PDT
> I wish to work upon this bug.
Hi! Thanks for your offer. Have you read Comment 2 yet?

Comment 7

[zephyr]

(In reply to Philip Chee from comment #6)
> > zephyr 2012-10-29 13:12:48 PDT
> > I wish to work upon this bug.
> Hi! Thanks for your offer. Have you read Comment 2 yet?

yes,and I have built the /comm-central from mercurial.

Comment 8

[Philip Chee]

That's great! Here is the Thunderbird changeset that we need to adapt to SeaMonkey:

http://hg.mozilla.org/comm-central/rev/cc7fcc38e0f2

Our equivalent to the TB mail3PaneWindowCommands.js etc resides here in /suite/mailnews/:
http://mxr.mozilla.org/comm-central/find?text=&string=%2Fsuite%2Fmailnews%2F

The Thunderbird patch includes some mozmill tests. Since we don't run mozmill tests you can ignore that part.

The best way to contact us is to hop on to IRC and connect to the mozilla server:
irc://moznet/seamonkey. Depending on the timezone we may or may not be around so please be patient. Most of the SeaMonkey developers are in the EU and a couple of us are in Asia.

Comment 9

[zephyr]

(In reply to Philip Chee from comment #8)
> That's great! Here is the Thunderbird changeset that we need to adapt to
> SeaMonkey:
> 
> http://hg.mozilla.org/comm-central/rev/cc7fcc38e0f2
> 
> Our equivalent to the TB mail3PaneWindowCommands.js etc resides here in
> /suite/mailnews/:
> http://mxr.mozilla.org/comm-central/find?text=&string=%2Fsuite%2Fmailnews%2F
> 
> The Thunderbird patch includes some mozmill tests. Since we don't run
> mozmill tests you can ignore that part.
> 
> The best way to contact us is to hop on to IRC and connect to the mozilla
> server:
> irc://moznet/seamonkey. Depending on the timezone we may or may not be
> around so please be patient. Most of the SeaMonkey developers are in the EU
> and a couple of us are in Asia.

what is your irc nick?

Comment 10

[Philip Chee]

zephyr: on IRC I'm Ratty (or RattyAway) even if I'm not around leave me a message and I'll see it in the channel logs.

Comment 11

[Jenifer Wang]

Hi,

I would like to work on this bug with a friend. I'm currently building the environment and I'll get back with questions here or in IRC :)

Comment 12

[Jenifer Wang]

I was able to build SeaMonkey and I'm trying to understand the code and figure out where I should identify the issue at. At http://mxr.mozilla.org/comm-central/source/suite/mailnews/mailWindowOverlay.js#867 , what does gDBView and hdrForFirstSelectedMessage mean and what do they do?

Comment 13

[Jenifer Wang]

Attachment: First patch fix submission

(Worked on this with vchinen)

Instead of checking the first message, we check all selected messages to determine if the they should mark as read or unread.

Comment 14

[Tony Mechelynck [:tonymec]]

Comment on attachment 718262 [details] [diff] [review]
First patch fix submission

IIUC, this patch returns false if there is only one message and it is not read, or if there are more than one and any messages other than the first one are not read. It returns true if either there is only one message and it is read, or if there are more than one and all of them other than the first (which can be anything) are read. I don't understand the logic.

I guess there is an off-by-one error in the "for" statement, where "let i = 1" should be "let i = 0"; but then the whole test on .length == 1 becomes superfluous.

Comment 15

[Jenifer Wang]

From my understanding of the bug, 'mark as' should set to read from Comment 0. With the current code, it checks only the first line which by default when clicking will be marked as read so everything becomes unread then you would have to click it once more to mark the entire selection as read.

A scenario: 
In the mail system, you have 10 messages in your inbox with the following statuses:
[ ] Read
[ ] Read
[ ] Read
[ ] Unread
[ ] Unread
[ ] Unread
[ ] Unread
[ ] Unread
[ ] Unread
[ ] Unread

STEP 1:
You want to select the last 5 and mark as read so you select them by clicking the top-most and then shift clicking the bottom. It will become like this:
[ ] Read
[ ] Read
[ ] Read
[ ] Unread
[ ] Unread
[x] Read     <- Unread message becomes read on click
[x] Unread
[x] Unread
[x] Unread
[x] Unread

STEP 2:
Now you click 'Mark' button
[ ] Read
[ ] Read
[ ] Read
[ ] Unread
[ ] Unread
[x] Unread     <- This becomes unread
[x] Unread
[x] Unread
[x] Unread
[x] Unread

STEP: 3
You click 'Mark' button again
[ ] Read
[ ] Read
[ ] Read
[ ] Unread
[ ] Unread
[x] Read
[x] Read
[x] Read
[x] Read
[x] Read

If that's the case, the first selected message should be ignored if there's more than 1 message selected as first message will always becomes read. Then if only one message is selected, we will only check to see if the message is read or not (returns true if read else returns false). So instead of going from Steps 1, 2, and 3, it's Steps 1 and 3.

Comment 16

[Tony Mechelynck [:tonymec]]

Comment on attachment 718262 [details] [diff] [review]
First patch fix submission

Thanks for the explanation. I'm still not sure if it's the Right Thing to do (IanN will have to decide that, since he's been selected as reviewer) but at least I'm satisfied that it's intentional.

Comment 17

[Philip Chee]

Hi Jenifer!

Please join us on IRC so that we can discuss this further. We are on the Mozilla IRC server in the SeaMonkey dev channel (irc://moznet/seamonkey).

Comment 18

[Jenifer Wang]

Hello!

Should "Mark as Unread" be added into SeaMonkey like it has been done in Thunderbird? The current patch assumes that there is only one Mark button which can either be Read or Unread depending on what is being selected.

Link to open discussion:
https://groups.google.com/forum/#!topic/mozilla.dev.apps.seamonkey/FKitb0AwuME

Comment 19

[Ian Neal]

Comment on attachment 718262 [details] [diff] [review]
First patch fix submission

>+++ b/suite/mailnews/mailWindowOverlay.js
Please use 2 spaces for each indentation not 4.

> function SelectedMessagesAreRead()
> {
>+    var thisSelectedMessage;
>+    if (gFolderDisplay.selectedMessages.length == 1) {
>+        thisSelectedMessage = gFolderDisplay.selectedMessages[0];
>+        if (!thisSelectedMessage.isRead)
>+            return false;
>     }
I see no reason to special case a single message.
>+    for (let i = 1; i < gFolderDisplay.selectedMessages.length; ++i) {
so you might as well start with i = 0. As the number of selected messages is usually not too high there is probably no benefit from getting the selection count before the for loop, but I'd not object to it either.
>+        thisSelectedMessage = gFolderDisplay.selectedMessages[i];
>+        if (!thisSelectedMessage.isRead)
Just inline gFolderDisplay.selectedMessages[i]
>+            return false;
>     }
>+    return true;
> }
f+ for the moment as I would like to see the new patch.

Comment 20

[Jenifer Wang]

Attachment: Changed the patch in response to first feedback [Checked in: Comment 21]

Comment 21

[Ian Neal]

Comment on attachment 724281 [details] [diff] [review]
Changed the patch in response to first feedback [Checked in: Comment 21]

http://hg.mozilla.org/comm-central/rev/88e9e61f5506