Closed Bug 722020 Opened 10 years ago Closed 10 years ago

Fennec Native doesn't show bookmarks in folders, or in desktop sequence

Categories

(Firefox for Android Graveyard :: General, defect, P2)

ARM
Android
defect

Tracking

(firefox13 verified, blocking-fennec1.0 beta+, fennec+)

VERIFIED FIXED
Firefox 13
Tracking Status
firefox13 --- verified
blocking-fennec1.0 --- beta+
fennec + ---

People

(Reporter: jan.manthay, Assigned: Margaret)

References

Details

(Keywords: uiwanted)

Attachments

(5 files, 12 obsolete files)

4.96 KB, application/zip
Details
63.44 KB, image/png
Details
93.49 KB, image/png
Details
4.25 KB, patch
wesj
: review+
Details | Diff | Splinter Review
39.71 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
What happens:
After activate Sync in Aurora 11, the synced bookmarks are out of order, and no folders were created, leaving all the bookmarks in a single, long list.

What should happen:
The display of bookmarks should lock exact like on the desktop and "pre-native" releases, meaning in the exact same order, an of course with folders.
Hello Jan. Thanks for filing a bug. Support for that functionality is currently not available but it sure is possible to see in a future version of Firefox. I'll let :rnewman add any additional scheduling or info about that or dupe this.
Whiteboard: sync
Hi Jan,

Sync is creating folders, but Fennec displays all of your bookmarks in one alphabetized list. See Bug 716918 for a start at splitting these into "Mobile Bookmarks" and "Desktop Bookmarks".

I'm not going to dupe this bug to that one, because it's strictly larger in scope. Rest assured that it's a known issue.
Status: UNCONFIRMED → NEW
Depends on: 716918
Ever confirmed: true
Summary: Sync on native Android UI is not creating folders → Fennec Native doesn't show bookmarks in folders, or in desktop sequence
Whiteboard: sync
tracking-fennec: --- → +
Priority: -- → P2
Are we going to want to make a UI like what we had in XUL fennec, or do we want to take this opportunity to update our bookmarks UI?
Assignee: nobody → margaret.leibovic
Keywords: uiwanted
(In reply to Margaret Leibovic [:margaret] from comment #3)
> Are we going to want to make a UI like what we had in XUL fennec, or do we
> want to take this opportunity to update our bookmarks UI?

We could take a look at Chrome's bookmark handling. It's more complex than we initially want, but the folder navigation is similar to what we do in XUL. One big difference is the folders are shown in a horizontal scrolling breadcrumb model across the top of the list. The list content is like our XUL concept where only folders and bookmarks for a given level are displayed.
Also, Chrome uses an Icon view where we use a List view
Attached patch WIP (obsolete) — Splinter Review
Since the patches to fix up the folder structure in the DB haven't landed yet, I just wrote this patch assuming the mobile folder is the root. Also, since the menu/toolbar/unfiled folders aren't children of the places folder like they should be (rnewman said he has a fix for this), you just have to make various folders/bookmarks under the mobile bookmarks folder to test this (rnewman, perhaps you could try this patch out with your fix to see what happens).

There are still a few TODO items in here, and it needs some UI polish, but it's a working start!
Attachment #596347 - Flags: review?(rnewman)
Attachment #596347 - Flags: review?(lucasr.at.mozilla)
Attachment #596347 - Attachment is patch: true
Attached image screenshot of root folder view (obsolete) —
Right now this is just all the bookmarks in the mobile bookmarks folder. I gave the folder with the places guid the "Desktop Bookmarks" title. In the future, when the folder parent business gets sorted out, we may have to use a MatrixCursor to get the folders/bookmarks we want on this screen.
Attached image screenshot of sub-folder view (obsolete) —
This is what happens after you tap on the "Foo" folder. Tapping the header at the top of this list will take you back up a level like in XUL fennec (we need to add some sort of indication that you can do this).
Comment on attachment 596347 [details] [diff] [review]
WIP

Oops, I meant feedback!
Attachment #596347 - Flags: review?(rnewman)
Attachment #596347 - Flags: review?(lucasr.at.mozilla)
Attachment #596347 - Flags: feedback?(rnewman)
Attachment #596347 - Flags: feedback?(lucasr.at.mozilla)
Attached patch WIP (obsolete) — Splinter Review
Geez, I'm not good at this today - forgot to hg add new files.
Attachment #596347 - Attachment is obsolete: true
Attachment #596347 - Flags: feedback?(rnewman)
Attachment #596347 - Flags: feedback?(lucasr.at.mozilla)
Attachment #596350 - Flags: feedback?(rnewman)
Attachment #596350 - Flags: feedback?(lucasr.at.mozilla)
Comment on attachment 596350 [details] [diff] [review]
WIP

Review of attachment 596350 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good!

::: mobile/android/base/AwesomeBar.java
@@ +446,5 @@
> +                return;
> +            }
> +
> +            Cursor cursor = (Cursor) selectedItem;
> +            title = cursor.getString(cursor.getColumnIndexOrThrow(URLColumns.TITLE));

