374613 - the context menu of places bookmarks with live titles is missing the "reload" option

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Dietrich Ayala (:dietrich)]

In Fx2, microsummarized bookmarks have a context menu option for reloading the title, labeled "Reload Live Title". This needs to be added to the Places context menu for those bookmarks.

Comment 1

[Steve Won (stevewon on irc)]

Attachment: Proposing a patch

Comment 2

[Asaf Romano (gone)]

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

This should be implemented as a separate command (and menu-item).

Comment 3

[Steve Won (stevewon on irc)]

Attachment: Proposed patch modified

Comment 4

[Dietrich Ayala (:dietrich)]

Comment on attachment 265966 [details] [diff] [review]
Proposed patch modified

seem to be missing the menu item in this patch.

Comment 5

[Steve Won (stevewon on irc)]

Attachment: Patch with All other files missing from last one

This should have all the files. Sorry.

Comment 6

[Dietrich Ayala (:dietrich)]

Comment on attachment 265988 [details] [diff] [review]
Patch with All other files missing from last one

the menu item in the organizer should only be enabled for microsummarized bookmarks, it's currently enabled for everything.

almost there :)

Comment 7

[Steve Won (stevewon on irc)]

Attachment: Weird

So I fixed the problem with the edit menu under the Bookmark manager you mentioned. But now something funky happens. When you first right click on a bookmark with microsummary in the toolbar, "Reload Live Title" will be grayed out. But if you right click on it for the 2nd time, "Reload Live Title" will not be grayed out. So I tried dumping out the value for "this" (mentioned in the first couple +'s in this diff file), first time it prints out "null" and the second time its a valid object.
A little confused on this one..

Comment 8

[Dietrich Ayala (:dietrich)]

(In reply to comment #7)
> Created an attachment (id=265999) [details]
> Weird
> 
> So I fixed the problem with the edit menu under the Bookmark manager you
> mentioned. But now something funky happens. When you first right click on a
> bookmark with microsummary in the toolbar, "Reload Live Title" will be grayed
> out. But if you right click on it for the 2nd time, "Reload Live Title" will
> not be grayed out. So I tried dumping out the value for "this" (mentioned in
> the first couple +'s in this diff file), first time it prints out "null" and
> the second time its a valid object.
> A little confused on this one..
> 

hm, i could not reproduce that problem. wfm on mac. what platform were you testing on?

Comment 9

[Steve Won (stevewon on irc)]

A mac here also.. does everything work on your computer?

Comment 10

[Asaf Romano (gone)]

Make sure to add all the missing spaces before landing this... ;)

Comment 11

[Asaf Romano (gone)]

(In reply to comment #8)
...
> hm, i could not reproduce that problem. wfm on mac. what platform were you
> testing on?


That's bug 381477 which is basically a cocoa widget issue.

Comment 12

[Steve Won (stevewon on irc)]

ah, it works fine when I right click. I kept ctrl+click before.
Sorry dietrich. Review please? =]

Comment 13

[Steve Won (stevewon on irc)]

Attachment: Same patch as before.

Comment 14

[Steve Won (stevewon on irc)]

Attachment: Same patch as before.

Comment 15

[Steve Won (stevewon on irc)]

I will put up another one with spaces fixed.

Comment 16

[Steve Won (stevewon on irc)]

Attachment: Patch with spacing fix

Comment 17

[Steve Won (stevewon on irc)]

Attachment: Same as the previous, but included mss into PlacesUtils

Comment 18

[Steve Won (stevewon on irc)]

Attachment: Patch

Comment 19

[Steve Won (stevewon on irc)]

Attachment: Patch

Comment 20

[Asaf Romano (gone)]

Comment on attachment 266785 [details] [diff] [review]
Patch

>Index: browser/components/places/content/controller.js
>===================================================================

>+    case "placesCmd_reloadMicrosummary":
>+      if (this._view.hasSingleSelection) {
>+        var selectedNode = this._view.selectedNode;
>+        var mss = PlacesUtils.microsummaries;
>+        if (mss.hasMicrosummary(selectedNode.itemId))

first check whether selectedNode is a bookmark node (PlacesUtils.nodeIsBookmark).

>Index: browser/components/places/content/places.xul
>===================================================================
>@@ -218,16 +218,19 @@
>           <menuitem command="placesCmd_setAsBookmarksToolbarFolder"
>                     label="&cmd.personalToolbarFolder.menuLabel;"
>                     accesskey="&cmd.personalToolbarFolder.menuAccesskey;"/>
>           <menuseparator/>
>           <menuitem id="editReload"
>                     command="placesCmd_reload"
>                     label="&cmd.reloadLivebookmark.label;"
>                     accesskey="&cmd.reloadLivebookmark.accesskey;"/>
>+          <menuitem id="editReloadMicrosummary"
>+                    command="placesCmd_reloadMicrosummary"
>+                    label="&cmd.reloadMicrosummary.label;"/>

This item used to have an accesskey in Fx2.

>Index: browser/components/places/content/placesOverlay.xul
>===================================================================
>   </commandset>
>@@ -174,16 +176,20 @@
>               selection="host|separator|link|folder|day"
>               forcehideselection="livemark/bookmarkFeedURI"/>
>     <menuseparator id="placesContext_deleteSeparator"/>
>     <menuitem id="placesContext_reload"
>               command="placesCmd_reload"
>               label="&cmd.reloadLivebookmark.label;"
>               accesskey="&cmd.reloadLivebookmark.accesskey;"
>               selection="livemark/feedURI|allLivemarks"/>
>+    <menuitem id="placesContext_reloadMicrosummary"
>+              command="placesCmd_reloadMicrosummary"
>+              label="&cmd.reloadMicrosummary.label;"

here too, probably.

Comment 21

[Steve Won (stevewon on irc)]

Attachment: Patch

Regarding the access key, "Reload Live Bookmark" and "Reload Live Bookmark" had the same access key in Fx2. So I set the access key in this patch as the access key for "Reload live Bookmark".

Comment 22

[Asaf Romano (gone)]

Comment on attachment 266801 [details] [diff] [review]
Patch

r=mano

Comment 23

[Asaf Romano (gone)]

mozilla/browser/components/places/content/controller.js 1.157
mozilla/browser/components/places/content/places.xul 1.75
mozilla/browser/components/places/content/placesOverlay.xul 1.12
mozilla/browser/components/places/content/utils.js 1.43
mozilla/browser/locales/en-US/chrome/browser/places/places.dtd 1.24

Comment 24

[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