422181 - Add Create Bookmark in history item context menu

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, enhancement)

Description

[Matthew Turnbull [Bluefang]]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b5pre) Gecko/2008031004 Minefield/3.0b5pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b5pre) Gecko/2008031004 Minefield/3.0b5pre

History items in the Places Library should have an option to create a new bookmark from them. This can be done through drag-and-drop , but I found that out by accident (it's not obvious).

Reproducible: Always

Steps to Reproduce:
1. Right-click (open context menu) on a history item
2. 
3.
Actual Results:  
No option to create new bookmark.

Expected Results:  
Presented an option to create a new bookmark. This open the new bookmark dialog populated with the information with that from the history item.

Comment 1

[Michael Kohler [:mkohler]]

Attachment: Patch v1

Comment 2

[Michael Kohler [:mkohler]]

huh, just seen that this is now also on the sidebar, so it's doubled there. How can I make sure mine doesn't show in the sidebar?

Comment 3

[Dietrich Ayala (:dietrich)]

Comment on attachment 374672 [details] [diff] [review]
Patch v1

r- per comment #2. you should find out what's driving the sidebar context menu and re-use that if possible.

Comment 4

[Michael Kohler [:mkohler]]

>+    <menuitem id="placesContext_createBookmark"
>+              command="placesCmd_createBookmark"
>+              label="&cmd.create_bookmark.label;"
>+              accesskey="&cmd.create_bookmark.accesskey;"
>+              forcehideselection="bookmark"/>

is the code which adds the menuitem "Bookmark this Site.." to the context menu. How can I disable it in the sidebar?

Comment 5

[Dietrich Ayala (:dietrich)]

talked over irc. instead of duplicating this code, should instead update the history sidebar to use the new function in the controller. should update the controller function to incorporate the check that the existing addHistoryBookmarks() does.

Comment 6

[Michael Kohler [:mkohler]]

Attachment: Patch v2

Comment 7

[Dietrich Ayala (:dietrich)]

Comment on attachment 374815 [details] [diff] [review]
Patch v2

>diff --git a/browser/components/places/content/controller.js b/browser/components/places/content/controller.js
>--- a/browser/components/places/content/controller.js
>+++ b/browser/components/places/content/controller.js
>@@ -187,16 +187,18 @@ PlacesController.prototype = {
>       return selectedNode && PlacesUtils.nodeIsLivemarkContainer(selectedNode);
>     case "placesCmd_sortBy:name":
>       var selectedNode = this._view.selectedNode;
>       return selectedNode &&
>              PlacesUtils.nodeIsFolder(selectedNode) &&
>              !PlacesUtils.nodeIsReadOnly(selectedNode) &&
>              this._view.getResult().sortingMode ==
>                  Ci.nsINavHistoryQueryOptions.SORT_BY_NONE;
>+    case "placesCmd_createBookmark":
>+      return this._hasRemovableSelection(false);
>     default:

i don't understand this.

actually, seems like maybe should have the uri node checks to here, instead of in the command execution itself.

>@@ -275,16 +277,26 @@ PlacesController.prototype = {
>       this.reloadSelectedLivemark();
>       break;
>     case "placesCmd_reloadMicrosummary":
>       this.reloadSelectedMicrosummary();
>       break;
>     case "placesCmd_sortBy:name":
>       this.sortFolderByName();
>       break;
>+    case "placesCmd_createBookmark":
>+      var node = this._view.selectedNode;
>+      if (node && PlacesUtils.nodeIsURI(node))
>+        PlacesUIUtils.showMinimalAddBookmarkUI(PlacesUtils._uri(node.uri), node.title);
>+      else {
>+        let aUri = PlacesUtils._uri(this._view.selectedNode.uri);
>+        let aTitle = this._view.selectedNode.title;
>+        PlacesUIUtils.showMinimalAddBookmarkUI(aUri, aTitle);
>+      }
>+      break;
>     }
>   },

the |if| and the |else| both seem to contain code that does the exact same thing...

> 
>-  <popup id="placesContext">
>-    <menuitem id="addBookmarkContextItem"
>-              label="&bookmarkLink.label;"
>-              accesskey="&bookmarkLink.accesskey;"
>-              selection="link"
>-              selectiontype="single"
>-              oncommand="historyAddBookmarks();"/>
>-  </popup>
>+  <!-- We need this, otherwise we would break the right click -->
>+  <popup id="placesContext" />

"required to overlay the context menu"

>diff --git a/browser/locales/en-US/chrome/browser/places/places.dtd b/browser/locales/en-US/chrome/browser/places/places.dtd
>--- a/browser/locales/en-US/chrome/browser/places/places.dtd
>+++ b/browser/locales/en-US/chrome/browser/places/places.dtd
>@@ -80,16 +80,19 @@
> <!ENTITY cmd.open.accesskey              "O">
> <!ENTITY cmd.open_window.label           "Open in a New Window">
> <!ENTITY cmd.open_window.accesskey       "N">
> <!ENTITY cmd.open_tab.label              "Open in a New Tab">
> <!ENTITY cmd.open_tab.accesskey          "w">
> <!ENTITY cmd.open_all_in_tabs.label      "Open All in Tabs">
> <!ENTITY cmd.open_all_in_tabs.accesskey  "O">
> 
>+<!ENTITY cmd.create_bookmark.label       "Bookmark This Site..">
>+<!ENTITY cmd.create_bookmark.accesskey   "B">
>+

maybe should move the string used by old history context menu item from history.dtd to here. this makes it so that a new string isn't added, as well as using the existing string for consistency.

Comment 8

[Michael Kohler [:mkohler]]

Attachment: Patch v2.1 (includes changes from comment 7)

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 374953 [details] [diff] [review]
Patch v2.1 (includes changes from comment 7)

>diff --git a/browser/components/places/content/controller.js b/browser/components/places/content/controller.js
>--- a/browser/components/places/content/controller.js
>+++ b/browser/components/places/content/controller.js
>@@ -187,16 +187,19 @@ PlacesController.prototype = {
>       return selectedNode && PlacesUtils.nodeIsLivemarkContainer(selectedNode);
>     case "placesCmd_sortBy:name":
>       var selectedNode = this._view.selectedNode;
>       return selectedNode &&
>              PlacesUtils.nodeIsFolder(selectedNode) &&
>              !PlacesUtils.nodeIsReadOnly(selectedNode) &&
>              this._view.getResult().sortingMode ==
>                  Ci.nsINavHistoryQueryOptions.SORT_BY_NONE;
>+    case "placesCmd_createBookmark":
>+      var node = this._view.selectedNode;
>+      return node && PlacesUtils.nodeIsURI(node);

this should be disabled for bookmarks, so please also check node.itemId == -1
This was not needed before since was applying only to history sidebar.

>diff --git a/browser/components/places/content/placesOverlay.xul b/browser/components/places/content/placesOverlay.xul
>--- a/browser/components/places/content/placesOverlay.xul
>+++ b/browser/components/places/content/placesOverlay.xul
>@@ -15,16 +15,17 @@
> #
> # The Initial Developer of the Original Code is Google Inc.
> # Portions created by the Initial Developer are Copyright (C) 2005-2006
> # the Initial Developer. All Rights Reserved.
> #
> # Contributor(s):
> #   Ben Goodger <[e-mail]>
> #   Asaf Romano <[e-mail]>
>+#   Michael Kohler <[e-mail]>
> #
> # Alternatively, the contents of this file may be used under the terms of
> # either the GNU General Public License Version 2 or later (the "GPL"), or
> # the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
> # in which case the provisions of the GPL or the LGPL are applicable instead
> # of those above. If you wish to allow use of your version of this file only
> # under the terms of either the GPL or the LGPL, and not to allow others to
> # use your version of this file under the terms of the MPL, indicate your
>@@ -88,16 +89,18 @@
>     <command id="placesCmd_reloadMicrosummary"
>              oncommand="goDoCommand('placesCmd_reloadMicrosummary');"/>
>     <command id="placesCmd_sortBy:name"
>              oncommand="goDoCommand('placesCmd_sortBy:name');"/>
>     <command id="placesCmd_moveBookmarks"
>              oncommand="goDoCommand('placesCmd_moveBookmarks');"/>
>     <command id="placesCmd_deleteDataHost"
>              oncommand="goDoCommand('placesCmd_deleteDataHost');"/>
>+    <command id="placesCmd_createBookmark"
>+             oncommand="goDoCommand('placesCmd_createBookmark');"/>
>   </commandset>
> 
>   <popup id="placesContext"
>          onpopupshowing="this._view = PlacesUIUtils.getViewForNode(document.popupNode);
>                          return this._view.buildContextMenu(this);"
>          onpopuphiding="this._view.destroyContextMenu();">
>     <menuitem id="placesContext_open"
>               command="placesCmd_open"
>@@ -150,16 +153,21 @@
>     <menuitem id="placesContext_new:separator"
>               command="placesCmd_new:separator"
>               label="&cmd.new_separator.label;"
>               accesskey="&cmd.new_separator.accesskey;"
>               closemenu="single"
>               selection="any"
>               hideifnoinsertionpoint="true"/>
>     <menuseparator id="placesContext_newSeparator"/>
>+    <menuitem id="placesContext_createBookmark"
>+              command="placesCmd_createBookmark"
>+              label="&bookmarkLink.label;"
>+              accesskey="&bookmarkLink.accesskey;"
>+              hideselection="bookmark|tagChild|separator|query"/>

hideselection does not exists, you should use selection="link" and forcehideselection="bookmark|tagChild" (should be enough, but please test this!

>diff --git a/browser/locales/en-US/chrome/browser/history/history.dtd b/browser/locales/en-US/chrome/browser/history/history.dtd
>--- a/browser/locales/en-US/chrome/browser/history/history.dtd
>+++ b/browser/locales/en-US/chrome/browser/history/history.dtd
>@@ -17,12 +17,10 @@
> <!ENTITY byDayAndSite.label "By Date and Site">
> <!ENTITY byDayAndSite.accesskey "t">
> <!ENTITY openLinkInWindow.label "Open">
> <!ENTITY openLinkInWindow.accesskey "O">
> <!ENTITY openInNewTab.label "Open in New Tab">
> <!ENTITY openInNewTab.accesskey "T">
> <!ENTITY openInNewWindow.label "Open in New Window">
> <!ENTITY openInNewWindow.accesskey "W">
>-<!ENTITY bookmarkLink.label "Bookmark This Linkâ¦">
>-<!ENTITY bookmarkLink.accesskey "B">
> <!ENTITY copyLink.label "Copy Link Location">
> <!ENTITY copyLink.accesskey "C">
>diff --git a/browser/locales/en-US/chrome/browser/places/places.dtd b/browser/locales/en-US/chrome/browser/places/places.dtd
>--- a/browser/locales/en-US/chrome/browser/places/places.dtd
>+++ b/browser/locales/en-US/chrome/browser/places/places.dtd
>@@ -80,16 +80,19 @@
> <!ENTITY cmd.open.accesskey              "O">
> <!ENTITY cmd.open_window.label           "Open in a New Window">
> <!ENTITY cmd.open_window.accesskey       "N">
> <!ENTITY cmd.open_tab.label              "Open in a New Tab">
> <!ENTITY cmd.open_tab.accesskey          "w">
> <!ENTITY cmd.open_all_in_tabs.label      "Open All in Tabs">
> <!ENTITY cmd.open_all_in_tabs.accesskey  "O">
> 
>+<!ENTITY bookmarkLink.label       "Bookmark This Site..">
>+<!ENTITY bookmarkLink.accesskey   "B">
>+

Is the change from "Link" to "Site" wanted? you should ask someone in the UX-team about that, but i think "Link" or "Page" is better since is referred to one specific url. The entity name should be something like cmd.bookmarkLink.label and cmd.bookmarkLink.accessKey and be grouped visually with other commands entity referring to the same context menu, for coherence.

Comment 10

[Michael Kohler [:mkohler]]

(in reply to comment 9)
> Is the change from "Link" to "Site" wanted? you should ask someone in the
> UX-team about that, but i think "Link" or "Page" is better since is referred to
> one specific url.

from IRC #ux:
<limi> MichaelKohler: yeah, you bookmark a page, not a site

Comment 11

[Michael Kohler [:mkohler]]

Attachment: includes the changes from comment 9

this patch includes the fixes from comment 9.

let's try again :)

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 376041 [details] [diff] [review]
includes the changes from comment 9

>diff --git a/browser/components/places/content/placesOverlay.xul b/browser/components/places/content/placesOverlay.xul
>--- a/browser/components/places/content/placesOverlay.xul
>+++ b/browser/components/places/content/placesOverlay.xul
>@@ -150,16 +153,22 @@
>     <menuitem id="placesContext_new:separator"
>               command="placesCmd_new:separator"
>               label="&cmd.new_separator.label;"
>               accesskey="&cmd.new_separator.accesskey;"
>               closemenu="single"
>               selection="any"
>               hideifnoinsertionpoint="true"/>
>     <menuseparator id="placesContext_newSeparator"/>
>+    <menuitem id="placesContext_createBookmark"
>+              command="placesCmd_createBookmark"
>+              label="&cmd.bookmarkLink.label;"
>+              accesskey="&cmd.bookmarkLink.accesskey;"
>+              selection="link"
>+              forcehideselection="bookmark|tagChild"/>

just for information, have you manually tested this is correct on all the different kind of nodes we have?
Sounds correct, but some additional testing would be appreciated.

r=me with the above answered.
Notice this functionality will be tested with bug 491269, since i need to use this to open the dialog.

Please, get an explicit ui-r from Limi on the text, just to make that official.

Comment 13

[Michael Kohler [:mkohler]]

I tested this manually and it is okay like it is now. Needs no further work here. Asking for a ui-r from Limi now for the naming.

Comment 14

[Michael Kohler [:mkohler]]

'checkin-needed' since this patch has a ui-r+

Comment 15

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/0aed00fef1ac

Comment 16

[Marco Bonardo [:mak]]

can't make 1.9.1 due to l10n changes

Comment 17

[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