214751 - Clean up ReverseThreads() and ReverseSort()

Status: RESOLVED FIXED

(MailNews Core :: Backend, enhancement)

Description

[[e-mail]]

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 :-)

Comment 1

[[e-mail]]

Attachment: Proposed patch

Comment 2

[[e-mail]]

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

Comment 3

[[e-mail]]

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

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

Comment 4

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

sorry for the spam.  making bugzilla reflect reality as I'm not working on these bugs.  filter on FOOBARCHEESE to remove these in bulk.

Comment 5

[David :Bienvenu]

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

this is obsolete, and I think Neil rolled this work into his nsTArray patch.

Comment 6

[[e-mail]]

Attachment: 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.

Comment 7

[David :Bienvenu]

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();
+  //}

Comment 8

[David :Bienvenu]

Re secondary sorting, it also needs to be finished, so that it does n level sorting.

Comment 9

[[e-mail]]

(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

[David :Bienvenu]

>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.

Comment 11

[[e-mail]]

Attachment: Updated for nits

Comment 12

[David :Bienvenu]

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 :-)

Comment 13

[[e-mail]]

Fix checked in.