Last Comment Bug 517726 - Update SeaMonkey's treeView.js to match new view API
: Update SeaMonkey's treeView.js to match new view API
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: General (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: seamonkey2.1a1
Assigned To: neil@parkwaycc.co.uk
:
:
Mentors:
Depends on: 498130
Blocks: 531696
  Show dependency treegraph
 
Reported: 2009-09-19 14:36 PDT by Mano (::mano, needinfo? for any questions; not reading general bugmail)
Modified: 2009-12-08 15:52 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Possible patch (881 bytes, patch)
2009-11-26 06:59 PST, neil@parkwaycc.co.uk
iann_bugzilla: review+
Details | Diff | Splinter Review
Update observing methods (15.24 KB, patch)
2009-12-02 09:22 PST, neil@parkwaycc.co.uk
iann_bugzilla: review+
Details | Diff | Splinter Review

Description Mano (::mano, needinfo? for any questions; not reading general bugmail) 2009-09-19 14:36:00 PDT
In bug 498130, we're changing the nsINavHistoryResultView and other places APIs. The new API isn't comapatible with he places tree view implementation of SM. I'm pretty sure that at this point you can just copy over the FX implementaion (we've removed showSessions too).

The API showed at the last patch there is very likely final. Please let me know if you want to correspond patch-landings.
Comment 1 Robert Kaiser 2009-09-19 14:46:04 PDT
Thanks for the note about it - I just so much would hope we'd have branched already, then doing this for trunk only would be fine, but due to Thunderbird folks blocking that branching, we need to keep things working with at least both 1.9.1 and trunk for now, and that complicates this. :(

Mark, here's a bug that we should take into account with branching plans, even if it's SeaMonkey-only.
Comment 2 neil@parkwaycc.co.uk 2009-09-19 15:22:38 PDT
(In reply to comment #0)
> I'm pretty sure that at this point you can just copy over the FX
> implementaion (we've removed showSessions too).
We didn't before, so we may want to merge changes instead.
Comment 3 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2009-09-26 02:52:42 PDT
It's now checked in.
Comment 4 neil@parkwaycc.co.uk 2009-11-26 06:59:59 PST
Created attachment 414728 [details] [diff] [review]
Possible patch

This is the minimum necessary to fix the bug. Places used to define a viewIndex property on their nodes which defaulted to -1; we now have to set that on the root manually but fortunately their nodes have classInfo which means that we can set arbitrary properties.
As a followup we could change our code to match their code more in two ways:
1. Rename viewIndex to _viewIndex
2. Move .properties to .node._cellProperties and eliminate the holder object
Comment 5 Ian Neal 2009-11-29 15:25:53 PST
Comment on attachment 414728 [details] [diff] [review]
Possible patch

r=me, could you file the follow up bug(s) please?
Comment 6 neil@parkwaycc.co.uk 2009-11-29 15:43:22 PST
Pushed changeset 5d3acfe7d0d7 to comm-central.
Comment 7 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2009-12-02 02:15:25 PST
You didn't update the observing methods afiact (item_Title_Changed etc.)).
Comment 8 neil@parkwaycc.co.uk 2009-12-02 08:47:14 PST
(In reply to comment #7)
> You didn't update the observing methods afiact (item_Title_Changed etc.)).
Could you be more specific? Neither item_Title_Changed nor itemTitleChanged show up in an MXR of mozilla-1.9.1 and there is one hit in mozilla-central.
Comment 9 neil@parkwaycc.co.uk 2009-12-02 08:50:06 PST
Oh, I see now, it used to be called itemChanged but now it's nodeXXXChanged.
Comment 10 neil@parkwaycc.co.uk 2009-12-02 09:22:32 PST
Created attachment 415655 [details] [diff] [review]
Update observing methods

* Renamed itemChanged to invalidateNode
* Added nodeXXXChanged methods
* Renamed [Ii]tem to [Nn]ode almost everywhere
Comment 11 Ian Neal 2009-12-08 15:33:06 PST
Comment on attachment 415655 [details] [diff] [review]
Update observing methods

>+   * This counts how many rows an node takes in the tree, that is, the
Nit: "a" rather than "an".

>+   * node itself plus any nodes following it with an increased indent.
>+   * This allows you to figure out how many rows an node (=1) or a
Nit: "a" rather than "an".

>     if (oldViewIndex > this._visibleElements.length)
>-      throw("Trying to remove an item with an invalid viewIndex");
>+      throw("Trying to remove an node with an invalid viewIndex");
Nit: "a" rather than "an".
Comment 12 neil@parkwaycc.co.uk 2009-12-08 15:52:18 PST
Pushed changeset 4b63c4badb42 to comm-central.

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