Closed
Bug 387749
Opened 18 years ago
Closed 17 years ago
add an item detail pane to the organizer
Categories
(Firefox :: Bookmarks & History, defect, P2)
Firefox
Bookmarks & History
Tracking
()
VERIFIED
FIXED
Firefox 3 beta1
People
(Reporter: dietrich, Assigned: asaf)
References
()
Details
(Whiteboard: [places-ui])
Attachments
(1 file, 12 obsolete files)
(deleted),
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
Updated•18 years ago
|
Assignee: nobody → swon
Updated•18 years ago
|
Status: NEW → ASSIGNED
Comment 1•18 years ago
|
||
>* 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).
Comment 2•18 years ago
|
||
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?
Comment 3•18 years ago
|
||
Checkpoint 1.
Comment 4•17 years ago
|
||
Basic functionality's almost all there in the patch.
But need 387485 to finish the tagging field.
Target Milestone: --- → Firefox 3 M8
Updated•17 years ago
|
Whiteboard: Waiting on Tag Editor; Need code clean up, but most of the basic funcationality is in the checkpt.
Updated•17 years ago
|
Whiteboard: Waiting on Tag Editor; Need code clean up, but most of the basic funcationality is in the checkpt. → [swag:1d][places-ui]
Comment 5•17 years ago
|
||
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)
Reporter | ||
Comment 6•17 years ago
|
||
Assignee | ||
Comment 7•17 years ago
|
||
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-
Reporter | ||
Comment 8•17 years ago
|
||
(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.
Comment 9•17 years ago
|
||
>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.
Updated•17 years ago
|
Assignee: stevewon → nobody
Status: ASSIGNED → NEW
Comment 10•17 years ago
|
||
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 | ||
Updated•17 years ago
|
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
Assignee | ||
Comment 11•17 years ago
|
||
Attachment #275541 -
Attachment is obsolete: true
Attachment #275558 -
Attachment is obsolete: true
Comment 12•17 years ago
|
||
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)
Assignee | ||
Comment 13•17 years ago
|
||
Seth, you need the patch in bug 385266 for this to work.
Comment 14•17 years ago
|
||
oops, thanks asaf.
See bug #387750 for a patch (and screen shot) that uses part of your patch here for image preview.
Comment 15•17 years ago
|
||
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")
Assignee | ||
Updated•17 years ago
|
Assignee | ||
Comment 16•17 years ago
|
||
Attachment #277318 -
Attachment is obsolete: true
Reporter | ||
Updated•17 years ago
|
Target Milestone: Firefox 3 M8 → Firefox 3 M9
Assignee | ||
Updated•17 years ago
|
Assignee | ||
Comment 17•17 years ago
|
||
Attachment #278996 -
Attachment is obsolete: true
Reporter | ||
Comment 18•17 years ago
|
||
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
Comment 19•17 years ago
|
||
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
Assignee | ||
Comment 20•17 years ago
|
||
Attachment #281493 -
Attachment is obsolete: true
Assignee | ||
Updated•17 years ago
|
Whiteboard: [swag:1d][places-ui] → [places-ui]
Assignee | ||
Comment 21•17 years ago
|
||
Attachment #281634 -
Attachment is obsolete: true
Assignee | ||
Comment 22•17 years ago
|
||
Attachment #281652 -
Attachment is obsolete: true
Attachment #281724 -
Flags: review?(dietrich)
Reporter | ||
Comment 23•17 years ago
|
||
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
Reporter | ||
Comment 24•17 years ago
|
||
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.
Assignee | ||
Comment 25•17 years ago
|
||
(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).
Assignee | ||
Comment 26•17 years ago
|
||
> - livemark child items are editable
what fields, for me anything but tags is disabled.
Assignee | ||
Comment 27•17 years ago
|
||
> - 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.
Reporter | ||
Comment 28•17 years ago
|
||
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-
Assignee | ||
Comment 29•17 years ago
|
||
Attachment #281724 -
Attachment is obsolete: true
Attachment #281875 -
Flags: review?(dietrich)
Assignee | ||
Comment 30•17 years ago
|
||
Attachment #281876 -
Flags: review?(dietrich)
Assignee | ||
Updated•17 years ago
|
Attachment #281875 -
Attachment is obsolete: true
Attachment #281875 -
Flags: review?(dietrich)
Reporter | ||
Comment 31•17 years ago
|
||
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 32•17 years ago
|
||
Comment on attachment 281876 [details] [diff] [review]
v1.0.2
r=me on the menulist.xml changes.
Attachment #281876 -
Flags: review+
Assignee | ||
Comment 33•17 years ago
|
||
> are you doing the winstripe css in a followup?
very likely!
Assignee | ||
Comment 34•17 years ago
|
||
Attachment #281876 -
Attachment is obsolete: true
Assignee | ||
Comment 35•17 years ago
|
||
Attachment #281889 -
Attachment is obsolete: true
Assignee | ||
Updated•17 years ago
|
Flags: blocking-firefox3?
Assignee | ||
Updated•17 years ago
|
Attachment #281890 -
Flags: approval1.9?
Updated•17 years ago
|
Attachment #281890 -
Flags: approval1.9? → approval1.9+
Assignee | ||
Comment 36•17 years ago
|
||
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: 17 years ago
Resolution: --- → FIXED
Updated•17 years ago
|
Flags: blocking-firefox3?
Whiteboard: [places-ui] → [places-ui][wanted-firefox3]
Comment 37•17 years ago
|
||
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
Updated•17 years ago
|
Flags: wanted-firefox3+
Whiteboard: [places-ui][wanted-firefox3] → [places-ui]
Comment 38•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
•