Last Comment Bug 335122 - Clean up XUL Sort Service
: Clean up XUL Sort Service
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: ---
Assigned To: Neil Deakin
:
: Neil Deakin
Mentors:
http://wiki.mozilla.org/XULSortServic...
Depends on: 350753 369423 460961
Blocks: 321171
  Show dependency treegraph
 
Reported: 2006-04-22 18:27 PDT by Neil Deakin
Modified: 2008-11-14 04:40 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Improve sort service code, as described (137.00 KB, patch)
2006-05-25 13:54 PDT, Neil Deakin
no flags Details | Diff | Splinter Review
Fixed review comments (130.23 KB, patch)
2006-07-12 14:23 PDT, Neil Deakin
jvarga: review+
jonas: superreview+
Details | Diff | Splinter Review

Description Neil Deakin 2006-04-22 18:27:45 PDT
Goals:

1. Move the sort inserting code directly into the content builder.
2. Make the sort inserting code and the container sort algorithms use the query processor for sorting such that they work on all datasources, rather than just RDF.
3. Make how one specifies the sort info consistent.
4. Simplify the sort code 
5. Allow sorting non-template built content

See http://wiki.mozilla.org/XULSortService#XUL_Sort_Service_Improvements for more info
Comment 1 Neil Deakin 2006-05-25 13:54:31 PDT
Created attachment 223368 [details] [diff] [review]
Improve sort service code, as described
Comment 2 Jan Varga [:janv] 2006-05-25 14:25:00 PDT
Nice cleanup, Neil
I went through the patch and I think the new sort architecture is great.
Comment 3 Neil Deakin 2006-06-04 09:30:24 PDT
So Jan, are you offering to review this patch?
Comment 4 Jan Varga [:janv] 2006-06-21 05:33:25 PDT
Yeah, I'll review it, but could you also fix the case when one of the results is null?

I fixed in Firefox 1.0 by adding these checks:

XULSortServiceImpl::CompareNodes()
...
 
+  if (!cellNode1 && cellNode2) {
+    sortOrder = -1;
+    return NS_OK;
+  }
+
+  if (cellNode1 && !cellNode2) {
+    sortOrder = 1;
+    return NS_OK;
+  }
+                        

nsXULTreeBuilder::CompareMatches()
...
+    if (!leftNode && rightNode)
+        return -1;
+
+    if (leftNode && !rightNode)
+        return 1;
+
Comment 5 Neil Deakin 2006-07-10 13:02:39 PDT
Jan, I can make that change. It just requires the same change you made but in nsXULTemplateQueryProcessorRDF::CompareResults. I assume you don't need a new patch?

Comment 6 Jan Varga [:janv] 2006-07-10 13:18:21 PDT
ok, no need for a new patch for now
I'll review it tonight or tomorrow
Comment 7 Jan Varga [:janv] 2006-07-11 03:11:45 PDT
Comment on attachment 223368 [details] [diff] [review]
Improve sort service code, as described

>+  nsCOMPtr<nsIDOMElement> element;
>+  aCol->GetElement(getter_AddRefs(element));
>+  if (mRoot && element) {

nit, it would a little bit more efficient to check for !mRoot

if (!mRoot)
  return NS_OK;

nsCOMPtr<nsIDOMElement> element;
aCol->GetElement(getter_AddRefs(element));
if (element) {


>+        if (sortdirection.EqualsLiteral("ascending"))
>+          sortdirection.AssignLiteral("descending");
>+        else
>+          sortdirection.AssignLiteral("ascending");

no support for 3-state (including natural) sorting?

I have patch somewhere that adds support for "two-state-sorting" flag (e.g. flags="dont-build-content two-state-sorting)


>+   * The type of the object. The predefined value 'separator' may be used
>+   * for separators. Other values may be used for application specific
>+   * purposes.
>+   */
>+  readonly attribute AString type;
>+

PRInt32 instead of string?

>+    if (mSortState.direction == nsSortState_natural && mQueryProcessor) {
>+        // sort in natural order
>+        nsresult rv = mQueryProcessor->CompareResults(aResult, match->mResult,
>+                                                      nsnull, aSortOrder);
>+        if (NS_FAILED(rv))
>+            return rv;
>+    }
>+    else {
>+        // iterate over each sort key and compare. If the nodes are equal,
>+        // continue to compare using the next sort key. If not equal, stop.
>+        PRInt32 length = mSortState.sortKeys.Count();
>+        for (PRInt32 t = 0; t < length; t++) {
>+              nsresult rv = mQueryProcessor->CompareResults(aResult, match->mResult,
>+                                                            mSortState.sortKeys[t], aSortOrder);
>+            if (NS_FAILED(rv))
>+                return rv;
>+
>+            if (*aSortOrder)
>+                break;
>+        }
>+    }

if (!mQueryProcessor)
  return NS_OK;

if (mSortState.direction == nsSortState_natural) {
...

>+    if (!sortkey.IsEmpty() && sortkey[0] != '?' &&
>+        !StringBeginsWith(sortkey, NS_LITERAL_STRING("rdf:")) &&
>+        mDB) {
>+        // if the sort key is not a template variable, it should be an RDF
>+        // predicate. Get the targets and compare those instead.
>+        nsCOMPtr<nsIRDFResource> predicate;
>+        nsresult rv = gRDFService->GetUnicodeResource(sortkey, getter_AddRefs(predicate));
>+        if (NS_FAILED(rv))
>+            return rv;
>+
>+        // create a predicate with '?sort=true' appended. This special
>+        // predicate may be used to have a different sort value than the
>+        // displayed value
>+        sortkey.AppendLiteral("?sort=true");
>+
>+        nsCOMPtr<nsIRDFResource> sortPredicate;
>+        rv = gRDFService->GetUnicodeResource(sortkey, getter_AddRefs(sortPredicate));
>+        if (NS_FAILED(rv))
>+            return rv;
>+
>+        rv = GetSortValue(aLeft, predicate, sortPredicate, getter_AddRefs(leftNode));
>+        if (NS_FAILED(rv))
>+            return rv;
> 
>-    nsCOMPtr<nsISupports> rightNode;
>-    aRight->GetBindingObjectFor(aVar, getter_AddRefs(rightNode));
>+        rv = GetSortValue(aRight, predicate, sortPredicate, getter_AddRefs(rightNode));
>+        if (NS_FAILED(rv))
>+            return rv;
>+    }
>+    else {
>+        // get the values for the sort key from the results
>+        aLeft->GetBindingObjectFor(aVar, getter_AddRefs(leftNode));
>+        aRight->GetBindingObjectFor(aVar, getter_AddRefs(rightNode));
>+    }

so, it will be possible to use sortResource="?myvar" with content builders too?

r=jan with those nits fixed
Comment 8 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-07-11 16:28:02 PDT
Comment on attachment 223368 [details] [diff] [review]
Improve sort service code, as described

>Index: layout/xul/base/src/tree/src/nsTreeContentView.cpp

> nsTreeContentView::CycleHeader(nsITreeColumn* aCol)
...
>+        nsAutoString sortdirection;
>+        column->GetAttr(kNameSpaceID_None, nsXULAtoms::sortDirection, sortdirection);
>+
>+        if (sortdirection.EqualsLiteral("ascending"))
>+          sortdirection.AssignLiteral("descending");
>+        else
>+          sortdirection.AssignLiteral("ascending");

This seems reversed? In any case you can use
column->AttrValueIs(kNameSpaceID_None, nsXULAtoms::sortDirection, nsXULAtoms::sortDirection,
                    nsXULAtoms::ascending, eCaseMatters);
here.


>Index: content/xul/templates/public/nsIXULTemplateResult.idl

>-[scriptable, uuid(d035f34f-d8ee-444a-bd46-41163f54ed25)]
>+[scriptable, uuid(a978492e-adfc-a059-8496c546dc16)]

This uuid is too short


>Index: content/xul/templates/src/nsXULContentBuilder.cpp

>@@ -135,32 +130,34 @@ class nsXULContentBuilder : public nsXUL
...
>     nsXULContentBuilder();
>-    virtual ~nsXULContentBuilder();
>-    nsresult InitGlobals();
>+    virtual ~nsXULContentBuilder() {};

Just use the default dtor.


>@@ -1759,16 +1772,23 @@ nsXULContentBuilder::AttributeChanged(ns
>         // We're on a XUL tag, and an ``open'' attribute changed.
>         if (aContent->AttrValueIs(kNameSpaceID_None, nsXULAtoms::open,
>                                   nsXULAtoms::_true, eCaseMatters))
>             OpenContainer(aContent);
>         else
>             CloseContainer(aContent);
>     }
> 
>+    if ((aContent->GetNameSpaceID() == kNameSpaceID_XUL) &&
>+        ((aAttribute == nsXULAtoms::sort) ||
>+         (aAttribute == nsXULAtoms::sortDirection) ||
>+         (aAttribute == nsXULAtoms::sortResource) ||
>+         (aAttribute == nsXULAtoms::sortResource2)))
>+        mSortState.initialized = PR_FALSE;
>+

Check the namespace of the attribute.

>+nsXULContentBuilder::CompareResultToNode(nsIXULTemplateResult* aResult,
>+                                         nsIContent* aContent,
>+                                         PRInt32* aSortOrder)
>+{
>+    NS_ENSURE_ARG_POINTER(aSortOrder);

You shouldn't really need to check that an out-param-pointer is non-null. Just asserting (if that) is fine.


>+    if (mSortState.direction == nsSortState_natural && mQueryProcessor) {
>+        // sort in natural order
>+        nsresult rv = mQueryProcessor->CompareResults(aResult, match->mResult,
>+                                                      nsnull, aSortOrder);
>+        if (NS_FAILED(rv))
>+            return rv;

NS_ENSURE_SUCCESS(rv, rv). Same thing appears elsewhere in the patch


>+nsXULContentBuilder::InsertSortedNode(nsIContent* aContainer,
...
>+            // compute the "static" XUL element count
>+            nsAutoString valueStr;
>+            for (PRUint32 childLoop = 0; childLoop < numChildren; ++childLoop) {
>+                child = aContainer->GetChildAt(childLoop);
>+                if (!child)
>+                    break;

You don't really need to null-check here since nothing you're doing in the loop should be able to cause the child-list to be changed.

>+                child->GetAttr(kNameSpaceID_None, nsXULAtoms::_template, valueStr);
>+                if (!valueStr.IsEmpty())
>+                    break;

You can use nsContentUtils::HasNonEmptyAttr

>+            // save the "static" XUL element count hint
>+            valueStr.Truncate();
>+            valueStr.AppendInt(staticCount);
>+            aContainer->SetAttr(kNameSpaceID_None, nsXULAtoms::staticHint, valueStr, PR_FALSE);

Should this really be set as an attribute in the DOM? You might want to consider using nsIContent::SetProperty to avoid mucking around with the users DOM unneccesarily.


>+                rv = CompareResultToNode(aResult, temp, &direction);
>+                if (((x == left) && (direction < 0)) || (((x == right)) &&
>+                    (direction >= 0)) || (left == right))

Whoa! Way too many parenthesis here.

(x == left && direction < 0) || (x == right && direction >= 0) || left == right

would be enough. (you actually don't even need those parens, but people tend to not like relying on that && binds harder than ||. Bah, where are peoples boolean algebra skills)


>+                {
>+                    PRInt32 thePos = ((direction > 0) ? x : x - 1);
>+                    aContainer->InsertChildAt(aNode, thePos, aNotify);
>+                    childAdded = PR_TRUE;
>+
>+                    mSortState.lastWasFirst = (thePos == staticCount) ? PR_TRUE : PR_FALSE;
>+                    mSortState.lastWasLast = (thePos >= realNumChildren) ? PR_TRUE : PR_FALSE;

just do
    mSortState.lastWasFirst = thePos == staticCount;
    mSortState.lastWasLast = thePos >= realNumChildren;

(or keep the parens if you want)


>Index: content/xul/templates/src/nsXULSortService.cpp

What has happened to the diff of this file? I doubt you rewrote every single line of this file, including the license. Did you change the newlines maybe?

>Index: content/xul/templates/src/nsXULSortService.h

>+  nsresult
>+  GetItemsToSort(nsIContent *aContainer,
>+                         nsSortState* aSortState,
>+                         nsTArray<contentSortInfo *>* aSortItems);

Fix alignment.

It might be nicer to pass a reference to the nsTArray, rather than a pointer to it. Up to you. Same thing applies to other functions in the interface.


>Index: content/xul/templates/src/nsXULTemplateQueryProcessorRDF.cpp

>+nsXULTemplateQueryProcessorRDF::CompareResults(nsIXULTemplateResult* aLeft,
...
>+    if (!aVar) {
>+        // if a result has a negative index, just sort it first
>+        PRInt32 leftindex = GetContainerIndexOf(aLeft);
>+        PRInt32 rightindex = GetContainerIndexOf(aRight);
>+        *aResult = (leftindex == rightindex) ? 0 : ((leftindex > rightindex) ? 1 : -1);

I like to rely on the way that ?: binds for things like this, just as we always do for if-else. So

*aResult = leftindex == rightindex ? 0 :
           leftindex > rightindex ? 1 :
           -1;

Up to you.


>Index: content/xul/templates/src/nsXULTreeBuilder.cpp

>     NS_NewXULTreeBuilder(nsISupports* aOuter, REFNSIID aIID, void** aResult);
> 
>     nsXULTreeBuilder();
>-    virtual ~nsXULTreeBuilder();
>-
>-    /**
>-     * Initialize the template builder
>-     */
>-    nsresult
>-    InitGlobals();
>+    virtual ~nsXULTreeBuilder() {};

Just use the implicit default dtor instead.


Please attach a new patch since the diff for nsXULSortService.cpp was horked.
Comment 9 Neil Deakin 2006-07-12 14:23:22 PDT
Created attachment 228997 [details] [diff] [review]
Fixed review comments

> no support for 3-state (including natural) sorting?

Added support for this.

>+  readonly attribute AString type;
>+
>
> PRInt32 instead of string?

I want the values to be application specific rather than hard coded to be specific types.

> so, it will be possible to use sortResource="?myvar" with content builders too?

Yes, except the syntax will always be sort="whatever". See the linked url for details.

------- Comment #8 From Jonas Sicking 2006-07-11 16:28 PDT [reply] -------

> Should this really be set as an attribute in the DOM? You might want to
> consider using nsIContent::SetProperty to avoid mucking around with the users
> DOM unneccesarily.

I'll do that in a follow up bug since the behaviour here is the same as the old code.

>Index: content/xul/templates/src/nsXULSortService.cpp

> What has happened to the diff of this file? I doubt you rewrote every single
> line of this file, including the license. Did you change the newlines maybe?

The newlines changed here. Will fix in the next patch.
Comment 10 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-07-12 14:55:26 PDT
Comment on attachment 228997 [details] [diff] [review]
Fixed review comments

>Index: content/xul/templates/src/nsXULSortService.cpp
> XULSortServiceImpl::SetSortColumnHints(nsIContent *content,
...
>-    if (child->IsNodeOfType(nsINode::eXUL)) {
>+    if (child->IsNodeOfType(nsIContent::eXUL)) {

These constants are defined in nsINode, so please use that (though they are inherited into nsIContent, which is why the above compiles)

with that, sr=sicking
Comment 11 neil@parkwaycc.co.uk 2006-07-13 16:32:38 PDT
Quoted from the linked URL:
>The form resource?collation=true isn't used anywhere so it will be removed.
I thought all the mailnews folder menus sorted by collation.

Note You need to log in before you can comment on or make changes to this bug.