380230 - personal toolbar overflow chevron not updating

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[(not reading, please use [e-mail] instead)]

personal toolbar overflow chevron not updating.

for example, use the bm manager to add bms to the personal toolbar, and you don't get the chevron until you resize.

I think this is fall out from bug #337868.  because we no longer rebuild the toolbar so aggressively, we also don't update the chevron like we used to.

(note:  I've got changes to toolbar.xml and menu.xml in my local tree, I need to confirm with a clean places build)

I think we need to fire the updateChevron() method when items are removed, added (and changed, think about the scenario where a bm title gets longer or shorter).

additionally, the "if (this.lastWidth == totalWidth)" optimization may need to be reworked.

fwiw:  in my local tree I've fixed the problem by firing the updateChevron() method when the _viewer gets called on itemInserted(), itemRemoved(), etc and I have commented out the lastWidth optimization.

Comment 1

[Neil Deakin]

(In reply to comment #0)
> 
> fwiw:  in my local tree I've fixed the problem by firing the updateChevron()
> method when the _viewer gets called on itemInserted(), itemRemoved(), etc 

This is the right way to fix this. Can you make a patch?

Comment 2

[(not reading, please use [e-mail] instead)]

> This is the right way to fix this. Can you make a patch?

thanks for the response.  here comes a patch.

Comment 3

[(not reading, please use [e-mail] instead)]

Attachment: patch (sorry, includes part of the fix for bug #337855)

notice I have removed the lastWidth optimization from dietrich's fix to bug #372623.

Comment 4

[Asaf Romano (gone)]

Comment on attachment 264507 [details] [diff] [review]
patch (sorry, includes part of the fix for bug #337855)

needs update, i guess.

Comment 5

[(not reading, please use [e-mail] instead)]

Attachment: patch

1) I have removed the lastWidth optimization from dietrich's fix to bug
#372623.
2) there is no _rebuildAll()
3) _currentURIs was unused (and is not coming back for bug #337855)

Comment 6

[Bill Gianopoulos [:WG9s]]

(In reply to comment #5)

> 3) _currentURIs was unused (and is not coming back for bug #337855)
> 
Looking at what the code does, I would have thought this would be used by microsummaries.

Comment 7

[(not reading, please use [e-mail] instead)]

> Looking at what the code does, I would have thought this would be used by
> microsummaries.

bill, can you elaborate?

I think that neil added that code as part of bug #337868 ("Implement result view for toolbar for dynamic updating"), which was a port of what dietrich was working on for bug #337855 ("Bookmarks menu painfully slow with Places build", aka "Implement result view for menu for dynamic updating")

Comment 8

[Bill Gianopoulos [:WG9s]]

CVS blame says this code was added as part of bug 333052 to make microsummaries work with places builds.

Comment 9

[Ria Klaassen (not reading all bugmail)]

When I click on a folder in the bookmarks toolbar and then hover over the overflow arrow it doesn't expand anymore like it used to. Is this a regression or just a change?

Comment 10

[(not reading, please use [e-mail] instead)]

ria, that's a regression from fx2 behavior, and I'm able to reproduce it.  I've logged bug #381878 on this issue.

Comment 11

[(not reading, please use [e-mail] instead)]

Comment on attachment 265519 [details] [diff] [review]
patch

would gladly take a review from either mano or dietrich on this patch.

Comment 12

[Dietrich Ayala (:dietrich)]

Comment on attachment 265519 [details] [diff] [review]
patch

r=me, thanks.

bill: currentURIs is left over from the old uri-based implementation. the new bookmark id microsummary support doesn't use it.

seth, i'm seeing an about an inch of space between the chevron and the end of the toolbar (and which was there before applying this patch); do you know if there's a bug open for that already?

Comment 13

[Phil Ringnalda (:philor)]

(In reply to comment #12)
> i'm seeing an about an inch of space between the chevron and the end

Your last non-overflowing bookmark must have a fairly short title, and the first overflowing one a fairly long title, then. It's actually just left-aligned rather than right-. Bug 331307.

Comment 14

[(not reading, please use [e-mail] instead)]

fixed.

Checking in browser/components/places/content/toolbar.xml;
/cvsroot/mozilla/browser/components/places/content/toolbar.xml,v  <--  toolbar.x
ml
new revision: 1.86; previous revision: 1.85
done

Comment 15

[Tracy Walker [:tracy]]

verified with Win XP trunk build from 06/07

Comment 16

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv