335122 - Clean up XUL Sort Service

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Neil Deakin]

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]

Attachment: Improve sort service code, as described

Comment 2

[Jan Varga [:janv]]

Nice cleanup, Neil
I went through the patch and I think the new sort architecture is great.

Comment 3

[Neil Deakin]

So Jan, are you offering to review this patch?

Comment 4

[Jan Varga [:janv]]

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]

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

ok, no need for a new patch for now
I'll review it tonight or tomorrow

Comment 7

[Jan Varga [:janv]]

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]

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]

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

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

[[e-mail]]

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.