382639 - bookmark toolbar change not reflected in UI

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Dietrich Ayala (:dietrich)]

STR:
- open the bookmarks organizer
- change the bookmark toolbar folder to the root bookmarks folder
- select the old bookmark toolbar folder, and open the "edit" menu

Expected result: the "set as bookmarks toolbar" item should be enabled

Actual: it's disabled

Comment 1

[Dietrich Ayala (:dietrich)]

http://lxr.mozilla.org/mozilla/source/browser/components/places/content/utils.js#1318

PlacesUtils uses cached root ids.

Comment 2

[Dietrich Ayala (:dietrich)]

Attachment: don't cache roots

so, this fixes part of it, but there's still an odd ui bit not working:

if i select a folder, set it as btf, and then with that folder still selected immediately open the edit menu again, the "set as btf" menu item is still enabled. it should be disabled.

if i select a different folder, and then re-select the new btf, and open "edit", the menu is disabled as it should be.

Comment 3

[Steve Won (stevewon on irc)]

So, all the values seem to get set right (such as the new toolbar folder id). But it seems like the menu under Edit is only rebuilt everytime a different bookmark or a bookmark folder is clicked (but I might be wrong). Kind of stuck... any suggestions would be extremely helpful. Thanks!

Comment 4

[Asaf Romano (gone)]

As a hack you could call goUpdate("whatever_the_command_name_is"); when setting the toolbar folder. This issue (places commands only updated after "select" event) also breaks undo/redo state though wrt. to all commands so I would rather fix that separately.

Also, the bookmarks & places ids should still be cached.

Comment 5

[Steve Won (stevewon on irc)]

mm... I can't seem to reproduce this part of the fix (even though I should make fix it so that bookmarks & places ids should be cached). Was there some change?

(In reply to comment #2)
> Created an attachment (id=266779) [details]
> don't cache roots
> 
> so, this fixes part of it, but there's still an odd ui bit not working:
> 
> if i select a folder, set it as btf, and then with that folder still selected
> immediately open the edit menu again, the "set as btf" menu item is still
> enabled. it should be disabled.
> 
> if i select a different folder, and then re-select the new btf, and open
> "edit", the menu is disabled as it should be.
> 

Comment 6

[Steve Won (stevewon on irc)]

nvmd. got it.
(In reply to comment #5)
> mm... I can't seem to reproduce this part of the fix (even though I should make
> fix it so that bookmarks & places ids should be cached). Was there some change?
> 
> (In reply to comment #2)
> > Created an attachment (id=266779) [details] [details]
> > don't cache roots
> > 
> > so, this fixes part of it, but there's still an odd ui bit not working:
> > 
> > if i select a folder, set it as btf, and then with that folder still selected
> > immediately open the edit menu again, the "set as btf" menu item is still
> > enabled. it should be disabled.
> > 
> > if i select a different folder, and then re-select the new btf, and open
> > "edit", the menu is disabled as it should be.
> > 
> 

Comment 7

[Steve Won (stevewon on irc)]

Attachment: Proposing a patch

Here's the proposed patch. Only concern I have has to do with undo/redo.
Currently, I set the oldFolderId when the toolbar folder is changed and calling undo after changing the toolbar folder seems to do everything correctly. But not sure if there are any cases where this wouldn't work.

Comment 8

[Dietrich Ayala (:dietrich)]

> This issue (places commands only updated after "select"
> event) also breaks undo/redo state though wrt. to all commands so I would
> rather fix that separately.

Mano, i don't understand the issue here. is there a bug for what you're referring to?

> 
> Also, the bookmarks & places ids should still be cached.
> 

in favor of adding a setter for the toolbar to PlacesUtils, and updating the value from the transaction?

Comment 9

[Asaf Romano (gone)]

(In reply to comment #8)
> > This issue (places commands only updated after "select"
> > event) also breaks undo/redo state though wrt. to all commands so I would
> > rather fix that separately.
> 
> Mano, i don't understand the issue here. is there a bug for what you're
> referring to?
> 

Neil filed a bug on this at some point, basically command-updating is only triggered when a "select" event fired (on the tree) while not all commands trigger selection.

> > 
> > Also, the bookmarks & places ids should still be cached.
> > 
> 
> in favor of adding a setter for the toolbar to PlacesUtils, and updating the
> value from the transaction?
> 

Er? the bookmarks & places roots are read-only.

Comment 10

[Dietrich Ayala (:dietrich)]

> > > 
> > > Also, the bookmarks & places ids should still be cached.
> > > 
> > 
> > in favor of adding a setter for the toolbar to PlacesUtils, and updating the
> > value from the transaction?
> > 
> 
> Er? the bookmarks & places roots are read-only.
> 

er, sorry i was thinking about how to avoid getting the toolbar direct from the bookmarks service every time. doing as i described would allow us to continue caching it. but yes, the other roots should still be cached.

Comment 11

[Dietrich Ayala (:dietrich)]

Comment on attachment 267992 [details] [diff] [review]
Proposing a patch


>Index: browser/components/places/content/controller.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/content/controller.js,v
>retrieving revision 1.158
>diff -u -8 -p -r1.158 controller.js
>--- browser/components/places/content/controller.js	1 Jun 2007 00:44:43 -0000	1.158
>+++ browser/components/places/content/controller.js	11 Jun 2007 18:30:27 -0000
>@@ -2246,17 +2246,19 @@ function PlacesSetBookmarksToolbarTransa
>   this._folderId = aFolderId;
>   this._oldFolderId = this.utils.toolbarFolder;
>   this.redoTransaction = this.doTransaction;
> }
> PlacesSetBookmarksToolbarTransaction.prototype = {
>   __proto__: PlacesBaseTransaction.prototype,
>   
>   doTransaction: function PSBTT_doTransaction() {
>+    this._oldFolderId = this.utils.bookmarks.toolbarFolder;

now that utils.toolbarFolder is not cached, there's no need for this line anymore. in any case, you'd want to fix the initial setting of _oldFolderId instead of doing this.

>     this.utils.bookmarks.toolbarFolder = this._folderId;
>+    goUpdateCommand("placesCmd_setAsBookmarksToolbarFolder");
>   },
> 
>   undoTransaction: function PSBTT_undoTransaction() {
>     this.utils.bookmarks.toolbarFolder = this._oldFolderId;
>   }
> };

does undoTransaction also need to update the command?

>   // identifier getters for special folders
>   get placesRootId() {
>-    if (!("_placesRootId" in this))
>-      this._placesRootId = this.bookmarks.placesRoot;
>-
>+    this._placesRootId = this.bookmarks.placesRoot;
>     return this._placesRootId;
>   },
> 
>   get bookmarksRootId() {
>-    if (!("_bookmarksRootId" in this))
>-      this._bookmarksRootId = this.bookmarks.bookmarksRoot;
>-
>+    this._bookmarksRootId = this.bookmarks.bookmarksRoot;
>     return this._bookmarksRootId;
>   },

per mano's comment, these can be cached, so no need for these changes.

Comment 12

[Steve Won (stevewon on irc)]

Waiting until bug#376004 is finished.

Comment 13

[Steve Won (stevewon on irc)]

Attachment: Patch

> As a hack you could call goUpdate("whatever_the_command_name_is"); when setting
> the toolbar folder. This issue (places commands only updated after "select"
> event) also breaks undo/redo state though wrt. to all commands so I would
> rather fix that separately.

In response to this comment, I removed the goUpdateCommand from the last patch so it can be dealt separately.

Comment 14

[Steve Won (stevewon on irc)]

Attachment: Patch. toolbarFolderId not cached anymore.

toolbarFolderId isn't cached anymore.

Comment 15

[Dietrich Ayala (:dietrich)]

Checking in browser/components/places/content/utils.js;
/cvsroot/mozilla/browser/components/places/content/utils.js,v  <--  utils.js
new revision: 1.58; previous revision: 1.57
done

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