If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Clean up ReverseThreads() and ReverseSort()

RESOLVED FIXED

Status

MailNews Core
Backend
--
enhancement
RESOLVED FIXED
14 years ago
9 years ago

People

(Reporter: neil@parkwaycc.co.uk, Assigned: neil@parkwaycc.co.uk)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

14 years ago
1. Use local objects instead of locally scoped heap allocations
2. Use CopyArray instead of InsertAt
3. Change return types to void
Plus other odds & ends :-)
(Assignee)

Comment 1

14 years ago
Created attachment 129018 [details] [diff] [review]
Proposed patch
(Assignee)

Updated

14 years ago
Attachment #129018 - Flags: superreview?(bienvenu)
Attachment #129018 - Flags: review?(bienvenu)
(Assignee)

Comment 2

14 years ago
Comment on attachment 129018 [details] [diff] [review]
Proposed patch

Heh, two bugs already:

>+        if (m_flags.GetAt(startThread) & MSG_VIEW_FLAG_ISTHREAD)
Better to check !(m_levels.GetAt(startThread)

>+    for (PRUint32 i = 0; i < --end; i++) 
Needs to be i <= end-- or equivalent
(Assignee)

Comment 3

14 years ago
Comment on attachment 129018 [details] [diff] [review]
Proposed patch

Um, no, that's still wrong :-[ I should check for GetSize() [!= 0]

Updated

14 years ago
Blocks: 236849
Product: MailNews → Core
sorry for the spam.  making bugzilla reflect reality as I'm not working on these bugs.  filter on FOOBARCHEESE to remove these in bulk.
Assignee: sspitzer → nobody

Comment 5

10 years ago
Comment on attachment 129018 [details] [diff] [review]
Proposed patch

this is obsolete, and I think Neil rolled this work into his nsTArray patch.
Attachment #129018 - Attachment is obsolete: true
Attachment #129018 - Flags: superreview?(bienvenu)
Attachment #129018 - Flags: review?(bienvenu)
(Assignee)

Comment 6

10 years ago
Created attachment 321069 [details] [diff] [review]
Updated for bitrot

While I was updating this I spotted some bugs in secondary sorting, but I'll address that in a separate bug since it needs a serious rewrite.
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #321069 - Flags: superreview?(bienvenu)
Attachment #321069 - Flags: review?(bienvenu)

Comment 7

10 years ago
Comment on attachment 321069 [details] [diff] [review]
Updated for bitrot

very nice - some nits:

         // since we won't call ReverseSort() if we
         // are in threaded mode, so m_levels are all the same.

This was wrong before, but while you're here, I think that should be "if we aren't in threaded mode".

+    PRUint32 end = GetSize();
...
+    for (PRUint32 i = 0; i < --end; i++) 

this code would be clearer if "i" was called topHalfIndex and end was called bottomHalfIndex, or something like that.

In ReverseThreads, if I'm reading the code correctly, endThread is not really the index of the last message in the thread, but one past it, i.e., the startOfNextThread (except at the bottom :-) ) So either a new var name, or just a comment in the code to that effect.

Might as well just remove this:

+  //if (sortOrder == nsMsgViewSortOrder::descending)
+  //{
+    //ReverseSort();
+  //}
Attachment #321069 - Flags: superreview?(bienvenu)
Attachment #321069 - Flags: superreview+
Attachment #321069 - Flags: review?(bienvenu)
Attachment #321069 - Flags: review+

Comment 8

10 years ago
Re secondary sorting, it also needs to be finished, so that it does n level sorting.
(Assignee)

Comment 9

10 years ago
(In reply to comment #7)
>          // since we won't call ReverseSort() if we
>          // are in threaded mode, so m_levels are all the same.
> 
> This was wrong before, but while you're here, I think that should be "if we
> aren't in threaded mode".
It is correct; but perhaps you would like it reworded to
"since we only call ReverseSort if we aren't in threaded mode"

> +    PRUint32 end = GetSize();
> ...
> +    for (PRUint32 i = 0; i < --end; i++) 
> 
> this code would be clearer if "i" was called topHalfIndex and end was called
> bottomHalfIndex, or something like that.
I can do that, but keeping the same names made the patch clearer.

> In ReverseThreads, if I'm reading the code correctly, endThread is not really
> the index of the last message in the thread, but one past it, i.e., the
> startOfNextThread (except at the bottom :-) ) So either a new var name
How about lastThread? Or will nextThread do?

(In reply to comment #8)
> Re secondary sorting, it also needs to be finished, so that it does n level sorting.
Ah, well it would help if it did 2 level sorting correctly in all cases. (In fact I think there are cases where it doesn't do 1 level sorting correctly.)

Comment 10

10 years ago
>It is correct; but perhaps you would like it reworded to
>"since we only call ReverseSort if we aren't in threaded mode"

duh, you're right. Maybe:

"Since we only call ReverseSort in non-threaded mode, m_levels are all the same."

>How about lastThread? Or will nextThread do?
nextThread is better, since it's only lastThread at the very start.

>Ah, well it would help if it did 2 level sorting correctly in all cases. (In
>fact I think there are cases where it doesn't do 1 level sorting correctly.)
Sure, I was just trying to say, if you rewrite it, it would be nice to avoid having to rewrite it again to do n level sorting.
(Assignee)

Comment 11

10 years ago
Created attachment 322231 [details] [diff] [review]
Updated for nits
Attachment #322231 - Flags: superreview?(bienvenu)

Comment 12

10 years ago
Comment on attachment 322231 [details] [diff] [review]
Updated for nits

when I think of bottom and top index, I think of the top and bottom in the view, visually, but if I think of bottom as the lower number and top as the higher number, it all makes sense :-)
Attachment #322231 - Flags: superreview?(bienvenu) → superreview+
(Assignee)

Comment 13

10 years ago
Fix checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Product: Core → MailNews Core
No longer blocks: 236849
You need to log in before you can comment on or make changes to this bug.