234480 - Bookmarks Manager gets object count (in status bar) wrong

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, P4)

Description

[Jeremy M. Dolan]

When you highlight a folder in Bookmarks Manager, the status bar apperently
tries to show you a count of the objects inside it. It seems to get it wrong for
folders that have subfolders though. In fact, I can't at all figure out how it
gets the number it gets.

It says "111 object(s)" for my top level "Bookmarks" folder. There are less than
that in the root, and definately more than that in the root+1 level down, so I
have no idea how that number is derived.

If the maintainer of that code can't figure it out, I can try and find a
simplified test case. Let me know.

(Also, "object(s)" is lazy programming... someone should probably add a "num !=
1" check.)

Comment 1

[Jeremy M. Dolan]

Simply rearanging the order of the folders in the top level changed the count
from 111 to 117. Figure THAT out.

Comment 2

[Jeremy M. Dolan]

Reproducable: Swapping one folder with the folder below it increases the object
count by one. Swapping them back increases it one more. It's as if it's a
revision counter. Moving two folders at once increases count by two.

Comment 3

[Asa Dotzler [:asa]]

I recently noticed this. Can't be too difficult to fix. Requesting that we get
this on the "polish it up" train for 1.0

Comment 4

[Ryan Polk (Quark)]

First off, it's not recursive.  So any entries within subfolders are not counted. 

It seems that the count is stored some global variable, because closing and
reopening Firefox will set any folder back to the right count.

This variable seems to be updated when anything is ADDED to the folder
(count++), but not when anything is REMOVED from a folder(no count--).  So
rearranging anything in the folder amounts to an 'add' and a 'remove': the total
count goes up, even though the same number of objects is there.

the offending code in bookmarksManager.js:
    if (selection.isContainer[0]) {// && protocol != "find" && protocol != "file") {
      RDFC.Init(aEvent.target.db, selection.item[0]);
      var count = RDFC.GetCount();
      displayValue = BookmarksUtils.getLocaleString("status_foldercount",
String(count));

I haven't been able to follow RDFC anymore because I don't understand how
bookmarks.js is creating it (and there's MANY GetCount functions), but that's a
head start and I'm outta time for now :P

Comment 5

[Asa Dotzler [:asa]]

*** Bug 238729 has been marked as a duplicate of this bug. ***

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #4)
> I haven't been able to follow RDFC anymore because I don't understand how
> bookmarks.js is creating it (and there's MANY GetCount functions), but that's a
> head start and I'm outta time for now :P

Heh, GetCount strikes again.  RDFC is just a wrapper around a RDF container; it
gets used all over the place (hence the .Init) to change which container it
points to.  GetCount is a misnomer; the RDF container utils uses a property
called "nextVal" to keep track of where the next item should be added for RDF
containers.  GetCount doesn't actually count, but just returns this value.  As
RDF container item numbers can have gaps, this means you get the problems when
just moving bookmarks around.

So, question: is this "# object(s)" business important enough to do a mildly
expensive count each time the user clicks on a folder?  What if it were to just
go away?

Comment 7

[Jeremy M. Dolan]

> So, question: is this "# object(s)" business important enough to do a mildly
> expensive count each time the user clicks on a folder?

I think the real question is: is it important enough to have a whole text bar at
the bottom of the bookmarks manager just to display it. The bar doesn't seem to
do anything else. I'd much rather have more room to display bookmarks, than a
slow or inaccurate count.

Slow object counts like that seem to be typically initiated by: 1) select
objects 2) right click, properties, or similar 3) dialog window presents stats.

I don't think counting bookmark needs all of that UI.

Comment 8

[Ryan Polk (Quark)]

Jeremy: the status bar is also used to display the URL of a selected bookmark.

Comment 9

[Jeremy M. Dolan]

(In reply to comment #8)
> Jeremy: the status bar is also used to display the URL of a selected bookmark.

We also show that in the tooltip, and by default it's one of the columns in the
password manager itself. Seems reasonable to me not to have it also in the
bottom-left corner.

Actually, scratch that. It's only a tooltip when you're hovering over the
"Location" column. Perhaps the tooltips for the bookmark managers entries should
look like those for bookmarks toolbar entries? (two lines, full name and location)

Comment 10

[Asa Dotzler [:asa]]

This isn't terribly useful and if it can't be made fast and accurate, then we
should just pull it.

Comment 11

[Blake Ross]

+'ing since this is basically broken UI, let's do one or the other here (fix it
or remove it). There's precedence for keeping it (Windows Explorer, which we're
basically emulating with this window) and I think it's a moderately useful
figure. However, if we can't make it work quickly enough to appear instantly, or
if it's significant work to make it accurately, let's ditch it for 1.0.

Comment 12

[Ryan Polk (Quark)]

Attachment: possible fix

Submitted more for thought than as a final patch.  It was simple to write,
based on other bookmarks code.	The realtime search shouldn't be an issue. 
Questions that need to be answered:
1)Is this better to fix or to get rid of?
2)Should this be recursive?
3)What needs to, or doesn't need to, be counted.  Currently everything but
BookmarkSeparator is counted with this.

Comment 13

[Ryan Polk (Quark)]

Comment on attachment 155529 [details] [diff] [review]
possible fix

Will this do as a search?  My worry code-wise is whether we need to be
exclusive or inclusive with the if() and making sure it's complete.

Comment 14

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 155529 [details] [diff] [review]
possible fix

r=vladimir

Looks fine to me; it might be better to ask the treeBoxObject what the child
count is at the releavant node, but I think that'll only work if the item is
actually open.

Comment 15

[Asa Dotzler [:asa]]

Comment on attachment 155529 [details] [diff] [review]
possible fix

a=asa for branch checkin.

Comment 16

[Petey]

With the solution in RC 1 (final proposed solution?) there is a bit of a hitch.
 If a folder is selected in the tree view and then an item in the file view
selected, refocusing the tree view will not re-calculate the object count.
Example: Click root "Bookmarks" folder in tree view.
Click an object in file view.
Click root "Bookmarks" folder in tree view again.
Note the status bar.

I think it would be additionally useful to be able to click on blank space in
the file view to deselect all items in that view and re-calculate the object
count for the parent folder.

Comment 17

[Steffen Wilberg]

Fixed on trunk by the branch landing.

Comment 18

[Mike Connor [:mconnor]]

sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs, filter on "beltznerLovesGoats" to get rid of this mass change