52789 - Sort columns broken in bookmarks, history, file list, etc

Status: VERIFIED FIXED

(SeaMonkey :: Bookmarks & History, defect, P3)

Description

[Gilles Durys]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; WinNT4.0; en-US; m18) Gecko/20000915
BuildID:    2000091505

In history window and manage bookmark windows, the sort feature seems completely
broken. Nothing gets sorted as it should.

Reproducible: Always
Steps to Reproduce:
1. Open history or manage bookmark
2. Try to sort either by menu or by clicking on the columns


Actual Results:  Window is updated but nothing is sorted like it should							

Expected Results:  Sorted columns

Comment 1

[Gilles Durys]

*** Bug 52788 has been marked as a duplicate of this bug. ***

Comment 2

[sairuh (rarely reading bugmail)]

tree bug...?

Comment 3

[John Morrison]

Sorting works in e.g., mail trees, so this is particular to the sort logic
bound to the tree in bookmarks.js. -> slamm/bookmarks, but cc: blake, who
had some recent changes there (along with rayw's progid-ectomy) and rjc for
sorting backend (?).

To be specific, what I see when sorting a flat bookmarks list is that that 
the first and last rows are exchanged, but the rest of the rows are unchanged 
by the sort. (linux 2000091508).

Comment 4

[Blake Ross]

yes, there's a problem here with sorting via column headers and the view menu.  
Jag and I are pretty sure it isn't us, because we didn't really touch the sort 
logic, but jag has a partial fix.  cc'ing him.  This may be a result of the 
progid changes...

Comment 5

[Ben Bucksch (:BenB)]

BTW: default for history should be descending last visited.

Comment 6

[Blake Ross]

OK, I tried this out in a debug build, and I get an assertion whenever clicking 
on the column headers or the sort view menuitems:

###!!! ASSERTION: Parent content should not be NULL!: 'Error', file 
C:\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp, line 763

Stack:

KERNEL32! bff768a0()
nsDebug::Assertion(0x0279a114, 0x100c6460, 0x0279a0d8, 763) line 256 + 13 bytes
nsDebug::Error(0x0279a114, 0x0279a0d8, 763) line 420 + 22 bytes
nsXULTreeOuterGroupFrame::FindPreviousRowContent(13, 0x041d3590, 0x00000000, 
0x007ad124) line 763 + 21 bytes
nsXULTreeOuterGroupFrame::FindPreviousRowContent(13, 0x04207c40, 0x00000000, 
0x007ad124) line 837
nsXULTreeOuterGroupFrame::FindRowContentAtIndex(0, 0x00000000, 0x007ad124) line 
745
nsXULTreeFrame::GetItemAtIndex(0x02d3f954, 0, 0x007ad234) line 228 + 38 bytes
nsTreeBoxObject::GetItemAtIndex(0x042b7480, 0, 0x007ad234) line 130 + 31 bytes
nsXULElement::RemoveChildAt(0x04178fa0, 0, 0) line 2524 + 63 bytes
XULSortServiceImpl::SortTreeChildren(0x04178fa0, 0x007ad62c, 0) line 1811
XULSortServiceImpl::DoSort(0x04173c74, {...}, {...}) line 2428
XULSortServiceImpl::Sort(0x0301f5b0, 0x04173c74, 0x042d1120, 0x042b7a90) line 
2291 + 46 bytes
XPTC_InvokeByIndex(0x0301f5b0, 3, 3, 0x007ada9c) line 139
nsXPCWrappedNativeClass::CallWrappedMethod(0x040c6b20, 0x042b7c50, 0x042b7cc4, 
CALL_METHOD, 3, 0x02d65954, 0x007adc50) line 915 + 43 bytes
WrappedNative_CallMethod(0x040c6b20, 0x02d55e20, 3, 0x02d65954, 0x007adc50) 
line 226 + 34 bytes
js_Invoke(0x040c6b20, 3, 0) line 825 + 23 bytes
js_Interpret(0x040c6b20, 0x007ae784) line 2626 + 15 bytes
js_Invoke(0x040c6b20, 1, 2) line 842 + 13 bytes
js_InternalInvoke(0x040c6b20, 0x02d55a40, 47538504, 0, 1, 0x007ae91c, 
0x007ae8ac) line 914 + 20 bytes
JS_CallFunctionValue(0x040c6b20, 0x02d55a40, 47538504, 1, 0x007ae91c, 
0x007ae8ac) line 3162 + 31 bytes
nsJSContext::CallEventHandler(0x040c6cd0, 0x02d55a40, 0x02d56148, 1, 
0x007ae91c, 0x007ae918, 0) line 906 + 33 bytes
nsJSEventListener::HandleEvent(0x042d1b74) line 154 + 64 bytes
nsEventListenerManager::HandleEventSubType(0x04174970, 0x042d1b74, 0x04174ac8, 
4, 7) line 788 + 19 bytes
nsEventListenerManager::HandleEvent(0x040ee4a0, 0x007af1f4, 0x007af118, 
0x04174ac8, 7, 0x007af4ec) line 935 + 39 bytes
nsXULElement::HandleDOMEvent(0x04174ac0, 0x040ee4a0, 0x007af1f4, 0x007af118, 1, 
0x007af4ec) line 3323
PresShell::HandleEventInternal(0x007af1f4, 0x00000000, 0x007af4ec) line 4231 + 
45 bytes
PresShell::HandleEventWithTarget(0x040ed860, 0x007af1f4, 0x02d4064c, 
0x04174ac0, 0x007af4ec) line 4212 + 18 bytes
nsEventStateManager::CheckForAndDispatchClick(0x041c7130, 0x040ee4a0, 
0x007af5fc, 0x007af4ec) line 1836 + 59 bytes
nsEventStateManager::PostHandleEvent(0x041c7138, 0x040ee4a0, 0x007af5fc, 
0x02d4064c, 0x007af4ec, 0x040efee0) line 917 + 28 bytes
PresShell::HandleEventInternal(0x007af5fc, 0x040efee0, 0x007af4ec) line 4251 + 
43 bytes
PresShell::HandleEvent(0x040ed864, 0x040efee0, 0x007af5fc, 0x007af4ec, 1, 1) 
line 4166 + 23 bytes
nsView::HandleEvent(0x040efee0, 0x007af5fc, 28, 0x007af4ec, 1, 1) line 379
nsViewManager2::DispatchEvent(0x040ee150, 0x007af5fc, 0x007af4ec) line 1429
HandleEvent(0x007af5fc) line 68
nsWindow::DispatchEvent(0x040efda4, 0x007af5fc, nsEventStatus_eIgnore) line 614 
+ 10 bytes
nsWindow::DispatchWindowEvent(0x007af5fc) line 635
nsWindow::DispatchMouseEvent(301, 0x00000000) line 3811 + 21 bytes
ChildWindow::DispatchMouseEvent(301, 0x00000000) line 4021
nsWindow::ProcessMessage(514, 0, 2031824, 0x007af978) line 2889 + 24 bytes
nsWindow::WindowProc(0x00000e68, 514, 0, 2031824) line 883 + 27 bytes
KERNEL32! bff7363b()
KERNEL32! bff94407()
007a89fe()

Since slamm is gone and this is asserting in 
nsXULTreeOuterGroupFrame::FindPreviousRowContent, reassigning to hyatt.

Comment 7

[Blake Ross]

cc bryner, update summary, up severity

Comment 8

[Ben Bucksch (:BenB)]

blake, what about history?

Comment 9

[Brian Ryner (not reading)]

Hm, this goes through code I've messed with too (nsXULElement::RemoveChildAt).
I'll take a look.

Comment 10

[Brian Ryner (not reading)]

cc'ing waterson.  I think this may be another case where the tree is in an
invalid state during the sort because of the attempt to reduce notifications. 
I'm wondering if we need a similar fix to the one in XULTemplateBuilder awhile
back.  Waterson, what do you think?

Comment 11

[Peter "jag" Annema]

Okay, so the way I understand things, we're using getAttribute("resource")
somewhere in sorting to get the "rdf:resource" attribute. jst's recent checkin
fixed getAttribute, since we should be using getAttributeNS for getting
attributes on different namespaces. Thus, things broke. However, according to
jst, we shouldn't be using "rdf:resource" to begin with, just "resource".

hyatt, if you agree with the suggested fix (remove "rdf:" from those
attributes), feel free to assign this one to me and I'll attach a patch when I'm
done fixing and testing.

Comment 12

[Chris Waterson]

Yes, that is the correct fix.

Comment 13

[Peter Trudelle]

reassigning to jag. For checkin, get permission of bookmarks/history owners, and
please have ben super-review it.

Comment 14

[Chris Waterson]

Like bryner said, it looks like we also need to fix nsXULSortService::DoSort()
to remove the content from the tree *before* sorting. This caused a bunch of
problems elsewhere. So jag, after you fix the "rdf:resource" stuff, send it back
to rjc.

Comment 15

[Brian Ryner (not reading)]

This patch fixes the assertion.  It should go along with the fix to the js.

Comment 16

[Brian Ryner (not reading)]

Attachment: patch

Comment 17

[sairuh (rarely reading bugmail)]

affects the xul file picker (open file, save file), so adding self to cc list.

Comment 18

[Peter "jag" Annema]

This bug depends on bug 53297, which I've posted for all 'rdf:' removal where
needed.

Comment 19

[Peter "jag" Annema]

Checked in part of the fix for 53297 which fixed the sorting front end.

Handing it over to [e-mail].

Comment 20

[Daniel Veditz [:dveditz]]

Updating summary based on my understanding of comments in the bug.

Comment 21

[Robert John Churchill]

Attachment: Diff to nsXULSortService to use rdf:type (RDF namespace)

Comment 22

[Robert John Churchill]

I've attached a small diff regarding sorting (file: nsXULSortService)... now that 
namespaces are distinct, since the Bookmarks service specifically uses rdf:type, 
nsXULSortService needs to look for rdf:type instead of {}:type.

With the diff applied, you'll notice when sorting bookmarks (either the window or 
the sidebar panel), sorts will happen in-between separators. Without the patch, 
the entire tree will be sorted and separators were being ignored, i.e. they'd all 
get sorted to the top or bottom.

