Closed
Bug 467670
Opened 16 years ago
Closed 15 years ago
Bookmark Management
Categories
(Firefox for Android Graveyard :: Bookmarks, defect)
Tracking
(fennec1.0b1+)
VERIFIED
FIXED
fennec1.0b1
Tracking | Status | |
---|---|---|
fennec | 1.0b1+ | --- |
People
(Reporter: pavlov, Assigned: mfinkle)
References
Details
(Keywords: meta)
Attachments
(14 files, 2 obsolete files)
12.70 KB,
image/png
|
Details | |
15.30 KB,
image/png
|
Details | |
13.98 KB,
image/png
|
Details | |
10.72 KB,
image/png
|
Details | |
11.15 KB,
image/png
|
Details | |
15.53 KB,
image/png
|
Details | |
15.59 KB,
image/png
|
Details | |
17.10 KB,
image/png
|
Details | |
18.47 KB,
image/png
|
Details | |
411 bytes,
image/png
|
Details | |
57.25 KB,
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
18.27 KB,
image/png
|
Details | |
10.15 KB,
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
1.04 KB,
image/png
|
Details |
Meta tracking bug for all of bookmark management in Fennec. adding/editing/deleting/reordering/folders/etc
Flags: blocking-fennec1.0+
Reporter | ||
Updated•16 years ago
|
Reporter | ||
Updated•16 years ago
|
Target Milestone: --- → Fennec A3
Updated•15 years ago
|
Reporter | ||
Updated•15 years ago
|
tracking-fennec: --- → 1.0b1+
Flags: blocking-fennec1.0+
Reporter | ||
Updated•15 years ago
|
Assignee: nobody → mark.finkle
Assignee | ||
Comment 1•15 years ago
|
||
This patch refactors the current bookmarking code in Fennec and adds support for folder management. Some points: * support for adding folders * support for navigating thru folders in the bookmark list * support for editing folder name * support for deleting a folder * refactor bookmarking code into XBL widgets * refactor any non-XBL code (UI management) into small helper JS objects Still needed: * removed more code from BrowserUI * add support for moving bookmarks and folders to a different parent folder
Assignee | ||
Comment 2•15 years ago
|
||
The basic bookmark list - note we are not themeing anything yet
Assignee | ||
Comment 3•15 years ago
|
||
Note how the "labels" appear to the left. Do we need that?
Assignee | ||
Comment 4•15 years ago
|
||
note we have no labels on left
Assignee | ||
Comment 5•15 years ago
|
||
One last comment on the snapshots. The bugzilla bookmark and the subfolder are in a folder: "Bookmarks Menu/My Stuff" You can see these parent folders above the blue line. You can click on those folders to go back "up" the folder tree.
Assignee | ||
Comment 6•15 years ago
|
||
This patch adds support for: 1 Moving a bookmark or folder to another folder 2 Removes unused code from BrowserUI 3 Adds a Folder Picker screen for choosing a new folder location (used by #1) This patch does not style the screens well and it still uses <button> where we may want to use images. I will work with Madhava on that part. I think this is functionally ready. A few more tests, then I'll ask for a functional review.
Attachment #362216 -
Attachment is obsolete: true
Assignee | ||
Comment 7•15 years ago
|
||
The folder picking screen. You see this screen if you want to move a bookmark or folder to another parent folder. Click/touch the folder and you're done. Click/touch button in top/right to cancel.
Assignee | ||
Comment 8•15 years ago
|
||
when not managing, no edit buttons are in the rows
Assignee | ||
Comment 9•15 years ago
|
||
when managing, the edit buttons appear
Assignee | ||
Comment 10•15 years ago
|
||
editing a folder while in manage mode (note no label on left of textbox)
Assignee | ||
Comment 11•15 years ago
|
||
shows a bookmark in manage mode
Assignee | ||
Comment 12•15 years ago
|
||
note no labels and the use of "empty text" for tags
Comment 13•15 years ago
|
||
Assignee | ||
Comment 14•15 years ago
|
||
This patch builds on the WIP2 patch. It has some changes based on IRC discussion with Madhava: * Adds the "Manage" button so editing buttons on each row can be shown/hidden. This makes for a cleaner default look. * Removed labels - we may add them back later * Added folder icon * minor theme/style tweaks
Attachment #362846 -
Attachment is obsolete: true
Attachment #363134 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 15•15 years ago
|
||
With new icon and style tweaks
Assignee | ||
Comment 16•15 years ago
|
||
The main patch breaks editing a bookmark via the star button. This patch fixes that functionality. Yeah, needs to be applied on top of the main patch. * Fixes editing via star button * Removes UIMODE_BOOKMARK and related code
Attachment #363379 -
Flags: review?(gavin.sharp)
Updated•15 years ago
|
Summary: Bookmark Managment → Bookmark Management
Comment 17•15 years ago
|
||
Updated•15 years ago
|
Attachment #363134 -
Flags: review?(gavin.sharp) → review+
Comment 18•15 years ago
|
||
Comment on attachment 363134 [details] [diff] [review] ready for review place-base: -mixing properties that only return the initial value of the attribute on construction (e.g. itemId) with properties that are "live" (e.g. type), but I suppose you did that for perf reasons? -this._uri (nsIURI) vs. this.uri (string) distinction is confusing. use .spec for the latter, perhaps? -seem to get "control" property often (e.g. in selectFolder), worth caching it? -does selectFolder need to support an "onfolder" on |this|? no one seems to be using it, seems simpler to omit it for now. -"onfolder" really looks like it should be "onmove"? place-list: -_isEditing/activeItem stuff is kind of confusing, and _ignoreEditing seems hacky, perhaps we can refactor that at some point? -get rid of the Components.reportError in manageUI getter? place-tree: -_getChildren is duplicated from place-list, any chance we could share that? -where does the magic number "24" come from in openFolder? BookmarkList: -use declarative handler for close() rather than addEventListener? FolderHelper: -seems like it should be more specific to moving: "onopen" -> "onselect", selectFolder -> "moveItem"? CSS: +#tool-bookmarks-manage .toolbarbutton-text { + display: block !important; +} why's this needed? I didn't review the CSS too closely...
Comment 19•15 years ago
|
||
Comment on attachment 363379 [details] [diff] [review] extra patch for editing via star button >diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js > selectFolder: function() { >+ if (this._control.removeItem) >+ this._control.removeItem(this._control.activeItem); Why wouldn't there be a removeItem?
Attachment #363379 -
Flags: review?(gavin.sharp) → review+
Assignee | ||
Comment 20•15 years ago
|
||
(In reply to comment #18) > (From update of attachment 363134 [details] [diff] [review]) > place-base: > -mixing properties that only return the initial value of the attribute on > construction (e.g. itemId) with properties that are "live" (e.g. type), but I > suppose you did that for perf reasons? No, just sloppy. "itemId" can't change and neither should "type". At one point I was setting "type" via the property. "itemId" is set via an attr when used as part of the bookmark list. But it's not set via the attr when used as the bookmark editor form, since that element is pre-created the <constructor> would not be fired for it. We pass it in via the elem.init(itemId) (it could use some cleanup) > -this._uri (nsIURI) vs. this.uri (string) distinction is confusing. use .spec > for the latter, perhaps? done > -seem to get "control" property often (e.g. in selectFolder), worth caching it? > -does selectFolder need to support an "onfolder" on |this|? no one seems to be > using it, seems simpler to omit it for now. Yes, it does. For the bookmark editor form (not the bookmark list) > -"onfolder" really looks like it should be "onmove"? Done > > place-list: > -_isEditing/activeItem stuff is kind of confusing, and _ignoreEditing seems > hacky, perhaps we can refactor that at some point? It is a bit hacky. The bookmark row has 2 modes: it can be active (remove and move) or it can be editing (edit) > -get rid of the Components.reportError in manageUI getter? Done > > place-tree: > -_getChildren is duplicated from place-list, any chance we could share that? I'll file a refactor bug for this and the other things > -where does the magic number "24" come from in openFolder? The favicon image size. So we indent by the favicon amount. I'll include this in the refactor bug. > > BookmarkList: > -use declarative handler for close() rather than addEventListener? close() is called either from the window.addEventListener or the button (top/right) in the bookmark list. Not sure I understand the declarative way to do this. > > FolderHelper: > -seems like it should be more specific to moving: "onopen" -> "onselect", > selectFolder -> "moveItem"? Done > > CSS: > +#tool-bookmarks-manage .toolbarbutton-text { > + display: block !important; > +} > > why's this needed? the "Manage" button needs to display text. All other toolbar buttons have their text hidden
Assignee | ||
Comment 21•15 years ago
|
||
(In reply to comment #19) > (From update of attachment 363379 [details] [diff] [review]) > >diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js > > > selectFolder: function() { > > >+ if (this._control.removeItem) > >+ this._control.removeItem(this._control.activeItem); > > Why wouldn't there be a removeItem? If used from the bookmark form
Assignee | ||
Comment 22•15 years ago
|
||
http://hg.mozilla.org/mobile-browser/rev/a3a6bfcdcb33 http://hg.mozilla.org/mobile-browser/rev/45a459ac4c3f
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment 23•15 years ago
|
||
one minor icon adjustment is when viewing the "folder" icon next to "Bookmarks Menu", it has a white background and the "Bookmarks Menu" has a blue background.
Comment 24•15 years ago
|
||
One other icon we should have is a depressed and normal version of the "manage" button when viewing the bookmarks pane.
Comment 25•15 years ago
|
||
verified FIXED On builds: Mozilla/5.0 (X11; U; Linux armv6l; en-US; rv:1.9.2b1pre) Gecko/20091002 Fennec/1.0b4 and Mozilla/5.0 (X11; U; Linux armv7l; en-US; rv:1.9.2b1pre) Gecko/200910002 Fennec/1.0b4
Status: RESOLVED → VERIFIED
Comment 26•15 years ago
|
||
we have redone this a couple times since this bug. Also other bugs exist for specific issues. Verified with 20091001 1.9.2 beta4 candidate on my n810
Updated•14 years ago
|
Component: General → Bookmarks
You need to log in
before you can comment on or make changes to this bug.
Description
•