Closed Bug 422181 Opened 12 years ago Closed 11 years ago

Add Create Bookmark in history item context menu

Categories

(Firefox :: Bookmarks & History, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
Firefox 3.6a1

People

(Reporter: sparky, Assigned: mkohler)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Depends on: 422193
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → michaelkohler
Status: NEW → ASSIGNED
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #374672 - Flags: review?
Attachment #374672 - Flags: review? → review?(dietrich)
Whiteboard: [needs review dietrich]
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?
Version: unspecified → Trunk
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.
Attachment #374672 - Flags: review?(dietrich) → review-
>+    <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?
Version: Trunk → unspecified
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.
Whiteboard: [needs review dietrich]
Version: unspecified → Trunk
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #374672 - Attachment is obsolete: true
Attachment #374815 - Flags: review?(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.
Attachment #374815 - Flags: review?(dietrich) → review-
Attachment #374815 - Attachment is obsolete: true
Attachment #374953 - Flags: review?(mak77)
Summary: Add Create Bookmark in history item content menu → Add Create Bookmark in history item context menu
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 <beng@google.com>
> #   Asaf Romano <mano@mozilla.com>
>+#   Michael Kohler <michaelkohler@live.com>
> #
> # 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.
Attachment #374953 - Flags: review?(mak77) → review-
(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
this patch includes the fixes from comment 9.

let's try again :)
Attachment #374953 - Attachment is obsolete: true
Attachment #376041 - Flags: review?(mak77)
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.
Attachment #376041 - Flags: review?(mak77) → review+
Attachment #376041 - Flags: ui-review?(limi)
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.
Attachment #376041 - Flags: ui-review?(limi) → ui-review+
'checkin-needed' since this patch has a ui-r+
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/0aed00fef1ac
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.6a1
can't make 1.9.1 due to l10n changes
Blocks: 491269
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.