Use SUM instead of COUNT for is_new in the list messages query

Interesting....

What about using a similar style as in the issue queue:
- Display a new/updated text after the subject, instead of the number of new messages (new: sum() == count(), updated: sum() > 0
- Display the amount of new messages in the answers column: 5 (3 new), where 3 new is a link with #new appended.

Oh, and "&& $thread['is_new'] <> 0" should not be needed, because empty(0) == FALSE.

I'm wondering if we should change that to Messages or so, instead of replies.

Another possibilty would be not to display "x new" for new messages. That's how d.o is doing it, see for example at http://drupal.org/project/issues/search/drupal?version[0]=156281&status[... and look for new issues with 0 replies.

Re-rolled, changes:

- use theme_mark() to display new/updated instead of a bold subject. This can easily be overwritten if someone wants to them it differently.
- Renamed Reply/Answers to Messages, display the "first" message now too
- Only display number of new messages for "updated" threads

Note: There is still a "bug" when you write a message to yourself.. you are count twice. This is not solvable as part of this patch.

I'm not liking the 'updated' mark when there are new messages in a thread, I look at a thread with 'new' and a thread with 'updated' and think: 'What's the difference?' Also, to me, 'updated' means the original post has been edited or changed in some way, which is clearly incorrect.

Also, Views doesn't print an upated mark for new comments (see screenshot attached). Maybe this is just tracker.module behaviour? In which case, we should probably mimic Views as tracker is quite likely to be replaced by it in Drupal 7.

Saying that, I can see the value of updated vs new as I will in most likelihood NOT be showing the count column

With that use case I can see the value of showing something, but it shouldn't be 'updated' in my opinion.

'new replies' or 'unread replies' -- or words to that effect -- would work well. Then the user won't have to learn what 'updated' means in this context. 'updated' is just bad usability. Think of the users! Won't somebody think of the users? ;)

This is definitely a step in the right direction, in my opinion. :)

Related to this is: #442042: Give messages anchors as this patch links to #new in the message thread list, this is required to add #new anchors to messages.

Using this patch. Not qualified to review the code, but in the look and feel dept. it's very nice addition.

This sounds really nice, but the patch doesn't apply anymore: ALL hunks failed. I'm not attempting a re-roll, as it's likely to be a complete re-write of the patch.

Oh, now I see: The patches here have Windows line-endings (CR+LF). This is difficult to see, as hexadecimal file dump is not exactly common tool, and normal text editors or browsers don't show that, but I quite never found a way to make such patches apply on my Linux-based environment. (Is there some simple way?) Windows suck.

I just observed the CR/LF pairs in the file, after dumping it byte-to-byte through a php script. I'm always applying patches by plain command-line 'patch' command on Linux, so that's pretty much "native" environment I guess. But I have nothing more to say to this, sorry. Just stated my findings here, to explain why I got these failures, and that it most probably doesn't affect the real contents of the patch.

some minor coding style stuff, then we can set that to RTBC, as it has been tested on nbz's for quite some time afaik.

+  $vars['message_anchors'][] = 'privatemsg-mid-'.$vars['mid'];

That should contain a space between . and $. Same for some other instances. Hint: D7 has changed coding style to include a space between ' and . too, not sure which one we want to follow. Always having spaces around . is imho better.

+  $fragments['select'][]      = 'SUM(pmi.is_new) as is_new';

We would have to rename some functions, but it might make sense to rename is_new to something like num_new or sum_new, to reflect what it actually is...

SUM(pmi.is_new) as is_new
I suspect this (untested) to have problems on messages sent to oneself, where we have two records for the same message, both set to "new". Since we decided NOT to remove duplicates from the index table over at #490650: Message sent to oneself breaks new/#replies data loading, we need to keep an eye on duplicate-rows-related behavior everywhere.

Can we use GROUP BY uid in pm_index to bypass duplicate record?

What about grouping by mid ?

Hm, is it correct that we add the "new" anchor to every new message?

According to http://webdesign.about.com/od/htmltags/p/blatnamea.htm, that is not valid: "This attribute names the current anchor so that it can be the destination of another link or anchor. The value must be unique and it shares the same namespace as the id attribute."

Oh, and there is still a spacing issue in the $anchor loop ;)

Ok, I reviewed the patch in detail and it looks good. I also tested it, it works fine and as expected. Additionally, it generates valid XHTML 1.0 Strict, wohooo! ;)

Let's get this in and RC3 rolled out, it's about time! ;)

Cheers :)