No folder properties

RESOLVED FIXED in Firefox 2 alpha2

Status

()

P1
normal
RESOLVED FIXED
13 years ago
9 years ago

People

(Reporter: ria.klaassen, Assigned: mozilla)

Tracking

({fixed1.8.1})

Trunk
Firefox 2 alpha2
x86
Windows XP
fixed1.8.1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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

Comment 1

13 years ago
Same here. Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20060318 Firefox/2.0a1
(Assignee)

Updated

13 years ago
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+
(Assignee)

Updated

13 years ago
Blocks: 331154
(Assignee)

Comment 5

13 years ago
Attachment #215710 - Attachment is obsolete: true
Attachment #215714 - Flags: review?(annie.sullivan)
Attachment #215710 - Flags: review?(annie.sullivan)

Comment 6

13 years ago
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 7

13 years ago
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+
(Assignee)

Comment 8

13 years ago
Attachment #215710 - Attachment is obsolete: true
Attachment #215714 - Attachment is obsolete: true
(Assignee)

Comment 9

13 years ago
Submitted on branch and trunk.
Status: NEW → RESOLVED
Last Resolved: 13 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.