Port browser parts of |Bug 472343 - Managing multiple bookmarks in the Library is very slow| to SeaMonkey

RESOLVED FIXED in seamonkey2.1b2

Status

SeaMonkey
Bookmarks & History
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: InvisibleSmiley, Assigned: InvisibleSmiley)

Tracking

Trunk
seamonkey2.1b2

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

7 years ago
The Sync back-end uses batching, so we need to port the browser/ parts of this:
http://hg.mozilla.org/mozilla-central/rev/4503884294a7
(Assignee)

Comment 1

6 years ago
Created attachment 499708 [details] [diff] [review]
patch

AFAICS we have treeView.js under both places/ and history/ and both need to be patched.
Assignee: nobody → jh
Status: NEW → ASSIGNED
Attachment #499708 - Flags: review?(neil)

Comment 2

6 years ago
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

6 years ago
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

6 years ago
[See also my rants in bug 472343.]
(Assignee)

Comment 5

6 years ago
(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

6 years ago
(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

6 years ago
(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

6 years ago
(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...
(Assignee)

Comment 9

6 years ago
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.
Attachment #499708 - Attachment is obsolete: true
Attachment #499769 - Flags: review?(neil)
Attachment #499708 - Flags: review?(neil)
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.]
Attachment #499769 - Flags: review?(neil) → review+
(Assignee)

Comment 11

6 years ago
Comment on attachment 499769 [details] [diff] [review]
patch v2 [Checkin: comment 11]

http://hg.mozilla.org/comm-central/rev/518a897f2fc9
Attachment #499769 - Attachment description: patch v2 → patch v2 [Checkin: comment 11]
(Assignee)

Updated

6 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1b2
(Assignee)

Comment 12

6 years ago
Looks like we need to port bug 620198, too. File bug 627408 for that.
You need to log in before you can comment on or make changes to this bug.