Closed
Bug 330023
Opened 18 years ago
Closed 18 years ago
No folder properties
Categories
(Firefox :: Bookmarks & History, defect, P1)
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.
Updated•18 years ago
|
Assignee: nobody → joe
Priority: -- → P1
Target Milestone: --- → Firefox 2 alpha2
Comment 1•18 years ago
|
||
Same here. Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20060318 Firefox/2.0a1
Assignee | ||
Comment 2•18 years ago
|
||
Assignee | ||
Updated•18 years ago
|
Attachment #215710 -
Flags: superreview?(bugs)
Attachment #215710 -
Flags: review?(annie.sullivan)
Comment 3•18 years ago
|
||
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 4•18 years ago
|
||
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 | ||
Comment 5•18 years ago
|
||
Attachment #215710 -
Attachment is obsolete: true
Attachment #215714 -
Flags: review?(annie.sullivan)
Attachment #215710 -
Flags: review?(annie.sullivan)
Comment 6•18 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•18 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•18 years ago
|
||
Attachment #215710 -
Attachment is obsolete: true
Attachment #215714 -
Attachment is obsolete: true
Assignee | ||
Comment 9•18 years ago
|
||
Submitted on branch and trunk.
Comment 10•15 years ago
|
||
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.
Description
•