Closed Bug 330236 Opened 18 years ago Closed 18 years ago

Can't drag columns more than one past original leftmost column

Categories

(Core :: XUL, defect)

Product:
Core
Component:
XUL
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: neil, Assigned: neil)

References

Details

Attachments

(1 file, 2 obsolete files)

Corrected patch
12.69 KB, patch
enndeakin
: review+
jag+mozilla
: superreview+
mtschrep
: approval1.8.1-
Details | Diff | Splinter Review 
Steps to reproduce problem:
1. Find a tree with draggable columns
2. Drag the second column all the way to the left
3. Drag the third column all the way to the left

Expected result: 3 2 1

Actual result: 2 3 1

This is the same underlying bug as bug 330170, only in the XBL for _firstOrdinalColumn, which we can replace with columns.getFirstColumn().
Attached patch Proposed patch (obsolete) — — Splinter Review 
Also simplifies by iterating over columns instead of boxObjects.
Assignee: Jan.Varga → neil
Status: NEW → ASSIGNED
Attachment #214827 - Flags: superreview?(jag)
Attachment #214827 - Flags: review?(Jan.Varga)

        Attachment #214828 -
        Flags: review?(Jan.Varga)

    
    

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

>Index: tree.xml
>===================================================================
>RCS file: /cvsroot/mozilla/xpfe/global/resources/content/bindings/tree.xml,v
>retrieving revision 1.49
>diff -u -1 -0 -r1.49 tree.xml
>--- tree.xml	9 Oct 2005 23:42:28 -0000	1.49
>+++ tree.xml	12 Mar 2006 15:42:52 -0000

>       <method name="_ensureColumnOrder">
>         <body><![CDATA[
>           if (this._columnsDirty) {
>             // update the ordinal position of each column to assure that it is
>             // an odd number and 2 positions above it's next sibling
>             var cols = [];
>             var i;
>+            for (i = 0; i < this.columns.count; ++i)
>+              cols.push(this.columns[i].element);
>             for (i = 0; i < cols.length; ++i)
>               cols[i].setAttribute("ordinal", (i*2)+1);

You can skip a step here :-)

>@@ -142,76 +124,68 @@
>       <method name="_reorderColumn">
...

>           if (aColBefore.ordinal < aColMove.ordinal) {
>+            col = this.columns.getColumnFor(aColBefore);
>+            while (col.element != aColMove) {
>+              cols.push(col.element);
>+              col = col.getNext();
>             }
> 
>             aColMove.ordinal = aColBefore.ordinal;
>             var i;
>             for (i = 0; i < cols.length; ++i)
>               cols[i].ordinal += 2;

I think these two loops can be merged too, like:

aColMove.ordinal = aColBefore.ordinal;
col = this.columns.getColumnFor(aColBefore);
for (; col.element != aColMove; col = col.getNext())
  col.element.ordinal += 2;

>           } else {
>+            col = this.columns.getColumnFor(aColMove);
>+            while (!aBefore || col && col.element != aColBefore) {

This test doesn't seem right. If |aBefore| is false the loop won't end. I think you want:

  while (col && (col.element != aColBefore || !aBefore)) {

>+              cols.push(col.element);
>+              col = col.getNext();
>             }
> 
>             aColMove.ordinal = aBefore ? aColBefore.ordinal-2 : aColBefore.ordinal;
> 
>             for (i = 0; i < cols.length; ++i)
>               cols[i].ordinal -= 2;

And these two loops can be merged too, like:

aColMove.ordinal = aBefore ? aColBefore.ordinal - 2 : aColBefore.ordinal;
col = this.columns.getColumnFor(aColMove);
for (; col && (col.element != aColBefore || !aBefore); col = col.getNext())
  col.element.ordinal -= 2;

        Attachment #214827 -
        Flags: superreview?(jag) → superreview-

    
    

    
  
As Neil pointed out, changing the ordinal value updates the column order, so we have to do two loops. *sigh*

    
    

    
  
More precisely, the cached column order in nsTreeColumns that I was counting on not getting updated until explicitly invalidated.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Corrected patch
        — 
          — Splinter Review
      

    


  
Tested using the right tree this time :-[

        Attachment #214827 -
        Attachment is obsolete: true

        Attachment #214828 -
        Attachment is obsolete: true

        Attachment #214840 -
        Flags: superreview?(jag)

        Attachment #214840 -
        Flags: review?(Jan.Varga)

        Attachment #214827 -
        Flags: review?(Jan.Varga)

        Attachment #214828 -
        Flags: review?(Jan.Varga)

    
    

    
  
Comment on attachment 214840 [details] [diff] [review]
Corrected patch

sr=jag, though when |aColBefore| is before |aColMove| |aBefore| is always true, so you could merge that one into the loop.

        Attachment #214840 -
        Flags: superreview?(jag) → superreview+

    
    

    
  
(In reply to comment #7)
>(From update of attachment 214840 [details] [diff] [review])
>when |aColBefore| is before |aColMove| |aBefore| is always true,
>so you could merge that one into the loop.
Yeah, but I wanted to make the function more generic, just in case.

    
    

    
  
Are the problems mentioned in Bug 338426 based on the same problem as here?

    
  

        Attachment #214840 -
        Flags: review?(Jan.Varga) → review?(enndeakin)

    
  

        Attachment #214840 -
        Flags: review?(enndeakin) → review+

    
    

    
  
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED

    
  

        Attachment #214840 -
        Flags: approval-branch-1.8.1?(enndeakin)

    
    

    
  
Using Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060519 Mozilla Sunbird/0.3a2+ (Build ID: 2006051923) it is still not possible to perform the steps from bug description. (The issues from Bug 338426 still exist too.) Doing this I get errors like:

Error: col has no properties
Source File: chrome://global/content/bindings/tree.xml Line: 153

Error: cols[0] has no properties
Source File: chrome://global/content/bindings/tree.xml Line: 157

    
    

    
  
(In reply to comment #11)
>it is still not possible to perform the steps from bug description.
Sorry, that was caused by bug 330170 which I have just fixed.

    
  
Blocks: 342072

    
    

    
  
I'd recommend against approval-1.8.1 until bug 342072 is resolved.

        Attachment #214840 -
        Flags: approval-branch-1.8.1?(enndeakin) → approval1.8.1?

    
    

    
  
Comment on attachment 214840 [details] [diff] [review]
Corrected patch

Given the last comment minusing the patch - once it is resolved please re-nominate

        Attachment #214840 -
        Flags: approval1.8.1? → approval1.8.1-

    
    

    
  
Considering the severity of the regression caused by the checkin for this bug,
I'd say we need better reviewing and/or testing of "fixes" to this code.

    
  
Component: XP Toolkit/Widgets: Trees → XUL
QA Contact: xptoolkit.trees → xptoolkit.widgets

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: