Last Comment Bug 613034 - Port browser parts of |Bug 472343 - Managing multiple bookmarks in the Library is very slow| to SeaMonkey
: Port browser parts of |Bug 472343 - Managing multiple bookmarks in the Librar...
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.1b2
Assigned To: Jens Hatlak (:InvisibleSmiley)
:
:
Mentors:
Depends on:
Blocks: 576970
  Show dependency treegraph
 
Reported: 2010-11-17 14:20 PST by Jens Hatlak (:InvisibleSmiley)
Modified: 2011-01-20 10:15 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (3.14 KB, patch)
2010-12-25 02:16 PST, Jens Hatlak (:InvisibleSmiley)
no flags Details | Diff | Splinter Review
patch v2 [Checkin: comment 11] (2.93 KB, patch)
2010-12-26 11:13 PST, Jens Hatlak (:InvisibleSmiley)
neil: review+
Details | Diff | Splinter Review

Description Jens Hatlak (:InvisibleSmiley) 2010-11-17 14:20:12 PST
The Sync back-end uses batching, so we need to port the browser/ parts of this:
http://hg.mozilla.org/mozilla-central/rev/4503884294a7
Comment 1 Jens Hatlak (:InvisibleSmiley) 2010-12-25 02:16:11 PST
Created attachment 499708 [details] [diff] [review]
patch

AFAICS we have treeView.js under both places/ and history/ and both need to be patched.
Comment 2 neil@parkwaycc.co.uk 2010-12-25 05:19:40 PST
Comment on attachment 499708 [details] [diff] [review]
patch

Bah, the code doesn't obey the (badly named) interface :s

[It should be something like void setBatching(aIsBatching); ]
Comment 3 neil@parkwaycc.co.uk 2010-12-25 05:29:37 PST
Comment on attachment 499708 [details] [diff] [review]
patch

>+  batching: function PTV__batching(aToggleMode) {
[Why the __? I notice sortingChanged also gets this wrong.]

>+      if (this.selection) {
>+        this.selection.selectEventsSuppressed = true;
>+      }
>+      this._tree.beginUpdateBatch();
[I don't think you can have a tree yet not have a selection...]

>+    else if (this._inBatchMode) {
[Sigh. Why is this testing the existing mode only half of the time?]
Comment 4 neil@parkwaycc.co.uk 2010-12-25 05:31:28 PST
[See also my rants in bug 472343.]
Comment 5 Jens Hatlak (:InvisibleSmiley) 2010-12-25 13:38:36 PST
(In reply to comment #3)
> >+  batching: function PTV__batching(aToggleMode) {
> [Why the __? I notice sortingChanged also gets this wrong.]

I guess the original author didn't get that the double underscore is just for private methods that already have an underscore in their function name. I don't care much either way, so if you want me to change it, I will.

> [I don't think you can have a tree yet not have a selection...]

It happened to me more than once (but hardly reproducibly) with the MailNews folder pane that the selection was gone, so that deleting messages didn't work anymore even though there was a selection in the thread pane. I'm not sure whether there was actually no selection in the back-end, though.

> >+    else if (this._inBatchMode) {
> [Sigh. Why is this testing the existing mode only half of the time?]

Well, in the other case they probably thought it doesn't matter because there it's set to true in any case. Anyway, I'm only speculating about things I've been trying to port 1:1. No what? Wait for an answer on the base bug? That might never come...
Comment 6 Robert Kaiser 2010-12-26 03:02:21 PST
(In reply to comment #1)
> AFAICS we have treeView.js under both places/ and history/ and both need to be
> patched.

If we need it in both right now, yes. Else the plans are to switch history to use the places/ code some time (probably post-2.1, bug is filed) - though someone else than me will probably need to pick that one up.
Comment 7 Robert Kaiser 2010-12-26 03:06:52 PST
(In reply to comment #5)
> (In reply to comment #3)
> > [I don't think you can have a tree yet not have a selection...]
> 
> It happened to me more than once (but hardly reproducibly) with the MailNews
> folder pane that the selection was gone, so that deleting messages didn't work
> anymore even though there was a selection in the thread pane. I'm not sure
> whether there was actually no selection in the back-end, though.

Forgot to comment on that - from all my work with trees in Data Manager and others it looks like they changed tree code in Mozilla 2.0 in a way that the whole selection object is being destroyed if the tree is empty or something. I had errors of undefined objects cropping up in code that worked fine in earlier versions.
Comment 8 neil@parkwaycc.co.uk 2010-12-26 08:12:32 PST
(In reply to comment #5)
> (In reply to comment #3)
> > >+  batching: function PTV__batching(aToggleMode) {
> > [Why the __? I notice sortingChanged also gets this wrong.]
> I guess the original author didn't get that the double underscore is just for
> private methods that already have an underscore in their function name. I don't
> care much either way, so if you want me to change it, I will.
Actually I don't like the double underscores for private methods either.

> > [I don't think you can have a tree yet not have a selection...]
> It happened to me more than once (but hardly reproducibly) with the MailNews
> folder pane that the selection was gone, so that deleting messages didn't work
> anymore even though there was a selection in the thread pane. I'm not sure
> whether there was actually no selection in the back-end, though.
What I meant was that setTree and setSelection are always called together. See
http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp?mark=525,530#516

> > >+    else if (this._inBatchMode) {
> > [Sigh. Why is this testing the existing mode only half of the time?]
> Wait for an answer on the base bug? That might never come...
Well, there are a number of options:
1. Leave the code as is and assume the caller gets things right
2. Remove _inBatchMode and assume the caller gets things right
3. Test _inBatchMode before setting it in case the caller gets things wrong
4. Use selectEventsSuppressed otherwise as 3.

(In reply to comment #7)
> Forgot to comment on that - from all my work with trees in Data Manager and
> others it looks like they changed tree code in Mozilla 2.0 in a way that the
> whole selection object is being destroyed if the tree is empty or something.
That's strange, because the link above is the only call to SetSelection...
Comment 9 Jens Hatlak (:InvisibleSmiley) 2010-12-26 11:13:08 PST
Created attachment 499769 [details] [diff] [review]
patch v2 [Checkin: comment 11]

I'd rather not like to change the API wrt. FF, so I kept the private member variable and method names, but I hope you like the implementation better this way.
Comment 10 neil@parkwaycc.co.uk 2010-12-26 13:37:33 PST
Comment on attachment 499769 [details] [diff] [review]
patch v2 [Checkin: comment 11]

>+    this._inBatchMode = this.selection.selectEventsSuppressed = aBatching;
[I wonder whether the assignment order makes any difference. Probably not.]
Comment 11 Jens Hatlak (:InvisibleSmiley) 2010-12-27 02:51:33 PST
Comment on attachment 499769 [details] [diff] [review]
patch v2 [Checkin: comment 11]

http://hg.mozilla.org/comm-central/rev/518a897f2fc9
Comment 12 Jens Hatlak (:InvisibleSmiley) 2011-01-20 10:15:13 PST
Looks like we need to port bug 620198, too. File bug 627408 for that.

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