Closed Bug 387749 Opened 12 years ago Closed 12 years ago

add an item detail pane to the organizer

Categories

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

defect

Tracking

()

VERIFIED FIXED
Firefox 3 beta1

People

(Reporter: dietrich, Assigned: mano)

References

()

Details

(Whiteboard: [places-ui])

Attachments

(1 file, 12 obsolete files)

108.68 KB, patch
mconnor
: approval1.9+
Details | Diff | Splinter Review
see the bug URL for mockups and notes.

* is always visible
* clicking on an item in the content pane should fill in the properties pane
Blocks: 387750
Assignee: nobody → swon
Status: NEW → ASSIGNED
>* is always visible

Actually there should be a menu item in the organize menu to display or hide the preview pane (for consistency with windows vista).
It seems like detail pane should be consistent with the properties window for a bookmark/folder/livemark. Then the detail pane would contain different properties depending on the type of the selected item.

Is this correct, Alex?
Attached patch Checkpoint 1 (obsolete) — Splinter Review
Checkpoint 1.
Depends on: 387485
Blocks: 387740
Basic functionality's almost all there in the patch.
But need 387485 to finish the tagging field.
Target Milestone: --- → Firefox 3 M8
Whiteboard: Waiting on Tag Editor; Need code clean up, but most of the basic funcationality is in the checkpt.
Whiteboard: Waiting on Tag Editor; Need code clean up, but most of the basic funcationality is in the checkpt. → [swag:1d][places-ui]
Attached patch checkpt 2 (obsolete) — Splinter Review
Basic functionality is there, without the Tag Editor.
Alignment between labels and corresponding textboxes are off right now (will be fixed).
Attachment #272579 - Attachment is obsolete: true
Attachment #275541 - Flags: review?(dietrich)
Attached image screenshot of checkpoint 2 on mac (obsolete) —
Comment on attachment 275541 [details] [diff] [review]
checkpt 2

