TopBookmarksView in bug 882365 doesnt have pinning support. Add back!

(This is actually done. But still posted as WIP, as the designs aren't ready yet. If you feel it's better to land and tweak the designs later, please move it to r? :D ) This adds PinBookmarkSearch (based on BrowserSearch) that shows a frecency list based on search query. I tried using BrowserSearch here. The advantage is re-usability of code. The disadvantages are: 1. This doesn't have suggestions. 2. This won't have any long press behavior. 3. A default list is shown when there is no search query entered. 4. BrowserSearch should extends a DialogFragment and switch the setShowsDialog() method accordingly. But then BrowserSearch already extends HomeFragment. This would make copying long press listeners from HomeFragment back to BrowserSearch. Basically that approach would cause way too if-else blocks and re-architecturing BrowserSearch. I thought of keeping this simple and separate. May be the query here can be different too -- Ian has to decide this. Apart from that, TopBookmarksView will expose an interface OnPinBookmarkListener. This currently support adding only. This can be extended to editing and removing. BookmarksPage will respond to it, and open a dialog. When a list entry is selected, BookmarksPage will add it to the db. If nothing is selected, no callback is called, and everything remains the same.

Remove the icons from the list view in the dialog. This should only show url and not "switch to tab" or "bookmark" icons.

Comment on attachment 776688 [details] [diff] [review] Part 1: (WIP) Pin bookmark Review of attachment 776688 [details] [diff] [review]: ----------------------------------------------------------------- This looks pretty good overall. The only major change I suggest is to factor out the SearchLoader bits (just like we do with FaviconsLoader) and share the same code between BrowserSearch and PinBookmarkSearch. ::: mobile/android/base/home/BookmarksPage.java @@ +152,5 @@ > mList = null; > mListAdapter = null; > mTopBookmarks = null; > mTopBookmarksAdapter = null; > + mPinBookmarkListener = null; Don't you have to set the OnPinBookmarkListener to null on mTopBookmarks too? @@ +182,5 @@ > + */ > + private class PinBookmarkListener implements OnPinBookmarkListener, > + OnBookmarkSelectedListener { > + // Tag for the PinBookmarkSearch fragment. > + private static final String TAG_PIN_BOOKMARK = "pin_bookmark"; I'd name this PIN_BOOKMARK_TAG for consistency with BrowserApp. @@ +185,5 @@ > + // Tag for the PinBookmarkSearch fragment. > + private static final String TAG_PIN_BOOKMARK = "pin_bookmark"; > + > + // Position of the pin. > + private int mPosition; If we wanted to be strictly correct in terms of API safety, having a stateful listener is a bit dangerous e.g. how would you handle intertwined calls to onAddPin? We seem to have good control over this use case though. So, this should be fine. @@ +198,5 @@ > + dialog = PinBookmarkSearch.newInstance(); > + } > + > + dialog.setOnBookmarkSelectedListener(this); > + nit: remove empty line here. @@ +208,5 @@ > + final int position = mPosition; > + ThreadUtils.postToBackgroundThread(new Runnable() { > + @Override > + public void run() { > + BrowserDB.pinSite(getActivity().getContentResolver(), url, title, position); This is a bit racy. The fragment might be detached before this code runs. You'll have to do a null check on the activity before running pinSite. ::: mobile/android/base/home/TopBookmarksView.java @@ +28,5 @@ > private static final String LOGTAG = "GeckoTopBookmarksView"; > > + // Listener for pinning bookmarks. > + public static interface OnPinBookmarkListener { > + public void onAddPin(int position); I'd name this method onPinBookmark for consistency. @@ +89,5 @@ > @Override > public void onItemClick(AdapterView<?> parent, View view, int position, long id) { > TopBookmarkItemView row = (TopBookmarkItemView) view; > String url = row.getUrl(); > Just for the sake of clarity, add a comment explaining why empty url means the pin listener has to be called instead of the OnUrlOpenListener. ::: mobile/android/base/locales/en-US/android_strings.dtd @@ +269,5 @@ > <!ENTITY abouthome_addons_title "Add-ons for your &brandShortName;"> > <!ENTITY abouthome_addons_browse "Browse all &brandShortName; add-ons"> > <!ENTITY abouthome_last_tabs_title "Your tabs from last time"> > <!ENTITY abouthome_last_tabs_open "Open all tabs from last time"> > +<!ENTITY pin_bookmark_dialog_hint "Enter a search word"> Is that the string that ibarlow recommended? It looks a bit awkward. For one, I'd use "keyword" instead of "word".

