322988 - Bookmark properties should be available for pages other than the currently-viewed page

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, P1)

Description

[Ben Goodger (use ben at mozilla dot org for email)]

We need a simple info panel for individual bookmarks/places that allow them to be edited. Something really simple is more than enough for right now.

Comment 1

[Annie Sullivan]

Joe, I think you're already working on this?

Comment 2

[Ben Goodger (use ben at mozilla dot org for email)]

*** Bug 317835 has been marked as a duplicate of this bug. ***

Comment 3

[Fredrik Henrysson]

*** Bug 325302 has been marked as a duplicate of this bug. ***

Comment 4

[Michael Mak]

Places are enabled starting 20060302 trunk nightly. However bookmark properties is still unavaliable.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060301 Firefox/1.6a1 ID:2006030201

Comment 5

[Brett Wilson]

*** Bug 329116 has been marked as a duplicate of this bug. ***

Comment 6

[hhh]

I can't seem to view properties or rename a bookmark of the current page either. Better summary for this bug, maybe?

Comment 7

[Joe Hughes]

Attachment: Patch that enables editing bookmark properties from context menu.

Comment 8

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 213926 [details] [diff] [review]
Patch that enables editing bookmark properties from context menu.

>+  /**
>+   * Save any changes that might have been made while the properties dialog
>+   * was open.
>+   */
>+
>+  _saveChanges: function PBD_saveChanges() {

nit: skip the nl between comment and definition. 

>+      var urlbox = this._dialogWindow.document.getElementById("edit-urlbar");
>+      if (urlbox.value != this._bookmarkURI.spec) {
>+        // TODO delete existing bookmark, create new one with same folder/locations
>+      }

can you LOG() a message here saying this too, so we don't forget!!

>+  /**
>+   * This method is called to exit the Bookmark Properties panel.
>+   */
>+
>+  _hideBookmarkProperties:
>+  function BPP__hideBookmarkProperties() {

nit: this will fit on one line, no need to wrap

>Index: browser/components/places/content/bookmarkProperties.xul
>+	ondialogaccept="BookmarkPropertiesPanel.dialogDone();"
>+        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" onload="BookmarkPropertiesPanel.init(window, window.arguments[0], window.arguments[1]);">

nit: wrap your longer lines:

<window id="goat"
        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
        onload="...">

>+
>+  <script type="application/x-javascript"
>+	  src="chrome://browser/content/places/bookmarkProperties.js"/>

nit: line up attributes, you inserted a tab too. never use \t in mozilla code. 

>Index: browser/components/places/content/controller.js
>   /**
>+   * Opens the bookmark properties for the selected URI Node.
>+   */
>+
>+  showBookmarkPropertiesForSelection: 
>+  function PC_showBookmarkPropertiesForSelection() {

nit: again - no newline between comment and defn. 

>+  _assertURINotString: function PC__assertURINotString(value) {
>+    ASSERT((typeof(value) == "object") && !(value instanceof String), "This method should be passed a URI as a nsIURI object, not as a string.");
>+  },

nit: wrap long lines

>+  _updateControlStates: function PC__updateControlStates() {
>+  showHistory: function PC_showHistory() {
>+  showBookmarks: function PC_showBookmarks() {
>+  _showPlacesView: function PC__showPlacesView(uriString) {

controller.js should not know about the structure of the browser window. These functions need to stay in the PlacesBrowserShim object in browser.js

>Index: browser/themes/winstripe/browser/places/bookmarkProperties.css
> #bookmarkproperties {
>-  background-color: yellow;
>+  background-color: -moz-Dialog;
>+  color: -moz-DialogText;
>+  font: -moz-dialog;

You shouldn't have to do this. Make your <dialog> an actual <dialog> now. This will come for free.

Comment 9

[Joe Hughes]

Attachment: Enables editing bookmark properties from more places, adds shortcut (aka keyword editing), tidies up the PlacesBrowserShim in browser.js

Comment 10

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 214050 [details] [diff] [review]
Enables editing bookmark properties from more places, adds shortcut (aka keyword editing), tidies up the PlacesBrowserShim in browser.js

>Index: browser/locales/en-US/chrome/browser/places/places.dtd
>===================================================================
>RCS file: /cvsroot/mozilla/browser/locales/en-US/chrome/browser/places/places.dtd,v
>retrieving revision 1.1.2.3
>diff -u -u -8 -p -r1.1.2.3 places.dtd
>--- browser/locales/en-US/chrome/browser/places/places.dtd	28 Feb 2006 22:59:53 -0000	1.1.2.3
>+++ browser/locales/en-US/chrome/browser/places/places.dtd	5 Mar 2006 02:29:19 -0000
>@@ -201,22 +201,22 @@
> <!ENTITY location.status.not_bookmarked
>   "Star (Off)">
> <!ENTITY location.status.bookmarked
>   "Star (On)">
> <!ENTITY location.status.bookmark.tooltip
>   "Add a Bookmark to this page">
> <!ENTITY bookmark.property.panel.title
>   "Bookmark Properties">
>+<!ENTITY bookmark.property.location
>+  "Location">

Separate .dtd file for these please

>Index: browser/components/places/content/bookmarkProperties.xul
> 
>+<?xml-stylesheet href="chrome://browser/skin/"?>

Don't include the browser skin, include chrome://global/skin/ instead - should be all you need for the dialog. 

>-		<button id="places-link" label="&cmd.show_bookmarks.label;" oncommand="PlacesBrowserShim.dialogShowBookmarks(window);"/>		
>-		<button label="&cmd.delete_bookmark.label;" oncommand="PlacesBrowserShim.dialogDeleteBookmark(window);" align="end"/>
>+		<button label="&cmd.delete_bookmark.label;" oncommand="BookmarkPropertiesPanel.dialogDeleteBookmark();" align="end"/>

nit: wrap

> 		<spacer flex="1"/>
>-		<button label="&cmd.hide_bookmark_properties.label;" oncommand="PlacesBrowserShim.dialogDone(window);"/>
>+		<button label="&cmd.hide_bookmark_properties.label;" oncommand="BookmarkPropertiesPanel.dialogDone();"/>

wrap!

>Index: browser/components/places/content/controller.js
>+  /* NOTE(jhughes): don't use yet; UI doesn't update correctly 2006-03-04*/

If the UI update doesn't work, file a bug on brettw and cite it here. 

One more patch.

Comment 11

[Joe Hughes]

Attachment: Incorporates requests from previous patch (line wrapping, moving some strings to a new DTD file).

Comment 12

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 214223 [details] [diff] [review]
Incorporates requests from previous patch (line wrapping, moving some strings to a new DTD file).

sr=[e-mail]

Comment 13

[Annie Sullivan]

Comment on attachment 214223 [details] [diff] [review]
Incorporates requests from previous patch (line wrapping, moving some strings to a new DTD file).

These are just some pretty small nits:

It seems like the terms tags and folders are still used interchangeably in some places -- is there any reason _populateTags(), _createTagElement(), tagClicked(), etc. don't refer to folders?

>+  _populateProperties: function BPP__populateProperties() {

Can you put comments above this function explaining what it does like you did with all the other functions?

>+  /**
>+   * Makes a URI from a spec.
>+   * @param   spec
>+   *          The string spec of the URI
>+   * @returns A URI object for the spec. 
>+   */
>+  _uri: function PC__uri(spec) {
>+    var ios = 
>+        Cc["@mozilla.org/network/io-service;1"].
>+        getService(Ci.nsIIOService);
>+    return ios.newURI(spec, null, null);
>+  },

Would it make sense to cache the IO Service here like we cache the bookmarks and history services?

>+  _assertURINotString: function PC__assertURINotString(value) {
>+    ASSERT((typeof(value) == "object") && !(value instanceof String), "This method should be passed a URI as a nsIURI object, not as a string.");
>+  },

Does this function really need to be duplicated in both controller.js and bookmarkProperties.js?

Comment 14

[Ben Goodger (use ben at mozilla dot org for email)]

*** Bug 329319 has been marked as a duplicate of this bug. ***

Comment 15

[Joe Hughes]

Attachment: A few final changes based on feedback; this patch reflects changes as committed on branch and trunk.

Comment 16

[Joe Hughes]

Notes from CVS commit:
* Allows the user to access Bookmark Properties dialog from context menus.
* Adds support for editing the bookmark shortcut (formerly keyword) in the Bookmark Properties dialog.
* Improves the appearance of the Bookmark Properties dialog.
* Removes dead code from PlacesBrowserShim in browser.js

NOTE: This doesn't reflect the final UI for this functionality; rather,
this change is intended to add functionality for users of the nightlies.

bug=322988
r=[e-mail]
sr=[e-mail]

Comment 17

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 214223 [details] [diff] [review]
Incorporates requests from previous patch (line wrapping, moving some strings to a new DTD file).

sr=[e-mail]

Comment 18

[Peter Lairo]

Nit 1: The dialog is taller than my screen (1024x768, WinXP). The titlebar (with the red X close button) is not reachable. To close the dialog one must (1) make the dialog shorter by dragging the bottom border up (this causes a sliver of the titlebar to appear, enough to grab & drag it), (2) drag the dialog down a bit, and (3) close via red X in upper-right of titlebar.

Nit 2: There are no "Cancel" and "OK" buttons (visible). How do we save our changes?

Comment 19

[James Ross]

Peter, see bug 329606 and bug 329761 for the height issue. There appears to be no bug for the fact it is a general UI disaster.

Comment 20

[Joe Hughes]

I hear you James (and Lairo).  As I mentioned above, this interface is mid-transition, and is likely to change dramatically soon.  However, feel free to file bugs against me (or discuss on mozilla.dev.apps.firefox) with particular concerns.  (My one request is that they be a little more specific than "general UI disaster". :] )

Comment 21

[James Ross]

Joe, certainly, see bug 329792 and bug 329795.

Comment 22

[Joe Hughes]

Thanks, James!

Comment 23

[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