This should re-use the new popup overlay (I will make sure it can edit a particular bookmark and not just the most-recent-item).
Attachment #275541 - Flags: review?(dietrich) → review-
(In reply to comment #7)
> (From update of attachment 275541 [details] [diff] [review])
> This should re-use the new popup overlay (I will make sure it can edit a
> particular bookmark and not just the most-recent-item).
> 

mano: check out the screen shot i attached. things are already looking a bit tight. i'm worried that the folder selector might be too small to be useful when crammed inside a vertically-oriented pane like this. with your new rewrite of the popup, is it able to be externally disabled?

alex: can you take a look at the screenshot, as well as mano's above comment (shared UI between the popup and the details pane).

steve: after playing with this patch, i think you'll need to expand the default width of the organizer.
>This should re-use the new popup overlay (I will make sure it can edit
>a particular bookmark and not just the most-recent-item).

If we do reuse the code between the pop-up overlay and the detail pane, here are the sections that we want displayed in each:

==New bookmark pop-up==
* name
* folder
* tags

==Details pane==
* name
* location
* tags
* description
* keyword
* load in sidebar

So even if the code is shared, only two elements appear in both (name and tags.)

>i think you'll need to expand the default width of the organizer.

Let's set default width to 1024x768 we are also going to need the extra vertical space for the folder tree view + favorites area in the far left pane. 
Assignee: stevewon → nobody
Status: ASSIGNED → NEW
Here is the latest iteration of the Places Organizer:

http://people.mozilla.com/~faaborg/files/granParadisoUI/placesOrganizer_i5WindowLayout.png

Note that the properties pane now appears on the bottom instead of the right, and contains a progressive disclosure control.
Assignee: nobody → mano
Depends on: 385266
Priority: -- → P2
Summary: add an item detail pane to the right side of the organizer → add an item detail pane to the organizer
Attached patch wip (obsolete) — Splinter Review
Attachment #275541 - Attachment is obsolete: true
Attachment #275558 - Attachment is obsolete: true
asaf, for your wip patch, gEditItem is undefined:

JavaScript error: chrome://browser/content/places/places.js, line 338: gEditItemOverlay is not defined

do you have an updated wip patch that I can play with, as I'm working on adding the image preview part (in bug #387750)
Seth, you need the patch in bug 385266 for this to work.
oops, thanks asaf.  

See bug #387750 for a patch (and screen shot) that uses part of your patch here for image preview.
note to asaf, it looks like part of this patch has landed along with bug #385266 (the change to browser/components/places/content/places.js)

I'm running with a modified version of this WIP patch in my tree for bug #387750 and here's something I've noticed:

when selection changes in the left hand pane, and nothing is selected in the right hand pane, the detail pane is not cleared (to show "No Info")
Attached patch more... (obsolete) — Splinter Review
Attachment #277318 - Attachment is obsolete: true
Target Milestone: Firefox 3 M8 → Firefox 3 M9
Attached patch more wip (obsolete) — Splinter Review
Attachment #278996 - Attachment is obsolete: true
Comment on attachment 281493 [details] [diff] [review]
more wip

some issues:

- "load in sidebar" should be disabled for folders and livemarks and queries
- bookmark preview should be disabled for folders and livemarks and queries
- the details pane should automatically open/close contextually, eg:
  -> select an item in the right pane opens the details pane
  -> select an item on the left pane or search closes the details pane

> 
>+    var container = PlacesUtils.bookmarks.getFolderIdForItem(this._itemId);
>     if (this._itemType == Ci.nsINavBookmarksService.TYPE_BOOKMARK) {
>       this._uri = PlacesUtils.bookmarks.getBookmarkURI(this._itemId);
>-      // tags field
>-      this._element("tagsField").value =
>-        PlacesUtils.tagging.getTagsForURI(this._uri).join(", ");
> 
>-      this._element("locationField").value = this._uri.spec;
>+      if (PlacesUtils.annotations
>+                     .itemHasAnnotation(container, "livemark/feedURI"))
>+        this._readOnly = true;
>+      else
>+        this._readOnly = false;

why this instead of using PlacesUtils.livemarks.isLivemark()?

>+  _updatePreview: function PO__updatePreview() {
>+    var bo = document.getElementById("previewBox").boxObject;
>+    var width  = bo.width;
>+    var height = bo.height;
>+
>+    var canvas = document.getElementById("itemThumbnail");
>+    var ctx = canvas.getContext('2d');
>+    var notAvailableText = canvas.getAttribute("notavailabletext");
>+    ctx.save();
>+    ctx.fillStyle = "-moz-field";
>+    ctx.fillRect(0, 0, width, height);
>+    ctx.translate(width/2, height/2);
>+
>+    ctx.fillStyle = "GrayText";
>+    ctx.mozTextStyle = "14pt sans serif";
>+    var len = ctx.mozMeasureText(notAvailableText);
>+    ctx.translate(-len/2,0);
>+    ctx.mozDrawText(notAvailableText);
>+    ctx.restore();
>+  },

hm, it'd be good to just not show anything if there's no preview available (if there's a way to aesthetically do so).

>+            <deck id="infoDeck" selectedIndex="0" height="90" persist="height">
>+              <vbox flex="1" align="center">
>+                <hbox flex="1" align="center">
>+                  <label value="&detialPane.noInfoAvailable.label;"/>
>+                </hbox>
>+              </vbox>
>+              <hbox flex="1">
>+                <hbox id="previewBox" style="min-width: 60px; border: 1px solid;">
>+                  <html:canvas id="itemThumbnail" notavailabletext="&detialPane.noPreviewAvailable.label;"/>
>+                </hbox>
>+                <scrollbox id="infoScrollbox" minimal="true" orient="vertical" flex="1" style='overflow: auto;'>
>+                  <vbox id="editBookmarkPanelContent"/>
>+                  <hbox>
>+                    <button type="image" id="infoScrollboxExpander"
>+                            lesslabel="&detialPane.less.label;"
>+                            morelabel="&detialPane.more.label;"
>+                            label="&detialPane.more.label;"
>+                            oncommand="PlacesOrganizer.toggleAdditionalInfoFields();"/>

s/detial/detail/

ditto for all other instances here and in the dtd
Here is a mockup of what the properties pane should look like when nothing is selected: http://people.mozilla.com/~faaborg/files/granParadisoUI/places_OrganizerPropertiesPane_i1.png
Attached patch more... (obsolete) — Splinter Review
Attachment #281493 - Attachment is obsolete: true
Whiteboard: [swag:1d][places-ui] → [places-ui]
Attached patch more... (obsolete) — Splinter Review
Attachment #281634 - Attachment is obsolete: true
Attached patch v1.0 (obsolete) — Splinter Review
Attachment #281652 - Attachment is obsolete: true
Attachment #281724 - Flags: review?(dietrich)
Comment on attachment 281724 [details] [diff] [review]
v1.0

a couple of issues so far:

- the keyword field is now showing in the star popup

- livemark child items are editable

- when a non-editable item is selected, the fields are populated with the properties of the previously viewed item
Comment on attachment 281724 [details] [diff] [review]
v1.0

>   _showHideRows: function EIO__showHideRows() {
>     var isBookmark = this._itemType == Ci.nsINavBookmarksService.TYPE_BOOKMARK;
>-    this._element("nameRow").hidden = this._hiddenRows.indexOf("name") != -1;
>-    this._element("folderRow").hidden =
>+
>+    this._element("nameRow").collapsed = this._hiddenRows.indexOf("name") != -1;
>+    this._element("folderRow").collapsed =
>       this._hiddenRows.indexOf("folderPicker") != -1;
>-    this._element("tagsRow").hidden =
>-      this._hiddenRows.indexOf("tags") != -1 || !isBookmark;
>-    this._element("descriptionRow").hidden =
>-      this._hiddenRows.indexOf("description") != -1;
>-    this._element("locationRow").hidden =
>-      this._hiddenRows.indexOf("location") != -1 || !isBookmark;
>+    this._element("tagsRow").collapsed = !isBookmark ||
>+      this._hiddenRows.indexOf("tags") != -1;
>+    this._element("descriptionRow").collapsed =
>+      this._hiddenRows.indexOf("description") != -1 ||
>+      this._readOnly;
>+    this._element("keywordRow").collapsed = !isBookmark || this._readOnly;
>+      this._hiddenRows.indexOf("keyword") != -1;

here's the keyword issue, s/;/||/

also, is there any case in which _readOnly is true, and some fields would *not* be disabled?

>+    else {
>+      this._readOnly = false;
>+      this._isLivemark = PlacesUtils.livemarks.isLivemark(this._itemId);
>+      if (this._isLivemark) {
>+        var feedURI = PlacesUtils.livemarks.getFeedURI(this._itemId);
>+        var siteURI = PlacesUtils.livemarks.getSiteURI(this._itemId);
>+        this._initTextField("feedLocationField", feedURI.spec);
>+        this._initTextField("siteLocationField", siteURI ? siteURI.spec : "");
>+      }
>+      this._uri = null;

isn't _uri already initialized to null?

>+  onFeedLocationFieldBlur: function EIO_onFeedLocationFieldBlur() {
>+    // XXXmano: uri fixup

please file followup bugs for this and the other instance.

>+    <broadcaster id="paneElementsBroadcaster"/>
>+
>     <grid id="editBookmarkPanelGrid" flex="1">
>       <columns>
>         <column/>
>         <column flex="1"/>
>       </columns>
>       <rows>
>         <row align="center" id="editBMPanel_nameRow">
>           <label value="&editBookmarkOverlay.name.label;"
>-                 contorl="editBMPanel_namePicker"/>
>+                 contorl="editBMPanel_namePicker"
>+                 observes="paneElementsBroadcaster"/>

s/contorl/control/

there's another instance of this for editBMPanel_locationField.

>-          </tree>
>+#include advancedSearch.inc
>+          <vbox flex="1">
>+            <tree id="placeContent" class="placesTree" context="placesContext" 
>+                  flex="1" type="places" 
>+                  ondblclick="this.controller.openSelectedNodeWithEvent(event);"
>+                  onclick="PlacesOrganizer.onTreeClick(event);"
>+                  onselect="PlacesOrganizer.onContentTreeSelect();">>

s/>>/>/

first pass, will do another tonight.
(In reply to comment #24)

> also, is there any case in which _readOnly is true, and some fields would *not*
> be disabled?
>

the tags field, for livemark children
 
> >+    else {
> >+      this._readOnly = false;
> >+      this._isLivemark = PlacesUtils.livemarks.isLivemark(this._itemId);
> >+      if (this._isLivemark) {
> >+        var feedURI = PlacesUtils.livemarks.getFeedURI(this._itemId);
> >+        var siteURI = PlacesUtils.livemarks.getSiteURI(this._itemId);
> >+        this._initTextField("feedLocationField", feedURI.spec);
> >+        this._initTextField("siteLocationField", siteURI ? siteURI.spec : "");
> >+      }
> >+      this._uri = null;
> 
> isn't _uri already initialized to null?

in-between items (say you've selected a bookmark, then a folder). 

> - livemark child items are editable

what fields, for me anything but tags is disabled.
> - when a non-editable item is selected, the fields are populated with the
properties of the previously viewed item

let's follow up on this, I'm not yet sure what's the correct behavior.
Comment on attachment 281724 [details] [diff] [review]
v1.0

>+  _updateThumbnail: function PO__updateThumbnail() {
>+    var bo = document.getElementById("previewBox").boxObject;
>+    var width  = bo.width;
>+    var height = bo.height;
>+
>+    var canvas = document.getElementById("itemThumbnail");
>+    var ctx = canvas.getContext('2d');
>+    var notAvailableText = canvas.getAttribute("notavailabletext");
>+    ctx.save();
>+    ctx.fillStyle = "-moz-Dialog";
>+    ctx.fillRect(0, 0, width, height);
>+    ctx.translate(width/2, height/2);
>+
>+    ctx.fillStyle = "GrayText";
>+    ctx.mozTextStyle = "12pt sans serif";
>+    var len = ctx.mozMeasureText(notAvailableText);
>+    ctx.translate(-len/2,0);
>+    ctx.mozDrawText(notAvailableText);
>+    ctx.restore();
>+  },

should we cache the "not available" image somehow? i don't know enough about the memory demands and speed of canvas to know if it would be worth it or not.

>     // Add the place: uri as a bookmark under the places root.
>-    var txn = PlacesUtils.ptm.createItem(placeURI, PlacesUtils.bookmarks.bookmarksRoot, PlacesUtils.bookmarks.DEFAULT_INDEX, input.value);

can you also fix the comment while you're there:

s/places root/bookmarks root/

>+        // Invalid input can cause newURI to barf, that's OK, tack "http://"
>         // onto the front and try again to see if the user omitted it
>         try {
>-          query.uri = ios.newURI("http://" + spec, null, null);
>+          query.uri = IO.newURI("http://");

need to append spec

> 
>+      <!--
>       <toolbarbutton id="viewMenu" type="menu"

why are you hiding the view menu?
Attachment #281724 - Flags: review?(dietrich) → review-
Attached patch v1.0.1 (obsolete) — Splinter Review
Attachment #281724 - Attachment is obsolete: true
Attachment #281875 - Flags: review?(dietrich)
Attached patch v1.0.2 (obsolete) — Splinter Review
Attachment #281876 - Flags: review?(dietrich)
Attachment #281875 - Attachment is obsolete: true
Attachment #281875 - Flags: review?(dietrich)
Comment on attachment 281876 [details] [diff] [review]
v1.0.2


>Index: toolkit/content/widgets/menulist.xml
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/content/widgets/menulist.xml,v
>retrieving revision 1.29
>diff -u -p -8 -r1.29 menulist.xml
>--- toolkit/content/widgets/menulist.xml	23 May 2007 20:13:22 -0000	1.29
>+++ toolkit/content/widgets/menulist.xml	21 Sep 2007 21:28:34 -0000

these changes look ok to me, but you should have a toolkit peer do a 2nd review.

>-                 contorl="editBMPanel_namePicker"/>
>+                 contorl="editBMPanel_namePicker"
>+                 observes="paneElementsBroadcaster"/>

fixed one, but the other is still there

>-          -->
>           <menu id="viewColumns" 
>                 label="&view.columns.label;" accesskey="&view.columns.accesskey;">
>             <menupopup onpopupshowing="ViewMenu.fillWithColumns(event, null, null, 'checkbox', null);"
>                        oncommand="ViewMenu.showHideColumn(event.target); event.stopPropagation();"/>
>           </menu>
>+-->

why are you hiding the column selector?

are you doing the winstripe css in a followup?

r=me otherwise for these changes.
Attachment #281876 - Flags: review?(dietrich) → review+
Comment on attachment 281876 [details] [diff] [review]
v1.0.2

r=me on the menulist.xml changes.
Attachment #281876 - Flags: review+
> are you doing the winstripe css in a followup?

very likely!
Attached patch for checkin (obsolete) — Splinter Review
Attachment #281876 - Attachment is obsolete: true
Flags: blocking-firefox3?
Attachment #281890 - Flags: approval1.9? → approval1.9+
mozilla/browser/base/content/browser-places.js 1.53
mozilla/browser/components/places/content/editBookmarkOverlay.js 1.6
mozilla/browser/components/places/content/editBookmarkOverlay.xul 1.5
mozilla/browser/components/places/content/organizer.css 1.6
mozilla/browser/components/places/content/places.css 1.11
mozilla/browser/components/places/content/places.js 1.104
mozilla/browser/components/places/content/places.xul 1.85
mozilla/browser/components/places/content/utils.js 1.68
mozilla/browser/locales/en-US/chrome/browser/places/editBookmarkOverlay.dtd 1.4
mozilla/browser/locales/en-US/chrome/browser/places/places.dtd 1.34
mozilla/browser/locales/en-US/chrome/browser/places/places.properties 1.29
mozilla/browser/themes/pinstripe/browser/jar.mn 1.57
mozilla/browser/themes/pinstripe/browser/places/editBookmarkOverlay.css 1.2
mozilla/browser/themes/pinstripe/browser/places/places.css 1.14
mozilla/browser/themes/pinstripe/browser/places/twisty-closed.gif initial revision: 1.1
mozilla/browser/themes/pinstripe/browser/places/twisty-open.gif initial revision: 1.1
mozilla/toolkit/content/widgets/menulist.xml 1.31
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 397183
Depends on: 397184
No longer depends on: 397184
Flags: blocking-firefox3?
Whiteboard: [places-ui] → [places-ui][wanted-firefox3]
verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b2pre) Gecko/2007112604 Minefield/3.0b2pre
Status: RESOLVED → VERIFIED
Flags: wanted-firefox3+
Whiteboard: [places-ui][wanted-firefox3] → [places-ui]
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.