Closed
Bug 330236
Opened 19 years ago
Closed 19 years ago
Can't drag columns more than one past original leftmost column
Categories
(Core :: XUL, defect)
Core
XUL
Tracking
()
RESOLVED
FIXED
People
(Reporter: neil, Assigned: neil)
References
Details
Attachments
(1 file, 2 obsolete files)
|
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().
|
Assignee | |
Comment 1•19 years ago
|
||
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)
|
|
Assignee | |
Comment 2•19 years ago
|
||
Attachment #214828 -
Flags: review?(Jan.Varga)
|
||
Comment 3•19 years ago
|
||
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-
|
|
||
Comment 4•19 years ago
|
||
As Neil pointed out, changing the ordinal value updates the column order, so we have to do two loops. *sigh*
|
|
||
Comment 5•19 years ago
|
||
More precisely, the cached column order in nsTreeColumns that I was counting on not getting updated until explicitly invalidated.
|
|
Assignee | |
Comment 6•19 years ago
|
||
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 7•19 years ago
|
||
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+
|
|
Assignee | |
Comment 8•19 years ago
|
||
(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.
|
||
Comment 9•19 years ago
|
||
Are the problems mentioned in Bug 338426 based on the same problem as here?
|
|
Assignee | |
Updated•19 years ago
|
Attachment #214840 -
Flags: review?(Jan.Varga) → review?(enndeakin)
|
||
Updated•19 years ago
|
Attachment #214840 -
Flags: review?(enndeakin) → review+
|
|
Assignee | |
Comment 10•19 years ago
|
||
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
|
|
Assignee | |
Updated•19 years ago
|
Attachment #214840 -
Flags: approval-branch-1.8.1?(enndeakin)
|
|
||
Comment 11•19 years ago
|
||
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
|
|
Assignee | |
Comment 12•19 years ago
|
||
(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.
|
||
Comment 13•19 years ago
|
||
I'd recommend against approval-1.8.1 until bug 342072 is resolved.
|
||
Updated•19 years ago
|
Attachment #214840 -
Flags: approval-branch-1.8.1?(enndeakin) → approval1.8.1?
|
||
Comment 14•19 years ago
|
||
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-
|
||
Comment 15•18 years ago
|
||
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.
Description
•