Closed Bug 467670 Opened 16 years ago Closed 15 years ago

Bookmark Management

Categories

(Firefox for Android Graveyard :: Bookmarks, defect)

Other
All
defect
Not set
normal

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+
Depends on: 465293
Depends on: 465290
Blocks: 465294
Blocks: 456482
No longer blocks: 456482, 465294
Depends on: 465291, 456482, 465294
Depends on: 465298
Target Milestone: --- → Fennec A3
Depends on: 465292
Blocks: 477628
Depends on: 464769, 454668
tracking-fennec: --- → 1.0b1+
Flags: blocking-fennec1.0+
Assignee: nobody → mark.finkle
Attached patch WIP 1: first cut, still rough (obsolete) — Splinter Review
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
Attached image bookmark list
The basic bookmark list - note we are not themeing anything yet
Note how the "labels" appear to the left. Do we need that?
Attached image editing a folder
note we have no labels on left
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.
Attached patch WIP 2: Close to finished (obsolete) — Splinter Review
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
Attached image picking a folder
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.
Depends on: 458741
Attached image the manage button
when not managing, no edit buttons are in the rows
Attached image managing
when managing, the edit buttons appear
Attached image editing while managing
editing a folder while in manage mode (note no label on left of textbox)
shows a bookmark in manage mode
note no labels and the use of "empty text" for tags
Attached patch ready for reviewSplinter Review
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)
With new icon and style tweaks
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)
Summary: Bookmark Managment → Bookmark Management
Attachment #363134 - Flags: review?(gavin.sharp) → review+
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 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+
(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
(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
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.
One other icon we should have is a depressed and normal version of the "manage" button when viewing the bookmarks pane.
Blocks: 481289
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
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
Component: General → Bookmarks
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: