When we start syncing desktop bookmarks over to Android Firefox, the bookmarks list is going to get very long, drowning mobile-created bookmarks in all of the bookmarks from the user's desktop system(s). The way we dealt with this in XUL Fennec was to show, in the "bookmarks" list view, a folder called "Desktop Bookmarks" at the top of the list. All desktop bookmarks, in their three top-level folders (unsorted, menu, toolbar) were in there, leaving your "mobile" bookmarks (i.e. the ones bookmarked locally) at the top level. We should do this in the native as well, I think.

Right now, we show all non-folder bookmarks from desktop and mobile in a flat alphabetically ordered list under the Bookmarks tab in awesomescreen. The DB backend is folder-aware IS aware of the folders though. So, this bug is about implementing the folder navigation UI to go under the Bookmarks tab. This is a non-trivial task and I suggest us to fix it post-1.0.

If we do need to wait until post Fx11 for true folders (which I think wd do), then I think we need to only show "mobile" bookmarks for Fx11. Flooding the flat list with desktop bookmarks would be less desirable, IMO. Desktop bookmarks will "appear" in the Top Sites list as part of searches.

(In reply to Mark Finkle (:mfinkle) from comment #2) > If we do need to wait until post Fx11 for true folders (which I think wd > do), then I think we need to only show "mobile" bookmarks for Fx11. Flooding > the flat list with desktop bookmarks would be less desirable, IMO. > > Desktop bookmarks will "appear" in the Top Sites list as part of searches. I agree with this proposal. Madhava, do you?

I worry that people will keep looking to see if their bookmarks are synced yet, and think that it's just not working.

I think this should be a higher priority. If we really just need to have the desktop bookmarks in a top level folder, is that too hard/time-consuming to do for 11?

(In reply to Madhava Enros [:madhava] from comment #5) > I think this should be a higher priority. If we really just need to have the > desktop bookmarks in a top level folder, is that too hard/time-consuming to > do for 11? The data model supports it. All of Fennec's bookmarks live in a single folder, tidily predetermined, and Sync works with that. (This is what XUL Fennec does, by the way.) My personal opinion is that it's a big enough UX win to warrant the work, but of course I'm lacking the context for the rest of the Fennec project, so that doesn't count for much! (In reply to Madhava Enros [:madhava] from comment #4) > I worry that people will keep looking to see if their bookmarks are synced > yet, and think that it's just not working. Very likely, and a support headache.

FWIW - this is what this looks like in XUL Fennec: http://dl.dropbox.com/u/9414926/for%20bugs/Screenshot_2012-01-26-15-57-30.png And then, a level in: http://dl.dropbox.com/u/9414926/for%20bugs/Screenshot_2012-01-26-15-59-28.png (but I don't think we'd need to do this for the first version, if it's too much -- most important thing is some segregation of local and synced bookmarks)

Again, for reference, here's the kind of thing people will be seeing after a sync if we don't do anything about it: http://dl.dropbox.com/u/9414926/for%20bugs/Screenshot_2012-01-26-17-33-45.png

If this is a beta blocker, I guess the priority should be raised?

Need to think about what to do with imported XUL bookmarks, which might also have come from Sync. Profile Migration currently doesn't try to preserve folder structure.

(In reply to Gian-Carlo Pascutto (:gcp) from comment #10) > Need to think about what to do with imported XUL bookmarks, which might also > have come from Sync. Profile Migration currently doesn't try to preserve > folder structure. Oh dear. Yeah, that would be problematic; the users' entire set of bookmarks would be duplicated on first sync. They need to preserve GUID, URI, title, modified time, and parent to avoid headaches, and the GUIDs for special folders need to be transformed from the Places hierarchy. Preserving tags etc. as well, such that our notion of record equality applies, will avoid some extra work for Sync. My notes on the translation: https://github.com/mozilla-services/android-sync/blob/develop/src/main/java/org/mozilla/gecko/sync/repositories/android/RepoUtils.java#L75 E.g., the Unsorted Bookmarks folder needs to be stored with GUID "unfiled" in the Fennec DB, and records that referred to it need to be altered accordingly. I'm happy to advise on this if you need me.

>They need to preserve GUID, URI, title, modified time, and parent to avoid >headaches, Not possible right now. BrowserDB only exposes URL and title. In any case it doesn't have more than this to work with: http://developer.android.com/reference/android/provider/Browser.BookmarkColumns.html I have a bug about migrating Sync settings from XUL. How about this: if the user has a Sync account, we do *not* Migrate his XUL profile.

As discussed in IRC: Fennec doesn't ship with a user-visible switch for global DB right now, so having migration code work at the lowest common denominator is gently harmful. My suggestion is to alter the migration code to: * Pull more data out of Places * Write into the ContentProvider interface exposed by Fennec. That way we can include GUIDs, modification times, folder positions, etc. etc. Essentially, turning part of this: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/ProfileMigrator.java#148 into this: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/sync/repositories/android/AndroidBrowserRepositoryDataAccessor.java#104 We lose the granularity of history visits, but I expect Sync will be able to recover those. It might even be possible to conditionalize on localDB, if you want to avoid bitrotting. If you'd rather not do this work (I think it's valuable, because it will reduce the number of writes on a first sync, and it'll seamlessly support a hierarchical bookmarks view as soon as that arrives), then in the mean time it's better to not migrate bookmarks and data for any user who's ever had Sync set up on their phone. (You should still migrate passwords, prefs, etc. of course.) Still happy to help with this :D

Just to be blindingly obvious, this has a couple of strings; I'm not sure how many of them come over via sync (i.e. as folder names) vs. being strings we ship in the product, but they are: Desktop Bookmarks Unsorted Bookmarks Bookmarks Menu Bookmarks Toolbar These are the same as were in XUL Fennec.

As a fallback position, if folders really cannot happen in the fx11 timeframe, we could do this: - divide the bookmarks list into sections, as we do for History (i.e. Today, Yesterday, etc.) - the first section would be your local "mobile" bookmarks - the second section has a header that reads "Desktop Bookmarks" - the second section contains the flat list of desktop bookmarks (even though we have the folder structure in the database) This gets the flood of desktop stuff out of the way, at least. And then we do foldering later.

Need to de-prioritize this as a beta blocker at this point.

(In reply to Erin Lancaster from comment #16) > Need to de-prioritize this as a beta blocker at this point. There are a few things we need to make sure still block beta: * Fennec can not break bookmark folders on desktop, due to Sync pushing data from Fennec back to desktop * Fennec needs to improve the display of bookmarks when using Sync. Way too many bookmarks are just thrown in the list. Comment 15 has a good, achievable plan for Fx11.

(In reply to Mark Finkle (:mfinkle) from comment #17) > * Fennec can not break bookmark folders on desktop, due to Sync pushing data > from Fennec back to desktop This shouldn't be a worry unless Fennec changes the folder of a bookmark after it's created. > * Fennec needs to improve the display of bookmarks when using Sync. Way too > many bookmarks are just thrown in the list. Comment 15 has a good, > achievable plan for Fx11. Agreed.

I can take this. So, do we just want to do Madhava's idea from comment 15? Do those strings exist somewhere for me to use, or will I need to make new strings? I can also explore making those sections collapsible, as described on IRC.

(In reply to Margaret Leibovic [:margaret] from comment #19) > I can take this. So, do we just want to do Madhava's idea from comment 15? > Do those strings exist somewhere for me to use, or will I need to make new > strings? I can also explore making those sections collapsible, as described > on IRC. New strings, since this is Java.

(In reply to Mark Finkle (:mfinkle) from comment #20) > (In reply to Margaret Leibovic [:margaret] from comment #19) > > I can take this. So, do we just want to do Madhava's idea from comment 15? > > Do those strings exist somewhere for me to use, or will I need to make new > > strings? I can also explore making those sections collapsible, as described > > on IRC. > > New strings, since this is Java. Actually, Margaret found that Sync already added these strings. We should be OK: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/locales/en-US/sync_strings.dtd#68

(In reply to Margaret Leibovic [:margaret] from comment #19) > I can take this. So, do we just want to do Madhava's idea from comment 15? > Do those strings exist somewhere for me to use, or will I need to make new > strings? I can also explore making those sections collapsible, as described > on IRC. Just a few suggestions: - CursorTreeAdapter is probably what you want to use. You'd then be using an ExpandableListView which gives the collapsible groups for free. - If you want to disable the collapsible behaviour in ExpandableListView, you can override clicks on the groups (see how the History tab in AwesomeBarTabs is implemented) - You'll have to tweak the BrowserDB API so that you can get cursor for bookmarks in certain categories. Something like: getTopLevelBookmarkFolders() and get getBookmarksForCategory(categoryId) should do it. You'll have to find a sane enough behavior for the AndroidBrowserDB implementation though as Android's DB doesn't support folders. Maybe do something that just shows all bookmarks in a flat list.

This seems to mostly work, but I'm having trouble finding a way to ensure that our "Mobile Bookmarks" folder is always the top-most group. Do you know if there's a way to sort folders alphabetically, but then move one to the top? Unfortunately, I think I may be running into a sync bug - before setting up sync, bookmarking a page adds it to the "Mobile Bookmarks" folder as expected, but after syncing, I have a "Mobile Bookmarks" folder that appears in the middle of the list, and it only contains "Firefox: Support" and "Firefox: Welcome" links that probably ended up in my sync data from an old mobile profile (although on desktop, my "Mobile Bookmarks" folder contains new pages I've bookmarked with Fennec native). I'm also ending up with an empty and untitled folder. I'm not sure if these issues are due to syncing with my crufty old sync data, but it's something we should address. (I just stubbed out the APIs I added to BrowserDB in AndroidBrowserDB to avoid errors in there, but based on conversation on IRC today it sounds like we're probably going to get rid of that, which will make this folder business easier.)

Getting rid of the late-l10n flag because this won't add new strings.

(In reply to Margaret Leibovic [:margaret] from comment #23) > This seems to mostly work, but I'm having trouble finding a way to ensure > that our "Mobile Bookmarks" folder is always the top-most group. Do you know > if there's a way to sort folders alphabetically, but then move one to the > top? You could try prepending a non-printing character (e.g., U+2060 word joiner (HTML: ⁠)) to the string… > Unfortunately, I think I may be running into a sync bug - before setting up > sync, bookmarking a page adds it to the "Mobile Bookmarks" folder as > expected, but after syncing, I have a "Mobile Bookmarks" folder that appears > in the middle of the list, and it only contains "Firefox: Support" and > "Firefox: Welcome" links that probably ended up in my sync data from an old > mobile profile (although on desktop, my "Mobile Bookmarks" folder contains > new pages I've bookmarked with Fennec native). I think the following has occurred: * A Mobile Bookmarks folder (that is, a folder with that title at the root) has been downloaded from the server that somehow doesn't have the GUID "mobile" * Its GUID has overwritten the local "magic" Mobile Bookmarks folder's GUID * Weirdness results. Do you have an adb log that I can look at? This should be reproducible if you uninstall then reinstall Fennec, and set up Sync again. A whole and complete log (just adb logcat to a file from prior to installation onwards) would be really helpful. Please file here: https://bugzilla.mozilla.org/enter_bug.cgi?product=Mozilla%20Services&component=Android%20Sync&rep_platform=ARM&op_sys=Android > I'm also ending up with an > empty and untitled folder. I'm not sure if these issues are due to syncing > with my crufty old sync data, but it's something we should address. Could be. Logs, please!

Comment on attachment 594046 [details] [diff] [review] WIP Review of attachment 594046 [details] [diff] [review]: ----------------------------------------------------------------- I like the direction of this patch. The main issue I see right now is changing getBookmarksInFolder() to list of non-folder bookmarks under each toplevel folder (taking into account that there might be sub-folders). ::: mobile/android/base/AwesomeBarTabs.java @@ +123,5 @@ > + ContentResolver resolver = mContext.getContentResolver(); > + > + // Each group is populated with bookmarks from a single parent folder > + int folderId = groupCursor.getInt(groupCursor.getColumnIndexOrThrow(Bookmarks._ID)); > + return BrowserDB.getBookmarksInFolder(resolver, folderId); I think the getBookmarksInFolder() implementation will have to be a bit smarter. It doesn't cover the case of a folder having sub-folders. For 1.0, I think it's ok to simply show a flat list of all non-folder bookmarks under each toplevel folders (i.e. ignore subfolders). @@ +148,5 @@ > + // Use the URL instead of an empty title for consistency with the normal URL > + // bar view - this is the equivalent of getDisplayTitle() in Tab.java > + if (title == null || title.length() == 0) { > + try { > + title = cursor.getString(cursor.getColumnIndexOrThrow(URLColumns.URL)); Redundant try here? Just use same try block than code below? @@ +152,5 @@ > + title = cursor.getString(cursor.getColumnIndexOrThrow(URLColumns.URL)); > + } catch (Exception e) { } > + > + try { > + // The mobile bookmarks folder doesn't have a title, so we need to give it one Does it only happen for mobile bookmarks? I thought sync would set title for all folders. Double-check with rnewman. This might be a sync bug. @@ +155,5 @@ > + try { > + // The mobile bookmarks folder doesn't have a title, so we need to give it one > + String guid = cursor.getString(cursor.getColumnIndexOrThrow(Bookmarks.GUID)); > + if (guid.equals(Bookmarks.MOBILE_FOLDER_GUID)) { > + Log.i("MARGARET", "Got mobile bookmarks folder"); I usually use BOOM for log tag :-) Don't know why. @@ +158,5 @@ > + if (guid.equals(Bookmarks.MOBILE_FOLDER_GUID)) { > + Log.i("MARGARET", "Got mobile bookmarks folder"); > + title = mContext.getResources().getString(R.string.bookmarks_folder_mobile); > + } > + } catch (Exception e) { } No sane fallback title in mind in case of an exception? Maybe return false in case of an exception? @@ +171,5 @@ > + int faviconIndex = cursor.getColumnIndexOrThrow(URLColumns.FAVICON); > + if (columnIndex == faviconIndex) { > + return updateFavicon(view, cursor, faviconIndex); > + } > + } catch (Exception e) { } return false in case of an exception? ::: mobile/android/base/db/LocalBrowserDB.java @@ +282,5 @@ > + Cursor c = cr.query(appendProfile(Bookmarks.CONTENT_URI), > + new String[] { Bookmarks._ID, > + Bookmarks.GUID, > + Bookmarks.TITLE }, > + Bookmarks.IS_FOLDER + " = 1", You probably want to also make sure you're only getting the toplevel folders here? Something like Bookmarks.PARENT = null. This way you avoid fetching sub-folders under Desktop Bookmarks, for example. @@ +295,5 @@ > + new String[] { Bookmarks._ID, > + Bookmarks.URL, > + Bookmarks.TITLE, > + Bookmarks.FAVICON }, > + Bookmarks.PARENT + " = " + folderId, What if some of the bookmarks in the folder are sub-folders?

(In reply to Margaret Leibovic [:margaret] from comment #23) > Created attachment 594046 [details] [diff] [review] > WIP > > This seems to mostly work, but I'm having trouble finding a way to ensure > that our "Mobile Bookmarks" folder is always the top-most group. Do you know > if there's a way to sort folders alphabetically, but then move one to the > top? One option if to fetch Mobile folder in one query, and the rest of the folders in a separate query using a where clause like "where GUID != MOBILE_FOLDER_GUID. Then you create a MatrixCursor[1] and add the rows in the order you want. You could also come up with a smarter sortOrder clause that does that for you. [1] http://developer.android.com/reference/android/database/MatrixCursor.html

> Does it only happen for mobile bookmarks? I thought sync would set title for > all folders. Double-check with rnewman. This might be a sync bug. If there's a title, we'll set it. But in this case the "mobile" folder is managed by createMobileBookmarksFolder in BrowserProvider. Sadly, that method doesn't set a parent or title :( :( Filed Bug 724045. Sync will set one the first time it downloads another device's Mobile Bookmarks folder, but that relies on you already having used XUL Fennec sometime in the past. Sync also has code to insert a Mobile Bookmarks folder with a title and a parent, but of course Fennec will likely get there first. > You probably want to also make sure you're only getting the toplevel folders > here? Something like Bookmarks.PARENT = null. This way you avoid fetching > sub-folders under Desktop Bookmarks, for example. No parent should be null, save for the root (0). Arguably that shouldn't be null, either :) Top-level folders (toolbar, etc.) should have parent with guid "places". You can also hard-code the list. Here's our default folder structure: public static final Map<String, String> SPECIAL_GUID_PARENTS; static { HashMap<String, String> m = new HashMap<String, String>(); m.put("places", null); m.put("menu", "places"); m.put("toolbar", "places"); m.put("tags", "places"); m.put("unfiled", "places"); m.put("mobile", "places"); SPECIAL_GUID_PARENTS = Collections.unmodifiableMap(m); }

(In reply to Richard Newman [:rnewman] from comment #25) > I think the following has occurred: > > * A Mobile Bookmarks folder (that is, a folder with that title at the root) > has been downloaded from the server that somehow doesn't have the GUID > "mobile" > * Its GUID has overwritten the local "magic" Mobile Bookmarks folder's GUID > * Weirdness results. > > Do you have an adb log that I can look at? > > This should be reproducible if you uninstall then reinstall Fennec, and set > up Sync again. A whole and complete log (just adb logcat to a file from > prior to installation onwards) would be really helpful. Filed bug 724102. > > I'm also ending up with an > > empty and untitled folder. I'm not sure if these issues are due to syncing > > with my crufty old sync data, but it's something we should address. > > Could be. Logs, please! It looks like I was making an expandable section for the root node, which has no title. I won't do this. (No bookmarks should be children of this root node, right? I assume bookmarks filed without a specified parent would be children of "Unsorted Bookmarks"/"unfiled".)

(In reply to Margaret Leibovic [:margaret] from comment #29) > Filed bug 724102. Thanks! Will address today. > It looks like I was making an expandable section for the root node, which > has no title. I won't do this. (No bookmarks should be children of this root > node, right? Correct. It should only be special folders. > I assume bookmarks filed without a specified parent would be > children of "Unsorted Bookmarks"/"unfiled".) Yup.

(In reply to Lucas Rocha (:lucasr) from comment #26) > ::: mobile/android/base/AwesomeBarTabs.java > @@ +123,5 @@ > > + ContentResolver resolver = mContext.getContentResolver(); > > + > > + // Each group is populated with bookmarks from a single parent folder > > + int folderId = groupCursor.getInt(groupCursor.getColumnIndexOrThrow(Bookmarks._ID)); > > + return BrowserDB.getBookmarksInFolder(resolver, folderId); > > I think the getBookmarksInFolder() implementation will have to be a bit > smarter. It doesn't cover the case of a folder having sub-folders. For 1.0, > I think it's ok to simply show a flat list of all non-folder bookmarks under > each toplevel folders (i.e. ignore subfolders). So for 1.0, I can just make sure getBookmarksInFolder doesn't return folders, and this code should suffice, right? > @@ +148,5 @@ > > + // Use the URL instead of an empty title for consistency with the normal URL > > + // bar view - this is the equivalent of getDisplayTitle() in Tab.java > > + if (title == null || title.length() == 0) { > > + try { > > + title = cursor.getString(cursor.getColumnIndexOrThrow(URLColumns.URL)); > > Redundant try here? Just use same try block than code below? This ViewBinder class is kinda confusing - it's called for both the group header views, as well as the child views. For child views, we'd be looking for a URL, and for group header views we'd be looking for a GUID. So, I can't actually put these in the same try block, since if we're updating a group header view, it would throw for a URL, but return a value for a GUID. I can look into ways of making this code cleaner, though. > @@ +152,5 @@ > > + title = cursor.getString(cursor.getColumnIndexOrThrow(URLColumns.URL)); > > + } catch (Exception e) { } > > + > > + try { > > + // The mobile bookmarks folder doesn't have a title, so we need to give it one > > Does it only happen for mobile bookmarks? I thought sync would set title for > all folders. Double-check with rnewman. This might be a sync bug. This turned out to be bug 724045, but as Richard mentioned in there, I'll need to still be careful here because old DBs might not have a title for the mobile folder. > @@ +171,5 @@ > > + int faviconIndex = cursor.getColumnIndexOrThrow(URLColumns.FAVICON); > > + if (columnIndex == faviconIndex) { > > + return updateFavicon(view, cursor, faviconIndex); > > + } > > + } catch (Exception e) { } > > return false in case of an exception? Once again, this is the problem I was talking about regarding this ViewBinder being used for both group header and child views, so a group header will throw when trying to get a favicon, but we won't want to return false, since we're setting a value for a different index in that case.

In BookmarksViewBinder, I decided to use getColumnIndex instead of getColumnIndexOrThrow to avoid all the try/catch blocks, and I'm just checking that index to know which view we're updating. I used a MatrixCursor to ensure that the mobile bookmarks folder is on top. This feels really verbose - I'm not sure if there's a better way to do this, or if that's just Java :) As suggested, I'm just ignoring sub-folders, and I'm just making all folders into top-level groups. I think all the sync issues I found are filed and/or fixed. This works really well with the build I just made from the latest m-c!

Comment on attachment 594400 [details] [diff] [review] patch Review of attachment 594400 [details] [diff] [review]: ----------------------------------------------------------------- I'd like to get Madhava's input before giving r+. The current implementation might lead to very strange behaviour if the user have a bunch of sub-folders in her desktop Firefox. ::: mobile/android/base/AwesomeBarTabs.java @@ +217,5 @@ > + favicon.setImageResource(R.drawable.favicon); > + } else { > + Bitmap bitmap = BitmapFactory.decodeByteArray(b, 0, b.length); > + favicon.setImageBitmap(bitmap); > + } Add an empty line here. @@ +224,5 @@ > + > + private boolean updateTitle(View view, Cursor cursor, int titleIndex) { > + String title = cursor.getString(titleIndex); > + // If we already have a good title, we don't need to do anything > + if (title != null && title.length() > 0) You can use android's TextUtils.isEmpty() instead. @@ +230,5 @@ > + > + TextView textView = (TextView) view; > + > + // If we're updating a child view, use the URL instead of an empty title > + int urlIndex = cursor.getColumnIndex(URLColumns.URL); I think it would safer and more clear if you returned IS_FOLDER from the queries. This way you can directly check it here instead of implying that this a bookmark is a folder if URL is null. @@ +241,5 @@ > + int guidIndex = cursor.getColumnIndex(Bookmarks.GUID); > + if (guidIndex >= 0) { > + String guid = cursor.getString(guidIndex); > + // In older DBs, the mobile bookmarks folder might not have > + // a title, so we need to give it one ourselves Hmm, this looks like the wrong place to workaround this problem. Maybe you could add this code to getAllBookmarkFolders() when building the MatrixCursor instead? This way you don't need to rely on users of the API to do it separately. In any case, I think it would good to factor this code in a separate "maybeSetMobileFolderTitle()" method. ::: mobile/android/base/db/LocalBrowserDB.java @@ +297,5 @@ > + new String[] { Bookmarks.MOBILE_FOLDER_GUID }, > + null); > + > + if (helperCursor.moveToFirst()) { > + c.addRow(new Object[] { helperCursor.getLong(helperCursor.getColumnIndexOrThrow(Bookmarks._ID)), Is this getColumnIndexOrThrow() really necessary here? The user of getAllBookmarkFolders() is getColumnIndexOrThrow() anyway. If there's a bug here, we'll catch it. @@ +315,5 @@ > + Bookmarks.IS_FOLDER + " = 1 AND " + > + Bookmarks.GUID + " != ? AND " + Bookmarks.GUID + " != ?", > + // Ignore the root node and the mobile bookmarks folder > + new String[] { Bookmarks.PLACES_FOLDER_GUID, Bookmarks.MOBILE_FOLDER_GUID }, > + Bookmarks.TITLE + " ASC"); This is fetching all toplevel folders and their subfolders in alphabetical order. Is that intended? If the user has a bunch of subfolders on desktop Firefox, this will look a bit odd as you're flattening the bookmark folder structure. Just asking because I thought the idea was to group all bookmarks into toplevel buckets (disregarding the subfolders). So, Desktop Bookmarks would be a flat list of all non-folder bookmarks in all its sub-folders. I'd like to know Madhava's opinion before giving r+. @@ +317,5 @@ > + // Ignore the root node and the mobile bookmarks folder > + new String[] { Bookmarks.PLACES_FOLDER_GUID, Bookmarks.MOBILE_FOLDER_GUID }, > + Bookmarks.TITLE + " ASC"); > + > + helperCursor.moveToPosition(-1); Maybe factor out the code to add a row to the MatrixCursor from helperCursor? Just avoid some code duplication here.

Based on some discussion in IRC, given that we're just going with a flat list within the top-level "Desktop Bookmarks" category (which is after the "Mobile Bookmarks" category), I'd like to see that flat list ordered this way: - bookmarks from the "Bookmarks Toolbar" folder - bookmarks from the "Bookmarks Menu" folder - bookmarks from the "Unsorted" folder

> - bookmarks from the "Bookmarks Toolbar" folder Transitive parent GUID = "toolbar" > - bookmarks from the "Bookmarks Menu" folder … "menu" > - bookmarks from the "Unsorted" folder … "unfiled". Bear in mind that if you flatten a whole tree, it makes sense to (a) sort the contents, and (b) remove duplicates.

As discussed on IRC, this patch simplifies things to make only "Mobile Bookmarks" and "Desktop Bookmarks" sections. Since I mostly copied AwesomeCursorViewBinder to make the ViewBinder in my previous patch, I decided to just extend that class to handle the new BookmarksListAdapter. Also, since I'm sure the groups have titles now that I'm setting them myself, we don't need to have an empty title check for group headers. I tried to keep this as simple as possible, so we can push things like making a better expandable group header UI and sorting the bookmarks differently to follow-ups.

Following conversation with Madhava on IRC, I tried to sort by DATE_CREATED, but since we don't sync that value, that doesn't work well, so I'm going to change that to DATE_MODIFIED, which accomplishes the same idea. Perhaps we want to reverse the order, so that your most recently modified bookmarks are at the top. Madhava, what do you think of that?

Comment on attachment 594789 [details] [diff] [review] patch v2 Review of attachment 594789 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. I just want to the questions answered (especially about the header clicks) before giving a r+. A screenshot would be good too. ::: mobile/android/base/AwesomeBarTabs.java @@ +179,5 @@ > } > > public boolean setViewValue(View view, Cursor cursor, int columnIndex) { > + // If we're updating a folder header in the bookmarks UI, > + // we don't need to do anything to update the view. Because the adapter handles the title automatically for us, right? @@ +200,5 @@ > } > } > > + private class RefreshChildrenCursorTask extends AsyncTask<String, Void, Cursor> { > + nit: Remove this empty line? @@ +204,5 @@ > + > + private int mGroupPosition; > + > + public RefreshChildrenCursorTask(int groupPosition) { > + this.mGroupPosition = groupPosition; No need for "this." here. @@ +234,5 @@ > + } > + > + @Override > + protected Cursor getChildrenCursor(Cursor groupCursor) { > + final String guid = groupCursor.getString(groupCursor.getColumnIndexOrThrow(Bookmarks.GUID)); Does this really need to be final? Just wondering. @@ +256,5 @@ > + Bookmarks.IS_FOLDER, > + Bookmarks.GUID, > + URLColumns.TITLE }, 2); > + c.addRow(new Object[] { 0, 1, Bookmarks.MOBILE_FOLDER_GUID, > + mContext.getResources().getString(R.string.bookmarks_folder_mobile)} ); Assign resources to a temp variable to avoid using getResources() twice here? This would also make the code a bit more readable. @@ +284,5 @@ > > + bookmarksList.setOnChildClickListener(new ExpandableListView.OnChildClickListener() { > + public boolean onChildClick(ExpandableListView parent, View view, > + int groupPosition, int childPosition, long id) { > + handleBookmarkItemClick(groupPosition, childPosition); The handleBookmarkItemClick() method doesn't seem to handle clicks on the headers properly. Make sure it doesn't crash in that case. @@ +759,5 @@ > } > > + private void handleBookmarkItemClick(int groupPosition, int childPosition) { > + Cursor cursor = mBookmarksAdapter.getChild(groupPosition, childPosition); > + String url = cursor.getString(cursor.getColumnIndexOrThrow(URLColumns.URL)); If you click on a the header, the url will be null (I guess?) and you're be calling the listener with a null url, right? ::: mobile/android/base/db/LocalBrowserDB.java @@ +301,5 @@ > + Bookmarks.URL, > + Bookmarks.TITLE, > + Bookmarks.FAVICON }, > + Bookmarks.IS_FOLDER + " = 0 AND " + > + Bookmarks.PARENT + " != ?", Maybe have a getBookmarks(boolean mobile) private method that has a this code and just add something like: Bookmarks.PARENT + (mobile ? " = ?" : " != ?"), Then getDesktopBookmarks() and getMobileBookmarks() would be implemented in terms of getBookmarks(false|true). This way you'd avoid this code duplication here.

Comment on attachment 594789 [details] [diff] [review] patch v2 Margaret says the click handler only applies to children, not the group headers. Giving r+ as this was the only major concern I had. It would be nice if the nits are fixed though.

Pushed to inbound with nits addressed: https://hg.mozilla.org/integration/mozilla-inbound/rev/263bd2869408

Comment on attachment 594789 [details] [diff] [review] patch v2 [Approval Request Comment] User impact if declined: Can't distinguish mobile/desktop bookmarks Testing completed (on m-c, etc.): Yes, some regressions found, so we'll want to land those follow-up bugs with this patch (and make sure we caught all the regressions) Risk to taking this patch (and alternatives if risky): Potentially more regressions, but the alternative is a big unsorted pile of bookmarks

Comment on attachment 594789 [details] [diff] [review] patch v2 [Triage Comment] Mobile only - approved for Aurora 12 and Beta 11.

Comment on attachment 594789 [details] [diff] [review] patch v2 Clearing approval for Aurora 12 and Beta 11 because we are not currently planning a Native Fennec release of these versions. If this changes in the future, we will likely do a mass uplift of all native fennec changes. For now, let's get these bugs off the channel triage radar. [Filter on the string "mbrubeck-bugspam" if you want to delete all of these emails at once.]

Verified fixed on: Firefox 13.0a1 (2012-02-29) 20120229031108 http://hg.mozilla.org/mozilla-central/rev/30b4f99a137c -- Device: Samsung Galaxy S2 OS: Android 2.3.4