Closed Bug 330023 Opened 18 years ago Closed 18 years ago

No folder properties

Categories

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

x86
Windows XP
defect

Tracking

()

RESOLVED FIXED
Firefox 2 alpha2

People

(Reporter: ria.klaassen, Assigned: mozilla)

References

Details

(Keywords: fixed1.8.1)

Attachments

(1 file, 2 obsolete files)

Steps to reproduce:
1. Rightclick on bookmarks folder, choose Properties.
2. Nothing happens.
In case you want to rename the folder for instance.
Assignee: nobody → joe
Priority: -- → P1
Target Milestone: --- → Firefox 2 alpha2
Same here. Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20060318 Firefox/2.0a1
Attachment #215710 - Flags: superreview?(bugs)
Attachment #215710 - Flags: review?(annie.sullivan)
Comment on attachment 215710 [details] [diff] [review]
Adds support for editing folder names (relative to patch 329792)

>--- browser/components/places.329792/content/bookmarkProperties.js	2006-03-20 11:43:22.000000000 -0800
>-      if (this._bms.isBookmarked(uri)) {
>+      if (typeof(identifier)=="number")

nit: spaces around == 

>+    if (this._identifierIsURI()) {
>+      this._bookmarkTitle = this._bms.getItemTitle(this._bookmarkURI);
>+    } else {
>+      this._bookmarkTitle = this._bms.getFolderTitle(this._folderId);
>+    }

nit: 

}
else {

not 

} else {

>+    } else {

See above. 

>   /**
>+   * Opens the folder properties panel for a given folder ID.
>+   *
>+   * @param folderid   an integer representing the ID of the folder to edit
>+   * @returns none
>+   */
>+
>+  showFolderProperties: function PC_showFolderProperties(folderId) {

nit: No newline between comment and function. 

>+  _showBookmarkDialog: function PC__showBookmarkDialog(identifier, action) {
>+    if (typeof(identifier) != "number")
>+      this._assertURINotString(identifier);

This is the wrong assert type. You should do:

NS_ASSERT(typeof(identifier) == "number", "identifier must be a folderId");

r-, for that last point. Revise your patch to fix this & the other nits.
Attachment #215710 - Flags: superreview?(bugs) → superreview-
Comment on attachment 215710 [details] [diff] [review]
Adds support for editing folder names (relative to patch 329792)

joe explained this some more. sr+ conditional on nits. Would be nice to have a comment saying "if this is not a folderId (i.e. an integer) then assert that itmust be a URI")
Attachment #215710 - Flags: superreview- → superreview+
Blocks: 331154
Attachment #215710 - Attachment is obsolete: true
Attachment #215714 - Flags: review?(annie.sullivan)
Attachment #215710 - Flags: review?(annie.sullivan)
Comment on attachment 215710 [details] [diff] [review]
Adds support for editing folder names (relative to patch 329792)


>-   * @param uri the URI to display the properties for
>+   * @param identifier the URI or folder ID to display the properties for
>    * @param action -- "add" if this is being triggered from an "add bookmark"
>    *                  UI action; or "edit" if this is being triggered from
>    *                  a "properties" UI action
>    *
>    * @returns one of the *_VARIANT constants
>    */
>-  _determineVariant: function BPP__determineVariant(uri, action) {
>+  _determineVariant: function BPP__determineVariant(identifier, action) {
>     if (action == "add") {
>-      if (this._bms.isBookmarked(uri)) {
>+      if (this._bms.isBookmarked(identifier)) {

isBookmarked() expects nsIURI.  Does _determineVariant() assume that it will never be called with identifier=folder_id and action="add"? If so, that should go in the comments, and an assertion should be added.

r=annie.sullivan@gmail.com, with this change.
Attachment #215710 - Attachment is obsolete: false
Attachment #215710 - Flags: review+
Comment on attachment 215714 [details] [diff] [review]
A small de-nitting update of the previous patch

Whoops, reviewed the wrong patch, but comment 6 still applies.
Attachment #215714 - Flags: review?(annie.sullivan) → review+
Attachment #215710 - Attachment is obsolete: true
Attachment #215714 - Attachment is obsolete: true
Submitted on branch and trunk.
Status: NEW → RESOLVED
Closed: 18 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
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.

Attachment

General

Creator:
Created:
Updated:
Size: