Closed Bug 476994 Opened 13 years ago Closed 13 years ago

issues with about:sessionrestore treeview implementation

Categories

(Firefox :: Session Restore, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 3.6a1

People

(Reporter: Gavin, Assigned: zeniko)

Details

(Keywords: verified1.9.1)

Attachments

(1 file, 1 obsolete file)

See bug 459550 comment 19 and bug 459550 comment 22.

Quoting Neil:

>+  hasNextSibling: function(idx, after) {
>+    var thisLevel = this.getLevel(idx);
>+    for (var t = idx + 1; t < gTreeData.length && this.getLevel(t) > thisLevel; t++);
>+    return thisLevel == this.getLevel(t);
There is an error in this calculation; it always returns true for the last
window and its last tab. I think this code should work:
for (var t = idx + 1; t < gTreeData.length; t++)
  if (this.getLevel(t) <= thisLevel)
    return this.getLevel(t) == thisLevel;
return false;

>+    item.open = !item.open;
This needs an extra this.treeBox.invalidateRow(idx); to work properly.
For reference, our treeview implementation was modeled after the sample code from <https://developer.mozilla.org/en/XUL_Tutorial/Tree_View_Details> which also contains both of these issues (although in the first one it returns |undefined| instead of |true| for the last children which looks like working correctly).
Attached patch fix per comment #0 (obsolete) — Splinter Review
Attachment #361140 - Flags: review?(dietrich)
(In reply to comment #0)
> for (var t = idx + 1; t < gTreeData.length; t++)
>   if (this.getLevel(t) <= thisLevel)
>     return this.getLevel(t) == thisLevel;
I've just spotted that you should start the search at after + 1 rather than idx + 1 as it is known that the idx element is an ancestor of after and therefore does not have a sibling before it. I'll update devmo in a sec.
Attached patch like so?Splinter Review
Assignee: nobody → zeniko
Attachment #361140 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #361181 - Flags: review?(neil)
Attachment #361140 - Flags: review?(dietrich)
Comment on attachment 361181 [details] [diff] [review]
like so?

r=me by code inspection only.
Attachment #361181 - Flags: review?(neil) → review+
Keywords: checkin-needed
Comment on attachment 361181 [details] [diff] [review]
like so?

Odd:
I get
{
UnicodeEncodeError: 'ascii' codec can't encode character u'\xfc' in position 13: ordinal not in range(128)
}
when I try to "qimportbz.py" this patch :-(
Then I prefer not to proceed.
(In reply to comment #6)
> UnicodeEncodeError: 'ascii' codec can't encode character u'\xfc'

That's a bug in qimportbz.py: The patch attacher's name isn't correctly encoded which isn't an issue with the patch itself (it applies cleanly if done manually) but just with the additional data it fetches from this bug's data (my surname).
Rob: qimportbz.py fails when the contributor's name contains funny characters (anything beyond \x7F) and probably also when the bug's description does (see comment #6). I guess you'll have to encode them as UTF-8 before writing them to disk...
(In reply to comment #8)

Oh well, that's exactly what I found out (working on bug 249141 comment 32) :->
I emailed a patch to rob for review...
http://hg.mozilla.org/mozilla-central/rev/3368baff687b
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.2a1
Attachment #361181 - Flags: approval1.9.1?
Comment on attachment 361181 [details] [diff] [review]
like so?

Thanks for all the check-ins, Dão!
Comment on attachment 361181 [details] [diff] [review]
like so?

a191=beltzner
Attachment #361181 - Flags: approval1.9.1? → approval1.9.1+
Keywords: checkin-needed
verified FIXED on builds:

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090514 Minefield/3.6a1pre ID:20090514031229

and

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b5pre) Gecko/20090514 Shiretoko/3.5b5pre ID:20090514031203
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.