737896 - Put desktop bookmark folders under a "Desktop Bookmarks" folder

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

Sigh, I failed to notice this in bug 727449 comment 5:

WHEN SYNC
- in first position, a folder called "Desktop Bookmarks"
   - it contains the three top-level desktop folders (menu, toolbar, unsorted)
- after that, we have a list of your mobile bookmarks - NOT in a "Mobile Bookmarks" folder

Comment 1

[Madhava Enros [:madhava]]

Thanks! Yes please!

Comment 2

[:Margaret Leibovic]

This is needed to fulfill Madhava's bookmarks UI design.

[21 09:09:57] <madhava> so - yes - I would _really_ _really_ _really_ like to see this fixed
[21 09:10:07] <madhava> (really)

Comment 3

[:Margaret Leibovic]

Attachment: WIP

This basically does what we want, modulo some weirdness with the divider below the hidden "Desktop Bookmarks" folder not hiding properly. I'm not sure if this fake folder row is a terrible hack, but it seems like the only way to make this happen, since we don't want to actually change the folder structure in the database, and I couldn't find a way to add a fake row to a real cursor.

I added an new async task within BookmarksQueryTask (yodawg) to find out whether or not desktop bookmarks exist, and it shouldn't matter which one of those tasks finishes first (the UI will work in either case). This makes me think we could potentially get rid of the code that caches mDesktopBookmarksExist within LocalBrowserDB, since with this patch we're only calling desktopBookmarksExist() once per AwesomeBar activity and keeping track of the result in AwesomeBarTabs.

This patch might need to change depending on what the patch for bug 715274 ends up looking like (it seems like that patch will be ready first).

Comment 4

[:Margaret Leibovic]

Attachment: patch

This required a decent amount of changes, but it's working well.

To avoid affecting the responsiveness of loading the bookmark items, I created a new AsyncTask (DesktopBookmarksQueryTask) to go do a check for desktop bookmarks in the background, then update the UI when it gets a result.

In bug 715274, Sriram changed how we show/hide the bookmarks title header view by dynamically adding/removing the header view, but when I tried to do that with more then one view, the activity would crash. You're not supposed to call addHeaderView() after calling setAdapter(), and we got around that by calling setAdapter(null), but that trick isn't robust enough (or supported anywhere I can find) to deal with multiple header views.

I tried adding two header views (one for the title header and one for the fake "Desktop Bookmarks" folder), but I couldn't get them to hide properly (a divider would still be visible), so I decided to just make a new LinearLayout as the header view, then stick the other views we want to dynamically show/hide inside of it. One hackish side effect of this is that handleBookmarkItemClick() needs to assume that the "Desktop Bookmarks" folder and the title header never appear at the same time (and that the "Desktop Bookmarks" folder is the one present in the root view). There may be a more robust way to figure out that click handling, but I figured this was okay for now, since that assumption is true.

The only issue I'm having is that I want to set the layout width of this new LinearLayout to FILL_PARENT, so that the header view will stretch all the way across, but I'm getting a crash when trying to call setLayoutParams. I'm working on that now, but I don't think that should prevent you from reviewing this patch.

Comment 5

[Lucas Rocha (:lucasr)]

Comment on attachment 610291 [details] [diff] [review]
patch

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

I'm a bit mixed about this patch. It adds a bit too much complexity to the UI code. Have you considered just returning a special wrapped cursor that "prepends" a row for the fake desktop bookmarks folder when you query for FIXED_ROOT_ID children? That would basically allow us to keep the UI code as is and do all the magic behind LocalBrowserDB. Giving r- just to discuss this a bit more before going ahead with this approach.

