Closed Bug 357316 Opened 14 years ago Closed 13 years ago

Implement Fx2-style Add/Edit items UI

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 3 alpha4

People

(Reporter: moco, Assigned: mano)

References

Details

(Whiteboard: [Fx2-parity])

Attachments

(4 files, 11 obsolete files)

37.82 KB, patch
Details | Diff | Splinter Review
69.67 KB, patch
Details | Diff | Splinter Review
30.99 KB, patch
dietrich
: review+
Details | Diff | Splinter Review
58.05 KB, patch
Details | Diff | Splinter Review
implement Fx2 style "bookmark this page..." dialog on top of places backend
Attached patch wip patch (obsolete) — Splinter Review
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?
Assignee: nobody → dietrich
Status: NEW → ASSIGNED
Assignee: dietrich → thunder
Status: ASSIGNED → NEW
Assignee: thunder → mano
Keywords: meta
Whiteboard: [Fx2-parity]
Attached patch second round of changes (obsolete) — Splinter Review
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.
Attachment #256880 - Flags: superreview?(sspitzer)
Attachment #256880 - Flags: review?(dietrich)
Status: NEW → ASSIGNED
Keywords: meta
Hardware: PC → All
Target Milestone: --- → Firefox 3 alpha3
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]
************************************************************
Attachment #256880 - Flags: review?(dietrich)
Attachment #256880 - Attachment is obsolete: true
Attachment #256880 - Flags: superreview?(sspitzer)
Attached patch more... (obsolete) — Splinter Review
Attachment #257176 - Attachment is obsolete: true
Attached patch more... (obsolete) — Splinter Review
Attachment #257192 - Attachment is obsolete: true
Attached patch more... (obsolete) — Splinter Review
Attachment #257209 - Attachment is obsolete: true
Attached patch more... (obsolete) — Splinter Review
Attachment #257284 - Attachment is obsolete: true
Summary: implement Fx2 style "bookmark this page..." dialog on top of places backend → Implement Fx2-style Add/Edit items UI
Attached patch more.. (obsolete) — Splinter Review
Attachment #257287 - Attachment is obsolete: true
Attachment #257331 - Flags: review?(dietrich)
Attachment #257287 - Flags: review?(dietrich)
Attached patch more.. (obsolete) — Splinter Review
missed few files.
Attachment #257331 - Attachment is obsolete: true
Attachment #257332 - Flags: review?(dietrich)
Attachment #257331 - Flags: review?(dietrich)
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.
Attachment #257332 - Flags: review?(dietrich) → review+
"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
Attachment #257917 - Flags: review?(dietrich)
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/
Attachment #257917 - Flags: review?(dietrich) → review+
Attachment #257430 - Attachment description: for checkin → changes listed in comment 14 (checked in)
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
Attachment #257917 - Attachment description: more → Changes listed on comment 15 (checked in)
Attached patch The rest (obsolete) — Splinter Review
* 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.
Attachment #259548 - Flags: review?(sspitzer)
Target Milestone: Firefox 3 alpha3 → Firefox 3 alpha4
Attached patch Some cleanup (obsolete) — Splinter Review
Attachment #259548 - Attachment is obsolete: true
Attachment #259631 - Flags: review?(sspitzer)
Attachment #259548 - Flags: review?(sspitzer)
Attachment #259631 - Attachment description: Some clenaup → Some cleanup
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
Attachment #259631 - Flags: review?(sspitzer) → review+
(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
Attachment #259631 - Attachment is obsolete: true
"Fixed"; please file nits and polish separately.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
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.