implement Fx2 style "bookmark this page..." dialog on top of places backend

still needs: - dialog doesnt resize at load (blank space where tree would be) - tree height is wrong - add description field (as anno) - evaluate if we need to port the microsummary picker from branch, or keep existing - change strings back to fx2 strings (eg: "shortcut" instead of keyword) - add MRU folders to menulist - Mano - "the js code seems to try very hard to look like a component" - add Faaborg-style radio boxes to title/LM/MS?

See bug 371840 for the first set of changes. * Implement "New Bookmark" in context menu and in the organizer window. * Hide various fields from "Bookmark This Page" dialog. * default insertion point support - esp. important for the New Bookmark command * documentation and simplified API for this dialog.

Comment on attachment 256880 [details] [diff] [review] second round of changes >@@ -32,16 +32,56 @@ > * use your version of this file under the terms of the MPL, indicate your > * decision by deleting the provisions above and replace them with the notice > * and other provisions required by the GPL or the LGPL. If you do not delete > * the provisions above, a recipient may use your version of this file under > * the terms of any one of the MPL, the GPL or the LGPL. > * > * ***** END LICENSE BLOCK ***** */ > >+/** >+ * The panel is initialized based on data given in the js object passed >+ * as window.arguments[0]. The object should have the following fields: "should" or "must"? It would be helpful to specify which if these are optional, if any, and what the defaults are. >+ * @ action (string). Possible values: >+ * - "add" - for adding a new item. >+ * @ type (string). Possible values: >+ * - "bookmark" >+ * - "folder" (not yet implemented) >+ * - "folder with items" >+ * @ URIList (nsIURI array)- list of uris to be bookmarked under the >+ * new folder. >+ * - "livemark" (not yet implemented) >+ * @ uri (nsIURI) - optional, the default uri for the new item. >+ * The property is not used for the "folder with items" type. >+ * @ title (string) - optional, the defualt title for the new item. >+ * @ defaultInsertionPoint (InsertionPoint) - optional, the default >+ * insertion point for the new item. >+ * Notes: >+ * 1) If |uri| is set for a bookmark/livemark item and |title| isn't, >+ * the dialog will query the history tables for the title associated >+ * with the given uri. For "folder with items" folder, a default >+ * static title is used ("[Folder Name]"). >+ * 2) The index field of the the default insertion point is ignored if >+ * the folder picker is shown. >+ * - "edit" - for editing a bookmark item or a folder. >+ * @ type (string). Possible values: >+ * - "bookmark" >+ * @ bookmarkId (integer) - the id of the bookmark item. >+ * - "folder" (also applies to livemarks) >+ * @ folderId (integer) - the id of the folder. >+ * @ hiddenFields (strings array) - optional, list of fields to be hidden. >+ * Possible values: >+ * - "title" >+ * - "location" >+ * - "description" (not yet implemented) >+ * - "keyword" >+ * - "folder picker" (hides both the tree and the menu) >+ * - "show in sidebar" (not yet implemented) Should make clear that regardless of the contents of this array, there are other factors that will determine whether a field is shown or not. Eg, passing in an empty array does not ensure that all fields will be shown. >+ _determineItemInfo: function BPP__determineItemInfo() { >+ var dialogInfo = window.arguments[0]; >+ var action = dialogInfo.action; >+ if (action == "add") { >+ NS_ASSERT("type" in dialogInfo, "missing type property for add action"); Maybe should assert if there's no |action| property, like you do with |type|? > /** > * Save any changes that might have been made while the properties dialog > * was open. > */ > _saveChanges: function BPP__saveChanges() { > var transactions = []; > var urlbox = this._element("editURLBar"); > var titlebox = this._element("editTitleBox"); > var newURI = this._bookmarkURI; > if (this._itemType == BOOKMARK_ITEM) > newURI = PlacesUtils._uri(urlbox.value); > > // adding one or more bookmarks > if (this._action == ACTION_ADD || this._action == ACTION_ADD_WITH_ITEMS) { >- var folder = PlacesUtils.bookmarks.bookmarksRoot; >- var selected = this._folderTree.getSelectionNodes(); >+ var folderId, index = -1; >+ if (this._defaultInsertionPoint) { >+ folderId = this._defaultInsertionPoint.folderId; >+ index = this._defaultInsertionPoint.index; >+ } >+ else if (!this._element("folderRow").hidden && !this._folderTree.hidden) >+ folderId = asFolder(this._folderTree.selectedNode).folderId; >+ else >+ folderId = PlacesUtils.bookmarks.bookmarksRoot; If a default insertion point was passed in, and the tree showing, wouldn't this ignore a tree selection by the user? >Index: browser/components/places/content/bookmarkProperties.xul >=================================================================== >RCS file: /cvsroot/mozilla/browser/components/places/content/bookmarkProperties.xul,v >retrieving revision 1.16 >diff -u -p -8 -r1.16 bookmarkProperties.xul >--- browser/components/places/content/bookmarkProperties.xul 28 Feb 2007 17:49:49 -0000 1.16 >+++ browser/components/places/content/bookmarkProperties.xul 1 Mar 2007 03:13:49 -0000 >@@ -100,18 +103,17 @@ > </vbox> > <vbox> > <tree id="folderTree" > class="placesTree" > flex="1" > type="places" > place="place:&amp;folder=1&amp;group=3&amp;excludeItems=1&amp;excludeQueries=1&amp;excludeReadOnlyFolders=1" > hidecolumnpicker="true" >- seltype="multiple" >- onselect="BookmarkPropertiesPanel.validateChanges()"> >+ seltype="multiple"> Selecting of multiple folders is disallowed in saveChanges, also needs to be disallowed here. >Index: browser/components/places/content/placesOverlay.xul >=================================================================== >RCS file: /cvsroot/mozilla/browser/components/places/content/placesOverlay.xul,v >retrieving revision 1.6 >diff -u -p -8 -r1.6 placesOverlay.xul >--- browser/components/places/content/placesOverlay.xul 19 Feb 2007 20:12:25 -0000 1.6 >+++ browser/components/places/content/placesOverlay.xul 1 Mar 2007 03:13:55 -0000 >@@ -125,16 +1election="folder"/> > <menuitem id="placesContext_openLinks:tabs" > command="placesCmd_open:tabs" > label="&cmd.open_all_in_tabs.label;" > accesskey="&cmd.open_all_in_tabs.accesskey;" > selectiontype="multiple" > selection="link"/> > <menuseparator id="placesContext_openSeparator"/> >+ <menuitem id="placesContext_new:bookmark" >+ command="placesCmd_new:bookmark" >+ label="&cmd.new_bookmark.label;" >+ accesskey="&cmd.new_bookmark.accesskey;" >+ selection="mutable"/> This fails when i try to create a bookmark this way, whether there's a value or not in the keyword field. I think the EditBookmarkKeyword transaction needs to get bundled up as a child of the CreateItem transaction when the action is add. ************************************************************ * Call to xpconnect wrapped JSObject produced this error: * [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsINavBookmarksService.setKeywordForBookmark]" nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)" location: "JS frame :: chrome://browser/content/places/controller.js :: PEBKT_doTransaction :: line 1899" data: no] ************************************************************

missed few files.

Comment on attachment 257332 [details] [diff] [review] more.. Is there already a bug for making the microsummary UI like Fx2 when adding bookmarks? Not being able to set the microsummary when adding the bookmark is a significant regression. I think we should leave that field visible when adding bookmarks, until we get UI parity there. Besides that, it looks good. Thanks for addressing the previous issues, r=me.

"More UI-parity pieces for places-bookmarks. Changes include: 1) Implement New Bookmark and New Livemark commands and UI using the shared bookmark properties dialog (bug 365102). 2) Make New Folder command use the shared dialog as well. 3) Show dialog when adding a live-bookmark (bug 332965). 4) Hide most fields when adding a bookmark using 'Bookmark This Page...'. 5) defualt titles for each item type (bookmark/folder/livemark). 6) Changing the uri of a bookmark is now undo-able. 7) Tidy up and document the dialog pseudo-API." mozilla/browser/base/content/browser-places.js 1.30 mozilla/browser/components/places/content/bookmarkProperties.js 1.32 mozilla/browser/components/places/content/bookmarkProperties.xul 1.17 mozilla/browser/components/places/content/controller.js 1.130 mozilla/browser/components/places/content/places.xul 1.63 mozilla/browser/components/places/content/placesOverlay.xul 1.7 mozilla/browser/components/places/content/utils.js 1.17 mozilla/browser/locales/en-US/chrome/browser/places/bookmarkProperties .properties 1.9; previous revision: 1.8 mozilla/browser/locales/en-US/chrome/browser/places/places.dtd 1.18 mozilla/browser/locales/en-US/chrome/browser/places/places.properties 1.16

* description annotation * fix callers to pass the default description, not sure whether i got them all right. * s/toolbarRoot/toolbarFolder in addLiveBookmark * XUL cleanup * a11y - set the control for each label

Comment on attachment 257917 [details] [diff] [review] Changes listed on comment 15 (checked in) > > var toolbarRootIP = >- new InsertionPoint(PlacesUtils.bookmarks.toolbarRoot, -1); >- PlacesUtils.showAddLivemarkUI(feedURI, browser.currentURI, >- title, toolbarRootIP, true); >+ new InsertionPoint(PlacesUtils.bookmarks.toolbarFolder, -1); >+ PlacesUtils.showAddLivemarkUI(feedURI, gBrowser.currentURI, >+ title, description, toolbarRootIP, true); > }, nit: should just call it toolbarIP, as it's not a root >+++ browser/components/places/content/bookmarkProperties.js 9 Mar 2007 04:30:37 -0000 >@@ -49,16 +49,18 @@ > * - "folder" > * - "folder with items" > * @ URIList (Array of nsIURI objects)- list of uris to be bookmarked > * under the new folder. > * - "livemark" > * @ uri (nsIURI object) - optional, the default uri for the new item. > * The property is not used for the "folder with items" type. > * @ title (String) - optional, the defualt title for the new item. >+ * @ description (String) - optional, the default direction for the new >+ * item. s/direction/description/

