Closed Bug 737896 Opened 8 years ago Closed 8 years ago

Put desktop bookmark folders under a "Desktop Bookmarks" folder

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 14
Tracking Status
firefox13 --- affected
blocking-fennec1.0 --- beta+

People

(Reporter: Margaret, Assigned: Margaret)

References

Details

(Whiteboard: yodawg)

Attachments

(1 file, 2 obsolete files)

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
Thanks! Yes please!
Assignee: nobody → 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)
blocking-fennec1.0: --- → ?
blocking-fennec1.0: ? → beta+
Whiteboard: yodawg
Attached patch WIP (obsolete) — Splinter Review
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).
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #608917 - Attachment is obsolete: true
Attachment #610291 - Flags: review?(lucasr.at.mozilla)
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.
Attachment #610291 - Flags: review?(lucasr.at.mozilla) → review-
Attached patch alternate patchSplinter Review
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.
Attachment #610291 - Attachment is obsolete: true
Attachment #611059 - Flags: review?(lucasr.at.mozilla)
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?
Attachment #611059 - Flags: review?(lucasr.at.mozilla) → review+
(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 :)
https://hg.mozilla.org/mozilla-central/rev/dc634206d156
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
Comment on attachment 611059 [details] [diff] [review]
alternate patch

[Approval Request Comment]
Mobile only. Beta blocker.
Attachment #611059 - Flags: approval-mozilla-aurora?
Comment on attachment 611059 [details] [diff] [review]
alternate patch

[Triage Comment]
Mobile only & blocking Fennec 1.0. Approved for Aurora 13.
Attachment #611059 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
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.
Marking as verified fixed based on above comments.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.