::: mobile/android/base/AwesomeBarTabs.java
@@ +228,5 @@
>              mParentStack.addFirst(folderPair);
>              refreshCurrentFolder();
>          }
>  
> +        public boolean inRootView() {

I'd do the inRootView() refactoring in a separate patch btw :-) No big deal, feel free to keep it in the same patch.

@@ +320,5 @@
> +        public void updateBookmarksTitleView(String title) {
> +            if (inRootView()) {
> +                mTitleHeaderView.setVisibility(GONE);
> +            } else {
> +                ((TextView) mTitleHeaderView.findViewById(R.id.title)).setText(title);

Hmm, those in-place type castings make the code less readable. I prefer assigning to a temp variable then using it. A bit more verbose but much easier to read.

@@ +404,5 @@
> +                // Initialize the folder title header view
> +                LinearLayout titleHeaderView = (LinearLayout) inflater.inflate(R.layout.awesomebar_header_row, null);
> +                titleHeaderView.setVisibility(GONE);
> +
> +                mBookmarksAdapter.setTitleHaderView(titleHeaderView);

typo: "setTitleHeaderView"

::: mobile/android/base/db/BrowserContract.java.in
@@ +104,5 @@
>      public static final class Bookmarks implements CommonColumns, URLColumns, ImageColumns, SyncColumns {
>          private Bookmarks() {}
>  
>          public static final int FIXED_ROOT_ID = 0;
> +        public static final int DESKTOP_FOLDER_ID = -1;

FAKE_DESKTOP_FOLDER_ID just for clarity? Otherwise people might misunderstand the intent here.

Comment 6

[:Margaret Leibovic]

Attachment: alternate patch

This patch does the CursorWrapper idea, and it's working well.

To re-map the positions, I decided to just make a special state for the desktop bookmarks folder row, and otherwise move the real cursor to a shifted position. I decided to only override the methods that we need in AweseomeBarTabs for the bookmarks UI, since that's the only place this Cursor is getting used. However, I don't know exactly what the CursorAdapter does, so maybe I should do more to be careful.

Comment 7

[Lucas Rocha (:lucasr)]

Comment on attachment 611059 [details] [diff] [review]
alternate patch

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

Much cleaner, nice :-) I think it's ok to just implement the Cursor API that we know the UI is using. We can fix any possible issues as we find them.

::: mobile/android/base/AwesomeBarTabs.java
@@ +255,5 @@
>                  return c.getString(c.getColumnIndexOrThrow(Bookmarks.TITLE));
>  
>              // Use localized strings for special folder names.
> +            if (guid.equals(Bookmarks.FAKE_DESKTOP_FOLDER_GUID))
> +                return mResources.getString(R.string.bookmarks_folder_desktop);

I guess this string is already defined?

Comment 8

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #7)
 
> ::: mobile/android/base/AwesomeBarTabs.java
> @@ +255,5 @@
> >                  return c.getString(c.getColumnIndexOrThrow(Bookmarks.TITLE));
> >  
> >              // Use localized strings for special folder names.
> > +            if (guid.equals(Bookmarks.FAKE_DESKTOP_FOLDER_GUID))
> > +                return mResources.getString(R.string.bookmarks_folder_desktop);
> 
> I guess this string is already defined?

Yup :)

Comment 9

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dc634206d156

Comment 10

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/dc634206d156

Comment 11

[:Margaret Leibovic]

Comment on attachment 611059 [details] [diff] [review]
alternate patch

[Approval Request Comment]
Mobile only. Beta blocker.

Comment 12

[Alex Keybl [:akeybl]]

Comment on attachment 611059 [details] [diff] [review]
alternate patch

[Triage Comment]
Mobile only & blocking Fennec 1.0. Approved for Aurora 13.

Comment 13

[Adrian Tamas (:AdrianT)]

Verified/fixed on:
Nightly 15.0a1 2012-04-25 / Aurora 14.0a2 2012-04-25
Device: HTC Desire (Android 2.2.2)

Leaving open as the fix has not landed in Aurora 13.0a2 2012-04-24. Please close as verified if it will not be integrated in Aurora 13 as approved in Comment 12.

Comment 14

[:Margaret Leibovic]

We are going to use Aurora 14 (the current aurora version) for fennec native 1.0, so this doesn't need to be uplifted to what's now beta.

Comment 15

[Andreea Pod]

Marking as verified fixed based on above comments.