Comment on attachment 257917 [details] [diff] [review] Changes listed on comment 15 (checked in) mozilla/browser/base/content/browser-places.js 1.31 mozilla/browser/base/content/nsContextMenu.js 1.13 mozilla/browser/components/places/content/bookmarkProperties.js 1.34 mozilla/browser/components/places/content/bookmarkProperties.xul 1.19 mozilla/browser/components/places/content/controller.js 1.135 mozilla/browser/components/places/content/utils.js 1.21 mozilla/browser/locales/en-US/chrome/browser/places/bookmarkProperties.dtd 1.10

* 2.0-style folder picker. For now, I implemented the recently used annotations using annotations, we may want to revise this when we get some spare time. * persist width/screenX/screenY - for "Bookmarks This Page"-llke UI we separate these on a separate chrome: uri. * The ported resizeTo hack doesn't work all that well, this might be a cocoa-widget issue, I will file a follow up to sort this out. * New Folder button. * Fixed labels.

Comment on attachment 259631 [details] [diff] [review] Some cleanup x) +# Provide another URI for the bookmarkProperties dialog so we can persist the +# attributes separately +% override chrome://browser/content/places/bookmarkPageDialog.xul chrome://browser/content/places/bookmarkProperties.xul Very cool, I didn't know we could do that. x) + // sizeToContent is not usable due to bug 90276, so we'll use resizeTo + // instead and cache the bookmarks tree view size. See WSucks in the legacy + // UI code (addBookmark2.js), apparently we're a bit more mature. Was WSucks for Windows Sucks or something? I'd suggest leaving out the ",apparently we're a bit more mature." part. x) + /** + * @see showAddBookmarkUI should be "@see showMinimalAddBookmarkUI", I think. x) + /** + * @see showAddLivemarkUI should be "@see showMinimalAddLivemarkUI", I think. x) + showMinimalAddLivemarkUI: + function PU_showAddLivemarkURI(aFeedURI, aSiteURI, aTitle, aDescription, Should be "function PU_showMinimalAddLivemarkUI(...", I think. x) are we ever passing in false for aShowPicker and non-null for aDefaultInsertionPoint? + if (aDefaultInsertionPoint) { + info.defaultInsertionPoint = aDefaultInsertionPoint; + if (!aShowPicker) + info.hiddenRows = ["folder picker"]; } Are we ever passing false for aShowPicker, but a null for aDefaultInsertionPoint?? Actually, are ever passing in a non-null aDefaultInsertionPoint when aShowPicker is true? For showMinimalAddLivemarkUI() and showAddLivemarkURI(), maybe we don't need aShowPicker. (I'm not sure about Note sure about showMinimalAddBookmarkUI() and showAddBookmarkUI() x) +/**** exapnder ****/ +#expander { + -moz-appearance: none; + margin-left: 8px; + padding: 0; + min-width: 0; +} typo in the comment, exapnder -> expander