Anyone care to review the diff?

Comment 23

[Peter "jag" Annema]

Fix the assertion message: XUL -> RDF

Also, why not put kXULNameSpaceURI near kRDFNameSpaceURI near ine 413? It's the 
only place it's used anyway.

Comment 24

[Robert John Churchill]

Attachment: Diff to nsXULSortService to use rdf:type (RDF namespace) [Tweak assertion msg]

Comment 25

[Robert John Churchill]

Tweaked brain-dead assertion msg.  Let's keep statics at the top o' the file.

Comment 26

[Peter "jag" Annema]

Looks a lot better. r=jag, though I would like someone who's a bit more into 
this to review too.

Comment 27

[Brendan Eich [:brendan]]

Other than the tabs (can't you get CodeWarrior to expand them to 4-space stops?)
I can only say that the NS_ConvertASCIItoUCS2 call on a string constant is now
better done via NS_LITERAL_STRING -- or so I hear.  Cc'ing scc for a definitive
opinion.

/be

Comment 28

[Scott Collins]

Yes, particularly since you convert the same string multiple times; this would be 
better rendered as

  NS_NAMED_LITERAL_STRING(literalNameSpaceURI, "the string here");
  // ...
  ...RegisterNameSpace(literalNameSpaceURI, ...
  ...RegisterNameSpace(literalNameSpaceURI, ...

unfortunately, macro expansion happens in the wrong order to use a |#define| for 
the string argument to |NS_LITERAL_STRING| or |NS_NAMED_LITERAL_STRING|, as hyatt 
discovered.  So watch out.  And yes, your real tabs are annoying :-)

Comment 29

[Robert John Churchill]

>  Yes, particularly since you convert the same string multiple times;



Ah, Scott... two different strings are each being converted once.



and since NS_NAMED_LITERAL_STRING can't easily be used here due to #defines which 

are sucked in from elsewhere, what's the point?



I'm going to start saying "thusly" to Brendan a lot.

Comment 30

[Scott Collins]

Oops.  Sorry I misread the diff.  OK, given that this code path is executed once 
per invocation of that app (from IRC conversation with rjc), the savings for 
making these be literal strings, over the cost of not using a |#define| for the 
strings is negligible.  I'm happy to r= this as it stands, with respect to its 
use of strings.

Comment 31

[Robert John Churchill]

I checked in my patch.

bryner, your patch doesn't seem to work for me though... after applying it, doing 
a sort doesn't seem to cause a reflow/update.  I'm bouncing this bug over to you 
so that you can take a look.

Comment 32

[Brian Ryner (not reading)]

rjc: wow, good catch.  I'm not sure exactly what is going on there.

I'd recommend that we hold off on this change since it seems to put sorting in
jeopardy and it appears that the assertion can be avoided by using the right
namespace.  Do you agree?

Comment 33

[Brian Ryner (not reading)]

I stand corrected.  This assertion still happens (although sorting works).  I'll
investigate.  Should I file a new bug, or keep it on this one?

Comment 34

[selmer (gone)]

Is there still some fix being proposed for beta3?

Comment 35

[Brian Ryner (not reading)]

There is no fix in hand, but there still is apparently something not quite right
going on during the sort that causes the tree to assert.

However, based on the original description, this bug IS fixed.  I'm marking it
as such and filing bug 53686 on the assertion.

Comment 36

[sairuh (rarely reading bugmail)]

hm, the default sort (alphanumeric by filename) is fixed in the file picker (i
checked using 2000.09.22.08 opt comm bits on linux). however, clicking the
*columns* in the filepicker does nothing. (although clicking the columns in the
bookmarks and history window does work.)

let me know if what am seeing in the file picker is a "feature", tho'. ;)

Comment 37

[Claudius Gayle]

VERIFIED Fixed on all platforms with 2000092213 builds

Comment 38

[Gilles Durys]

With build 2000092220 on Win2000, in history, sorting by URL is still broken
(title and last visited is ok)

Comment 39

[Peter "jag" Annema]

You're right. For some reason, we don't seem to sort on the NC-rdf#URL resource.
In case two urls are the same, we secondary sort on NC-rdf#Name, but since we
don't sort on the URL, that becomes our primary sorting now.

Comment 40

[Brian Ryner (not reading)]

back over to rjc... this is definitely your area

Comment 41

[Peter Trudelle]

PDT marking nsbeta3-/PDTP3

Comment 42

[Robert John Churchill]

Here's a fix for why sorting doesn't work on the #URL column in the history 
window.  Chris (Waterson), care to review?

Basically, when asking a node for its "#URL" arc, the sorting code expects a 
literal back, not a resource...

Comment 43

[Robert John Churchill]

Attachment: Fix for sorting on the #URL column in the History window

Comment 44

[Chris Waterson]

I'm pretty sure you want to use NS_ConvertUTF8toUCS2 here. I think you should be
able to say...

  gRDFService->GetLiteral(NS_ConvertUTF8toUCS2(uri),
                          getter_AddRefs(literal));


or...

  gRDFService->GetLiteral(NS_ConvertUTF8toUCS2(uri).GetUnicode(),
                          getter_AddRefs(literal));

Comment 45

[Robert John Churchill]

Attachment: OK, use NS_ConvertUTF8toUCS2

Comment 46

[Chris Waterson]

Looks good!

Comment 47

[Scott Collins]

([s]r|a)=scc on the 10/02/00 10:25 patch, as well, if you need it

Comment 48

[Daniel Veditz [:dveditz]]

[rtm+] for reviewed and super reviewed patch

Comment 49

[Robert John Churchill]

.

Comment 50

[Claudius Gayle]

dang rjc, help us out here. resolved-->fixed, with a comment of "." means you checked this into
the trunk on 10/04 and it should show up in trunk opt. build on 10/05, right?

keep in mind that the only time I know that "." = fixed is when I read the bugzilla email. Later
when I (or anyone) really looks at this I will have no clue what the hell went on.

Comment 51

[Robert John Churchill]

.

Comment 52

[sairuh (rarely reading bugmail)]

mass-verifying claudius' Fixed bugs which haven't changed since 2001.12.31.

if you think this particular bug is not fixed, please make sure of the following
before reopening:

a. retest with a *recent* trunk build.
b. query bugzilla to see if there's an existing, open bug (new, reopened,
assigned) that covers your issue.
c. if this does need to be reopened, make sure there are specific steps to
reproduce (unless already provided and up-to-date).

thanks!

[set your search string in mail to "AmbassadorKoshNaranek" to filter out these
messages.]