Comment on attachment 776698 [details] [diff] [review] Part 2: No frills list Review of attachment 776698 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/home/TwoLinePageRow.java @@ +88,5 @@ > public void updateFromCursor(Cursor cursor) { > + updateFromCursor(cursor, true); > + } > + > + public void updateFromCursor(Cursor cursor, boolean showIcons) { Seems like the wrong place to toggle the showIcons. I'd prefer this to be set in a separate setter method setShowIcons(boolean) on TwoLinePageRow with default value as true.

(In reply to Lucas Rocha (:lucasr) from comment #4) > Comment on attachment 776698 [details] [diff] [review] > Part 2: No frills list > > Review of attachment 776698 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mobile/android/base/home/TwoLinePageRow.java > @@ +88,5 @@ > > public void updateFromCursor(Cursor cursor) { > > + updateFromCursor(cursor, true); > > + } > > + > > + public void updateFromCursor(Cursor cursor, boolean showIcons) { > > Seems like the wrong place to toggle the showIcons. I'd prefer this to be > set in a separate setter method setShowIcons(boolean) on TwoLinePageRow with > default value as true. Do you want to do something like, row.setShowIcons(false); row.updateFromCursor(cursor); This is making is stateful right? I thought of passing an argument and dealing with it.

(In reply to Lucas Rocha (:lucasr) from comment #3) > Comment on attachment 776688 [details] [diff] [review] > Part 1: (WIP) Pin bookmark > > Review of attachment 776688 [details] [diff] [review]: > ----------------------------------------------------------------- > > This looks pretty good overall. The only major change I suggest is to factor > out the SearchLoader bits (just like we do with FaviconsLoader) and share > the same code between BrowserSearch and PinBookmarkSearch. > > > @@ +182,5 @@ > > + */ > > + private class PinBookmarkListener implements OnPinBookmarkListener, > > + OnBookmarkSelectedListener { > > + // Tag for the PinBookmarkSearch fragment. > > + private static final String TAG_PIN_BOOKMARK = "pin_bookmark"; > > I'd name this PIN_BOOKMARK_TAG for consistency with BrowserApp. I somehow doesn't like this way of name. The "kind" show go first. And the actual name should follow. So that, it would be clear on what that constant is for. Say for example "TAG_*", "LOADER_ID_*", "STATUS_*". The reverse naming like "*_TAG" makes us read the entire word to figure out the kind. I recommend moving to the "TAG_*" and "LOADER_ID_*" in other places too. https://github.com/romainguy/ViewServer/blob/master/src/com/android/debug/hv/ViewServer.java#L120 If we had to name this in our convention, these would be like "PROTOCOL_VERSION_VALUE" and "PROTOCOL_VERSION_COMMAND". I tend to prefer the approach there. > > @@ +185,5 @@ > > + // Tag for the PinBookmarkSearch fragment. > > + private static final String TAG_PIN_BOOKMARK = "pin_bookmark"; > > + > > + // Position of the pin. > > + private int mPosition; > > If we wanted to be strictly correct in terms of API safety, having a > stateful listener is a bit dangerous e.g. how would you handle intertwined > calls to onAddPin? We seem to have good control over this use case though. > So, this should be fine. > > @@ +198,5 @@ > > + dialog = PinBookmarkSearch.newInstance(); > > + } > > + > > + dialog.setOnBookmarkSelectedListener(this); > > + > > nit: remove empty line here. > > @@ +208,5 @@ > > + final int position = mPosition; > > + ThreadUtils.postToBackgroundThread(new Runnable() { > > + @Override > > + public void run() { > > + BrowserDB.pinSite(getActivity().getContentResolver(), url, title, position); > > This is a bit racy. The fragment might be detached before this code runs. > You'll have to do a null check on the activity before running pinSite. > > ::: mobile/android/base/home/TopBookmarksView.java > @@ +28,5 @@ > > private static final String LOGTAG = "GeckoTopBookmarksView"; > > > > + // Listener for pinning bookmarks. > > + public static interface OnPinBookmarkListener { > > + public void onAddPin(int position); > > I'd name this method onPinBookmark for consistency. > > @@ +89,5 @@ > > @Override > > public void onItemClick(AdapterView<?> parent, View view, int position, long id) { > > TopBookmarkItemView row = (TopBookmarkItemView) view; > > String url = row.getUrl(); > > > > Just for the sake of clarity, add a comment explaining why empty url means > the pin listener has to be called instead of the OnUrlOpenListener. > > ::: mobile/android/base/locales/en-US/android_strings.dtd > @@ +269,5 @@ > > <!ENTITY abouthome_addons_title "Add-ons for your &brandShortName;"> > > <!ENTITY abouthome_addons_browse "Browse all &brandShortName; add-ons"> > > <!ENTITY abouthome_last_tabs_title "Your tabs from last time"> > > <!ENTITY abouthome_last_tabs_open "Open all tabs from last time"> > > +<!ENTITY pin_bookmark_dialog_hint "Enter a search word"> > > Is that the string that ibarlow recommended? It looks a bit awkward. For > one, I'd use "keyword" instead of "word". Nope :P That's why still in feedback statement. He should give me all these design related ones.

This moves the SearchCursorLoader to a SearchLoader.

Setting a state for showing icons. Leaving the other patch here for comparison.

Making the listeners and the cache null for the TopBookmarksView (as per the feedback comment for part 1).

Part 1 moved the SearchCursorLoader out into SearchLoader. I didn't want to clean up the getSearchTerm() there. This patch does that. CursorLoaderCallbacks knows the search term, as it is not a static class.

Comment on attachment 778165 [details] [diff] [review] Part 4: Search Term cleanup Review of attachment 778165 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/home/BrowserSearch.java @@ +601,5 @@ > > // We should handle autocompletion based on the search term > // associated with the currently loader that has just provided > // the results. > + handleAutocomplete(mSearchTerm, c); You can't use mSearchTerm here. mSearchTerm might change before the search loader finishes (see the comment just before this code).

Comment on attachment 778161 [details] [diff] [review] Part 1: Pin Bookmark Review of attachment 778161 [details] [diff] [review]: ----------------------------------------------------------------- Looks nice overall but needs some more cleanups. I assume ibarlow has approved the UI? Could you please share screenshots and/or test builds for UI review? ::: mobile/android/base/home/BookmarksPage.java @@ +153,5 @@ > mList = null; > mListAdapter = null; > mTopBookmarks = null; > mTopBookmarksAdapter = null; > + mPinBookmarkListener = null; No need to call setOnPinBookmarkListener(null) on mTopBookmarks? @@ +205,5 @@ > + > + @Override > + public void onBookmarkSelected(final String url, final String title) { > + final int position = mPosition; > + final Activity activity = getActivity(); I wonder if this is actually safe to do. Although the final here will ensure the instance will be kept around, the internal state of the activity might not be (if the activity gets destroyed). For instance, is a destroyed activity instance going to return a valid ContentResolver? It would be nice to double-check that. Maybe it would be safer to store the ContentResolver in a final here instead. I'd prefer to be safe and simply do a null check on getActivity() and only run the DB command if the fragment is up and running, which seems like the expected behavior. ::: mobile/android/base/home/BrowserSearch.java @@ +394,5 @@ > // Restart loaders with the new search term > + Bundle bundle = new Bundle(); > + bundle.putString(SearchLoader.KEY_SEARCH_TERM, mSearchTerm); > + bundle.putBoolean(SearchLoader.KEY_PERFORM_EMPTY_SEARCH, true); > + getLoaderManager().restartLoader(SEARCH_LOADER_ID, bundle, mCursorLoaderCallbacks); I'd prefer to keep these keys as an implementation detail of SearchLoader (see how FaviconsLoader's restartFromCursor() method is implemented). Simply pass searchTerm and performEmptySearch as arguments to the restart() comment in SearchLoader. Overload restart() method to have a version without the performEmptySearch argument (defaults to 'true'). ::: mobile/android/base/home/PinBookmarkDialog.java @@ +138,5 @@ > + filter(""); > + } > + > + private void filter(String searchTerm) { > + if (!TextUtils.equals(searchTerm, "") && isEmpty() instead? @@ +149,5 @@ > + // Restart loaders with the new search term > + Bundle bundle = new Bundle(); > + bundle.putString(SearchLoader.KEY_SEARCH_TERM, mSearchTerm); > + bundle.putBoolean(SearchLoader.KEY_PERFORM_EMPTY_SEARCH, true); > + getLoaderManager().restartLoader(SEARCH_LOADER_ID, bundle, mLoaderCallbacks); Use the suggest SearchLoader.restart(getLoaderManager(), SEARCH_LOADER_ID, mLoaderCallbacks, mSearchterm, false). I assume you do *not* want to perform the empty search here?

(In reply to Lucas Rocha (:lucasr) from comment #12) > Comment on attachment 778161 [details] [diff] [review] > Part 1: Pin Bookmark Before I forget: I'd probably factor out and use SearchLoader in BrowserSearch in a separate patch. This would have made this patch easier to review btw.

(In reply to Lucas Rocha (:lucasr) from comment #11) > Comment on attachment 778165 [details] [diff] [review] > Part 4: Search Term cleanup > > Review of attachment 778165 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mobile/android/base/home/BrowserSearch.java > @@ +601,5 @@ > > > > // We should handle autocompletion based on the search term > > // associated with the currently loader that has just provided > > // the results. > > + handleAutocomplete(mSearchTerm, c); > > You can't use mSearchTerm here. mSearchTerm might change before the search > loader finishes (see the comment just before this code). Arent we changing the mSearchTerm everytime before loading the cursor? And whenever search term changes, aren't we changing the loader? So, when the loader completes, won't it be for the latest mSearchTerm?

(In reply to Lucas Rocha (:lucasr) from comment #12) > Comment on attachment 778161 [details] [diff] [review] > Part 1: Pin Bookmark > > Review of attachment 778161 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks nice overall but needs some more cleanups. I assume ibarlow has > approved the UI? Could you please share screenshots and/or test builds for > UI review? mfinkle and ibarlow agreed on landed the patch (after getting r+) and polishing the UI during the hackathon week. > > ::: mobile/android/base/home/BookmarksPage.java > @@ +153,5 @@ > > mList = null; > > mListAdapter = null; > > mTopBookmarks = null; > > mTopBookmarksAdapter = null; > > + mPinBookmarkListener = null; > > No need to call setOnPinBookmarkListener(null) on mTopBookmarks? mTopBookmarks doesn't its part of cleanup when it detaches from the window (Patch 3). This is not needed. > > @@ +205,5 @@ > > + > > + @Override > > + public void onBookmarkSelected(final String url, final String title) { > > + final int position = mPosition; > > + final Activity activity = getActivity(); > > I wonder if this is actually safe to do. Although the final here will ensure > the instance will be kept around, the internal state of the activity might > not be (if the activity gets destroyed). For instance, is a destroyed > activity instance going to return a valid ContentResolver? It would be nice > to double-check that. Maybe it would be safer to store the ContentResolver > in a final here instead. > > I'd prefer to be safe and simply do a null check on getActivity() and only > run the DB command if the fragment is up and running, which seems like the > expected behavior. I'm planning to use the ApplicationContext ( getActivity().getApplicationContext() ). It's better to use Application context with the DB calls than the activity itself. > > ::: mobile/android/base/home/BrowserSearch.java > @@ +394,5 @@ > > // Restart loaders with the new search term > > + Bundle bundle = new Bundle(); > > + bundle.putString(SearchLoader.KEY_SEARCH_TERM, mSearchTerm); > > + bundle.putBoolean(SearchLoader.KEY_PERFORM_EMPTY_SEARCH, true); > > + getLoaderManager().restartLoader(SEARCH_LOADER_ID, bundle, mCursorLoaderCallbacks); > > I'd prefer to keep these keys as an implementation detail of SearchLoader > (see how FaviconsLoader's restartFromCursor() method is implemented). Simply > pass searchTerm and performEmptySearch as arguments to the restart() comment > in SearchLoader. Overload restart() method to have a version without the > performEmptySearch argument (defaults to 'true'). Is there a reason for using it this way. I'll change it -- just curious to know why.

Made changes as requested.

Screenshot with patches 1 & 2 applied.

Comment on attachment 779279 [details] [diff] [review] Part 1: Pin Bookmark Review of attachment 779279 [details] [diff] [review]: ----------------------------------------------------------------- Nice.

http://hg.mozilla.org/projects/fig/rev/8e852b2a313a http://hg.mozilla.org/projects/fig/rev/bd393061461a http://hg.mozilla.org/projects/fig/rev/9aca0034429f

Looks like this is causing a crash on fig: java.lang.NoSuchMethodError: android.os.Bundle.getStringat org.mozilla.gecko.home.SearchLoader.createInstance(SearchLoader.java:33) https://tbpl.mozilla.org/php/getParsedLog.php?id=25577032&tree=Fig I won't back it out, but you should land a follow-up to fix this.

https://hg.mozilla.org/mozilla-central/rev/8e852b2a313a https://hg.mozilla.org/mozilla-central/rev/bd393061461a https://hg.mozilla.org/mozilla-central/rev/9aca0034429f