(In reply to comment #20) > (From update of attachment 259631 [details] [diff] [review]) > + // sizeToContent is not usable due to bug 90276, so we'll use resizeTo > + // instead and cache the bookmarks tree view size. See WSucks in the legacy > + // UI code (addBookmark2.js), apparently we're a bit more mature. > > Was WSucks for Windows Sucks or something? > > I'd suggest leaving out the ",apparently we're a bit more mature." part. yes; whatever. > + /** > + * @see showAddBookmarkUI > > should be "@see showMinimalAddBookmarkUI", I think. er, "@see" stands for "see the other method for arguments documentation". > > + /** > + * @see showAddLivemarkUI > > should be "@see showMinimalAddLivemarkUI", I think. > ditto. > are we ever passing in false for aShowPicker and non-null for > aDefaultInsertionPoint? > yes, for the new bookmark/new livebookmark/new folder commands. > + if (aDefaultInsertionPoint) { > + info.defaultInsertionPoint = aDefaultInsertionPoint; > + if (!aShowPicker) > + info.hiddenRows = ["folder picker"]; > } > > Are we ever passing false for aShowPicker, but a null for > aDefaultInsertionPoint?? no, and that combination is invalid, we could add an assert. > Actually, are ever passing in a non-null aDefaultInsertionPoint when > aShowPicker is true? > yes, when adding a live bookmark from the feed preview page. > For showMinimalAddLivemarkUI() and showAddLivemarkURI(), maybe we don't need > aShowPicker. (I'm not sure about > I would like to keep the arguments in sync, thus "@see". > Note sure about showMinimalAddBookmarkUI() and showAddBookmarkUI() hrm?

>> should be "@see showMinimalAddBookmarkUI", I think. > >er, "@see" stands for "see the other method for arguments documentation". duh. thanks. >> For showMinimalAddLivemarkUI() and showAddLivemarkURI(), maybe we don't need >> aShowPicker. (I'm not sure about > >I would like to keep the arguments in sync, thus "@see". OK. >> Note sure about showMinimalAddBookmarkUI() and showAddBookmarkUI() > hrm? Given that you want to keep the arguments in sync, you can ignore this.

mozilla/browser/base/content/browser-places.js 1.32 mozilla/browser/components/places/jar.mn 1.35 mozilla/browser/components/places/content/bookmarkProperties.js 1.39 mozilla/browser/components/places/content/bookmarkProperties.xul 1.21 mozilla/browser/components/places/content/utils.js 1.25 mozilla/browser/locales/en-US/chrome/browser/places/bookmarkProperties.dtd 1.12 mozilla/browser/locales/en-US/chrome/browser/places/bookmarkProperties.properties 1.10 mozilla/browser/themes/pinstripe/browser/jar.mn 1.40 mozilla/browser/themes/pinstripe/browser/places/bookmarkProperties.css 1.7 mozilla/browser/themes/winstripe/browser/places/bookmarkProperties.css 1.7

"Fixed"; please file nits and polish separately.

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