Why is it necessary to move this code? If it's just an unrelated code cleanup, do it in a separate patch.

::: mobile/android/base/AwesomeBarTabs.java
@@ +171,1 @@
>              if (TextUtils.isEmpty(title)) {

Unrelated cleanup. Should go on a separate patch.

@@ +202,5 @@
>  
> +        LayoutInflater mInflater;
> +
> +        // Keeps track of parent folder ids
> +        public LinkedList<Integer> parentStack = new LinkedList<Integer>();

You're accessing/changing this list from multiple threads and LinkedList is not thread-safe. You should either create the list like this Collections.synchronizedList(new LinkedList(...)) or enclose all accesses to it with synchronized.

@@ +234,5 @@
> +            String title = c.getString(c.getColumnIndexOrThrow(Bookmarks.TITLE));
> +            if (TextUtils.isEmpty(title)) {
> +                int guidIndex = c.getColumnIndexOrThrow(Bookmarks.GUID);
> +                if (c.getString(guidIndex).equals(Bookmarks.PLACES_FOLDER_GUID))
> +                    title = mContext.getResources().getString(R.string.bookmarks_folder_desktop);

Maybe you won't need to do that after I land my patch that ensures all special folder have titles properly set?

@@ +253,3 @@
>  
> +            TextView titleView = (TextView) convertView.findViewById(R.id.title);
> +            updateFolderTitle(position, titleView);

Maybe the folder view should show the description? Check with UX team.

@@ +260,5 @@
>  
>      private class BookmarksQueryTask extends AsyncTask<Void, Void, Cursor> {
>          protected Cursor doInBackground(Void... arg0) {
> +            ContentResolver cr = mContext.getContentResolver();
> +            return BrowserDB.getBookmarksInRootFolder(cr);

See my previous comment. You'll be able to simply use the FIXED_ROOT_ID here. Just need to land my bookmark migration patch.

@@ +275,5 @@
>                                 URLColumns.FAVICON },
>                  new int[] { R.id.title, R.id.url, R.id.favicon }
>              );
>  
> +            // TODO: Come up with a more robust way to get the root id

I added a FIXED_ROOT_ID constant to Bookmarks for that. Or are you assuming id = 1 for mobile bookmarks here?

@@ +295,5 @@
> +            View headerView = inflater.inflate(R.layout.awesomebar_folder_header_row, null);
> +
> +            // Hide the header view to begin with
> +            mBookmarksTitleView = (TextView) headerView.findViewById(R.id.title);
> +            mBookmarksTitleView.setVisibility(View.GONE);

Maybe the header should look like it's part of the list but with a "up" icon? Need to check with the UX crew.

@@ +324,4 @@
>          }
> +
> +        protected void onPostExecute(Cursor cursor) {
> +            mBookmarksAdapter.changeCursor(cursor);

Need to cover the case where the current folder changes before the AsyncTask finishes. Hold the target folderId in the AsyncTask and check in onPostExecute() if the parentStack still points to it. Ideally, you'd cancel any ongoing task when that happens.

@@ +331,5 @@
> +    private void handleBookmarkItemClick(int position) {
> +        Cursor cursor = mBookmarksAdapter.getCursor();
> +
> +        // If we tap on the header view, go up a level
> +        if (position == 0) {

This handleBookmarkItemClick method is getting a bit too chunky. Factor out this code into a separate "goToParent()" private method?

@@ +338,5 @@
> +            int parentId = mBookmarksAdapter.parentStack.getFirst();
> +
> +            if (mBookmarksAdapter.parentStack.size() == 1) {
> +                // Hide the header text if we're back at the root folder
> +                mBookmarksTitleView.setText("");

Is setting text necessary here? You're hiding it anyway.

@@ +343,5 @@
> +                mBookmarksTitleView.setVisibility(View.GONE);
> +            } else {
> +                // Otherwise, update the text to use the new folder title
> +                // TODO: Figure out how to do this
> +                mBookmarksTitleView.setText("We went up a folder");

Isn't the title in the cursor?

@@ +354,5 @@
> +        // The header view takes up a spot in the list
> +        cursor.moveToPosition(position - 1);
> +
> +        int isFolder = cursor.getInt(cursor.getColumnIndexOrThrow(Bookmarks.IS_FOLDER));
> +        if (isFolder == 1) {

Factor out this code into a separate "openFolder" private method?

@@ +830,5 @@
>                  (InputMethodManager) mContext.getSystemService(Context.INPUT_METHOD_SERVICE);
>  
>          imm.hideSoftInputFromWindow(view.getWindowToken(), 0);
>      }
>  

Any special reason for moving the handleBookmarkItemClick() method up? If you want to move it, do it in a separate patch.

::: mobile/android/base/db/BrowserDB.java
@@ +82,2 @@
>  
> +        public Cursor getBookmarksInRootFolder(ContentResolver cr);

This separate method won't be necessary once I land my patch for bug 723841. You'll be able to simply do getBookmarksInFolder(cr, Bookmarks.FIXED_ROOT_ID) to get the root folder contents.

@@ +148,5 @@
>      public static Cursor getAllBookmarks(ContentResolver cr) {
>          return sDb.getAllBookmarks(cr);
>      }
>  
> +    public static Cursor getBookmarksInFolder(ContentResolver cr, long id) {

Rename the id argument to folderId for clarity.
Attachment #596350 - Flags: feedback?(lucasr.at.mozilla) → feedback+
(In reply to Lucas Rocha (:lucasr) from comment #11)

Thanks for all the comments! I especially appreciate the comments about threads and async tasks, since I'm not completely familiar with everything that can go wrong with those.

> ::: mobile/android/base/AwesomeBar.java
> @@ +446,5 @@
> > +                return;
> > +            }
> > +
> > +            Cursor cursor = (Cursor) selectedItem;
> > +            title = cursor.getString(cursor.getColumnIndexOrThrow(URLColumns.TITLE));
> 
> Why is it necessary to move this code? If it's just an unrelated code
> cleanup, do it in a separate patch.

I can do this in a separate patch, but we need to change the logic here to undo the changes from bug 725171.

> @@ +234,5 @@
> > +            String title = c.getString(c.getColumnIndexOrThrow(Bookmarks.TITLE));
> > +            if (TextUtils.isEmpty(title)) {
> > +                int guidIndex = c.getColumnIndexOrThrow(Bookmarks.GUID);
> > +                if (c.getString(guidIndex).equals(Bookmarks.PLACES_FOLDER_GUID))
> > +                    title = mContext.getResources().getString(R.string.bookmarks_folder_desktop);
> 
> Maybe you won't need to do that after I land my patch that ensures all
> special folder have titles properly set?

Does that include the "places" folder? However, if that becomes the root, we won't actually need to use its title.

> @@ +253,3 @@
> >  
> > +            TextView titleView = (TextView) convertView.findViewById(R.id.title);
> > +            updateFolderTitle(position, titleView);
> 
> Maybe the folder view should show the description? Check with UX team.

What description are you referring to?

> @@ +260,5 @@
> >  
> >      private class BookmarksQueryTask extends AsyncTask<Void, Void, Cursor> {
> >          protected Cursor doInBackground(Void... arg0) {
> > +            ContentResolver cr = mContext.getContentResolver();
> > +            return BrowserDB.getBookmarksInRootFolder(cr);
> 
> See my previous comment. You'll be able to simply use the FIXED_ROOT_ID
> here. Just need to land my bookmark migration patch.

Sweet, that's what I was hoping for :)

> @@ +275,5 @@
> >                                 URLColumns.FAVICON },
> >                  new int[] { R.id.title, R.id.url, R.id.favicon }
> >              );
> >  
> > +            // TODO: Come up with a more robust way to get the root id
> 
> I added a FIXED_ROOT_ID constant to Bookmarks for that. Or are you assuming
> id = 1 for mobile bookmarks here?

Yeah, I was just using 1 for the mobile folder for now. FIXED_ROOT_ID will be exactly what I want here.

> @@ +343,5 @@
> > +                mBookmarksTitleView.setVisibility(View.GONE);
> > +            } else {
> > +                // Otherwise, update the text to use the new folder title
> > +                // TODO: Figure out how to do this
> > +                mBookmarksTitleView.setText("We went up a folder");
> 
> Isn't the title in the cursor?

I was finding this part tricky. The cursor holds the titles of all the children bookmarks. If we're tapping on a folder and going into it, we have its title before we change the cursor, but if we're going back up we don't have it. I was contemplating keeping titles with ids in the parentStack, although that could cause problems if the titles change while we're in the bookmarks view. I guess that same problem affects folder structure as well, but maybe that's an edge case.
Comment on attachment 596350 [details] [diff] [review]
WIP

Review of attachment 596350 [details] [diff] [review]:
-----------------------------------------------------------------

Quick scan to follow on from lucasr's comments: looks pretty good to me.

::: mobile/android/base/AwesomeBarTabs.java
@@ +213,5 @@
>  
>          @Override
> +        public int getItemViewType(int position) {
> +            Cursor c = getCursor();
> +            c.moveToPosition(position);

This can return false to indicate that the move failed. Best to check…

@@ +228,5 @@
> +        }
> +
> +        private void updateFolderTitle(int position, TextView titleView) {
> +            Cursor c = getCursor();
> +            c.moveToPosition(position);

Check failure.

@@ +234,5 @@
> +            String title = c.getString(c.getColumnIndexOrThrow(Bookmarks.TITLE));
> +            if (TextUtils.isEmpty(title)) {
> +                int guidIndex = c.getColumnIndexOrThrow(Bookmarks.GUID);
> +                if (c.getString(guidIndex).equals(Bookmarks.PLACES_FOLDER_GUID))
> +                    title = mContext.getResources().getString(R.string.bookmarks_folder_desktop);

Best to be safe, IMO.

::: mobile/android/base/db/LocalBrowserDB.java
@@ +287,5 @@
> +                                           Bookmarks.URL,
> +                                           Bookmarks.TITLE,
> +                                           Bookmarks.IS_FOLDER,
> +                                           Bookmarks.PARENT,
> +                                           Bookmarks.FAVICON },

Lift this into a const; DEFAULT_BOOKMARK_COLUMNS. The compiler won't necessarily do enough analysis to do it for you.
Attachment #596350 - Flags: feedback?(rnewman) → feedback+
Depends on: 723841
(In reply to Richard Newman [:rnewman] from comment #13)

> ::: mobile/android/base/db/LocalBrowserDB.java
> @@ +287,5 @@
> > +                                           Bookmarks.URL,
> > +                                           Bookmarks.TITLE,
> > +                                           Bookmarks.IS_FOLDER,
> > +                                           Bookmarks.PARENT,
> > +                                           Bookmarks.FAVICON },
> 
> Lift this into a const; DEFAULT_BOOKMARK_COLUMNS. The compiler won't
> necessarily do enough analysis to do it for you.

Should we also be doing something like this for the other queries in LocalBrowserDB? If so, we should file a bug on that.
(In reply to Margaret Leibovic [:margaret] from comment #14)

> Should we also be doing something like this for the other queries in
> LocalBrowserDB? If so, we should file a bug on that.

Eeeeeeverywhere. :D
(In reply to Richard Newman [:rnewman] from comment #15)
> (In reply to Margaret Leibovic [:margaret] from comment #14)
> 
> > Should we also be doing something like this for the other queries in
> > LocalBrowserDB? If so, we should file a bug on that.
> 
> Eeeeeeverywhere. :D

Filed bug 726813.
Attached patch WIP v2 (obsolete) — Splinter Review
I'm going to meet with Madhava tomorrow to work through the UX details, so hopefully I'll be able to finish this up soon.

I decided to move more logic into BookmarksListAdapter, and I'm only letting one method create RefreshBookmarkCursorTasks, so that we can keep track of them and cancel them easily if we need to. I also stopped accessing mParentStack from multiple threads, so that should be safe now.

To test this without the folder structure cleanup patches from bug 723841 (they were bit-rotted when I tried to apply them), I've just been setting FIXED_ROOT_ID to 1, which uses the mobile folder as the root. Once those patches land I'll definitely do more testing to make sure the folders structure appears as expected.
Attachment #596350 - Attachment is obsolete: true
Attachment #596825 - Flags: feedback?(lucasr.at.mozilla)
Attached patch patch to fix context menu (obsolete) — Splinter Review
I pulled this out of my original WIP patch. This is do address the inverse issue of bug 725171.
Comment on attachment 596825 [details] [diff] [review]
WIP v2

Review of attachment 596825 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good :-)

::: mobile/android/base/AwesomeBarTabs.java
@@ +36,1 @@
>   * ***** END LICENSE BLOCK ***** */

Add your name to the contributors list on the license header? :-)

@@ +207,5 @@
> +        public BookmarksListAdapter(Context context, int layout, Cursor c, String[] from, int[] to) {
> +            super(context, layout, c, from, to);
> +
> +            mInflater = (LayoutInflater) mContext.getSystemService(Context.LAYOUT_INFLATER_SERVICE);
> +            mParentStack = new LinkedList<Pair<Integer, String>>();

Add a short comment here explaining what sort of information mParentStack holds in each pair?

@@ +226,5 @@
> +        }
> +
> +        public void moveToParentFolder() {
> +            mParentStack.pop();
> +            refreshCurrentFolder();

Much cleaner now. Nice :-)

@@ +303,5 @@
> +                return BrowserDB.getBookmarksInFolder(cr, mFolderId);
> +            }
> +
> +            protected void onPostExecute(Cursor cursor) {
> +                mBookmarksAdapter.changeCursor(cursor);

You need to set mRefreshTask to null here as you don't want to cancel an already finished task when you access a folder again.

::: mobile/android/base/Makefile.in
@@ +329,5 @@
>    res/drawable/doorhanger_popup_bg.9.png \
>    res/drawable/site_security_identified.png \
>    res/drawable/site_security_verified.png \
>    res/drawable/urlbar_stop.png \
> +  res/drawable/folder_32.png \

Need icons for mdpi, hdpi and xhdpi here. Request from UX.

::: mobile/android/base/db/LocalBrowserDB.java
@@ +54,5 @@
>  import android.graphics.BitmapFactory;
>  import android.graphics.drawable.BitmapDrawable;
>  import android.net.Uri;
>  import android.provider.Browser;
> +import android.util.Log;

Unused new import?

@@ +293,5 @@
>          Cursor c = cr.query(appendProfile(Bookmarks.CONTENT_URI),
> +                            DEFAULT_BOOKMARK_COLUMNS,
> +                            Bookmarks.PARENT + " = ?",
> +                            new String[] { String.valueOf(folderId) },
> +                            null);

We probably want to show folders before bookmarks right? Add a sort by IS_FOLDER here? Or maybe we just respect the POSITION value. Need to check with UX.
Attachment #596825 - Flags: feedback?(lucasr.at.mozilla) → feedback+
(In reply to Lucas Rocha (:lucasr) from comment #19)

> We probably want to show folders before bookmarks right? Add a sort by
> IS_FOLDER here? Or maybe we just respect the POSITION value. Need to check
> with UX.

Bear in mind that Fennec doesn't set a useful position, nor can we guarantee that they'll always be valid and sequential, so perhaps the best you could do would be to ORDER BY ABS(position).
Following guidance from UX folks, the system back button should take you back to previous folders. This is my attempt at trying to do that, but I'm finding onKeyPreIme always gets called twice, which results in us going up two levels, or going up one level and exiting the awesome screen. Lucas, you worked on this code, so I'm wondering if you know what might be causing this issue.
Attachment #597258 - Flags: feedback?(lucasr.at.mozilla)
Addressed previous comments and added new images. I also started testing this on top of the patches in bug 723841 and tried adding some logic for the case where only mobile bookmarks exist (no sync or profile migration). I'm running into some sql errors in onlyMobileBookmarksExist, and I'm not sure if we should be hiding this logic in LocalBrowserDB (this was just the easiest way to start).

Also, Madhava talked about wanting a UI structure like XUL fennec where we have "Desktop Bookmarks" and all your mobile bookmarks on the first bookmarks screen, then have the toolbar/menu/etc. folders inside "Desktop Bookmarks". I'm not sure what the best way to do this is - maybe I could add another header view for that Desktop Bookmarks folder item, then when the user taps on that, do getBookmarksInFolder(ROOT_ID), but filter out the mobile folder. I can try working on that tomorrow.
Attachment #596825 - Attachment is obsolete: true
(In reply to Margaret Leibovic [:margaret] from comment #23)
 
> Also, Madhava talked about wanting a UI structure like XUL fennec where we
> have "Desktop Bookmarks" and all your mobile bookmarks on the first
> bookmarks screen, then have the toolbar/menu/etc. folders inside "Desktop
> Bookmarks". I'm not sure what the best way to do this is - maybe I could add
> another header view for that Desktop Bookmarks folder item, then when the
> user taps on that, do getBookmarksInFolder(ROOT_ID), but filter out the
> mobile folder. I can try working on that tomorrow.

Let's not bite off too much in one patch. I don't completely follow Madhava's suggestion, but I think we can get a spec and tweak this code in a followup.
(In reply to Margaret Leibovic [:margaret] from comment #23)

> Also, Madhava talked about wanting a UI structure like XUL fennec where we
> have "Desktop Bookmarks" and all your mobile bookmarks on the first
> bookmarks screen, then have the toolbar/menu/etc. folders inside "Desktop
> Bookmarks". I'm not sure what the best way to do this is - maybe I could add
> another header view for that Desktop Bookmarks folder item, then when the
> user taps on that, do getBookmarksInFolder(ROOT_ID), but filter out the
> mobile folder. I can try working on that tomorrow.

It sounds like you want and/or have built:

1. A method that consumes a cursor to present a sequence of items
2. A parameterized query for each 'layer' to produce that cursor from some input
3. A starting query
4. Some logic to decide how much to cache and how much to recompute.

Presumably you already have (1). 

(4) can be "always recompute" for now; just remember the stack of inputs to (2).

(2) can be "WHERE parent = ?x ORDER BY title GROUP BY IS_FOLDER".

That leaves us with the starting query. For now I (and I'm sure mfinkle feels similarly) suggest (3) = (2) with input = "places".

Madhava would like something like: (3) to be "all children of 'mobile', followed by 'places' (titled 'Desktop Bookmarks')", with (2) special-cased to ignore the 'mobile' subfolder if its input is 'places'.

(This, incidentally, is why we started with Mobile Bookmarks as the root. Now we'll just return to making that virtual.)

I suggest that should be a follow-up. It's very necessary for user experience, but pretty much anything will be an improvement on what we have now.
No longer depends on: 716918
Blocks: 726270
Comment on attachment 597258 [details] [diff] [review]
(part 3) WIP - go up a folder level on back press

Review of attachment 597258 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/AwesomeBarTabs.java
@@ +330,5 @@
>  
> +    // This method checks to see if we're in a bookmark sub-folder. If we are,
> +    // it will go up a level and return true. Otherwise it will return false.
> +    public boolean onBackPressed() {
> +        Log.i("MARGARET", "AwesomeBarTabs.onBackPressed");

Remove logging on final patch.
Attachment #597258 - Flags: feedback?(lucasr.at.mozilla) → feedback+
(In reply to Margaret Leibovic [:margaret] from comment #22)
> Created attachment 597258 [details] [diff] [review]
> (part 3) WIP - go up a folder level on back press
> 
> Following guidance from UX folks, the system back button should take you
> back to previous folders. This is my attempt at trying to do that, but I'm
> finding onKeyPreIme always gets called twice, which results in us going up
> two levels, or going up one level and exiting the awesome screen. Lucas, you
> worked on this code, so I'm wondering if you know what might be causing this
> issue.

Weird. Have you tried same code on different devices? Sometimes OEMs customize system keys in broken ways. Another possibility is that this is caused by some focus-related issue. The location TextView doesn't always have focus which might cause some odd behaviour for onKeyPreIme. Maybe add an actual key handler for the case when the bookmarks listview is focused (not the location TextView)? Just throwing ideas here.
Depends on: 727449
Depends on: 727451
(In reply to Mark Finkle (:mfinkle) from comment #24)
> (In reply to Margaret Leibovic [:margaret] from comment #23)
>  
> > Also, Madhava talked about wanting a UI structure like XUL fennec where we
> > have "Desktop Bookmarks" and all your mobile bookmarks on the first
> > bookmarks screen, then have the toolbar/menu/etc. folders inside "Desktop
> > Bookmarks". I'm not sure what the best way to do this is - maybe I could add
> > another header view for that Desktop Bookmarks folder item, then when the
> > user taps on that, do getBookmarksInFolder(ROOT_ID), but filter out the
> > mobile folder. I can try working on that tomorrow.
> 
> Let's not bite off too much in one patch. I don't completely follow
> Madhava's suggestion, but I think we can get a spec and tweak this code in a
> followup.

I split that off into bug 727449. Also I split off the back press issue into bug 727451. This bug is already really big :)
Attached patch (1/2) Bookmark folder UI (obsolete) — Splinter Review
I think this is finally ready for review, since I split the remaining issues into follow-up bugs.

However, I've been testing this on top of the patches in bug 723841, and I've found that after I sync, the folder with id = FIXED_ROOT_ID has no children, so we'll want to fix that issue before this lands.
Attachment #597273 - Attachment is obsolete: true
Attachment #597429 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 597258 [details] [diff] [review]
(part 3) WIP - go up a folder level on back press

This will be addressed in bug 727451.
Attachment #597258 - Attachment is obsolete: true
Depends on: 727482
No longer blocks: 722413
Depends on: 727483
Attached patch (2/2) Fix the context menu (obsolete) — Splinter Review
We need to undo the effects of bug 725171, since we're switching back to using a ListView for bookmarks.
Attachment #596826 - Attachment is obsolete: true
Attachment #597437 - Flags: review?(wjohnston)
Attachment #596348 - Attachment is obsolete: true
Attachment #596349 - Attachment is obsolete: true
Attached image screenshot on ICS
(In reply to Margaret Leibovic [:margaret] from comment #29)

> However, I've been testing this on top of the patches in bug 723841, and
> I've found that after I sync, the folder with id = FIXED_ROOT_ID has no
> children, so we'll want to fix that issue before this lands.

Got a log?
Attached patch (2/2) Fix the context menu (v2) (obsolete) — Splinter Review
Now we won't show the context menu for folders. The only relevant item would be "remove", but I don't even know if we want to support that, since we shouldn't let users remove special folders (bug 727483), and we can't let them create folders anyway.
Attachment #597437 - Attachment is obsolete: true
Attachment #597437 - Flags: review?(wjohnston)
Attachment #597621 - Flags: review?(wjohnston)
Attached patch (1/2) Bookmark folder UI (obsolete) — Splinter Review
Un-bitrotted. Also, this patch now depends on bug 727482, which reverts the custom observer Brian made to deal with the CursorTreeAdapter.
Attachment #597429 - Attachment is obsolete: true
Attachment #597429 - Flags: review?(lucasr.at.mozilla)
Attachment #597910 - Flags: review?(lucasr.at.mozilla)
This also got bit-rotted, so I updated this as well.
Attachment #597621 - Attachment is obsolete: true
Attachment #597621 - Flags: review?(wjohnston)
Attachment #597911 - Flags: review?(wjohnston)
Attachment #597911 - Flags: review?(wjohnston) → review+
(I messed up resolving conflicts in the last patch.)
Attachment #597910 - Attachment is obsolete: true
Attachment #597910 - Flags: review?(lucasr.at.mozilla)
Attachment #597933 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 597933 [details] [diff] [review]
(1/2) Bookmark folder UI

Review of attachment 597933 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/AwesomeBarTabs.java
@@ +253,5 @@
> +        public int getViewTypeCount() {
> +            return VIEW_TYPE_COUNT;
> +        }
> +
> +        // This method adds titles to special folders that are missing titles

If this patch depends on the bookmarks migration from bug 723841, you don't need to do that because all special folders will have titles properly set.
Attachment #597933 - Flags: review?(lucasr.at.mozilla) → review+
Blocks: 728849
I'm going to wait for some sync bugs to land before I land this patch because without them the bookmarks folders disappear from the root list view after syncing. However, I tested with rnewman's patches applied, and they appeared to fix the problem, so hopefully those bugs will be resolved soon!
Depends on: 728783, 718238
Blocks: 725483
We need this change for beta. Bookmarks + Sync is a poor UX without this fixed.
(In reply to Mark Finkle (:mfinkle) from comment #41)
> We need this change for beta. Bookmarks + Sync is a poor UX without this
> fixed.

Sync part (well, six parts!) will land tomorrow, and Margaret's patches are looking pretty good in my tree.
… landed (Bug 718238).
https://hg.mozilla.org/mozilla-central/rev/230c6a8153c6
https://hg.mozilla.org/mozilla-central/rev/fa2de31b5ac0
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 13
Verified fixed on:

Firefox 13.0a1 (2012-02-24)
20120224031039
http://hg.mozilla.org/mozilla-central/rev/cd120efbe4c6

--
Device: Motorola Droid 2
OS: Android 2.3.3
Status: RESOLVED → VERIFIED
Depends on: 730358
blocking-fennec1.0: --- → beta+
Depends on: 730874
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.