:Margaret Leibovic Reporter
	Description • 11 years ago

Not sure if this should be part of v1, but it's something to think about when designing our data storage.

It would also be necessary if we were to dogfood the list system such that we used it to build our own bookmarks UI.

	Josh Dover [:jdover] Assignee
	Comment 4 • 10 years ago

Use this patch to get sample data that has a folder hierarchy. 

The main thing that still needs to be determined is how we want to display which folder the user is currently in. Do we want to do this the same way as bookmarks? In general, it seems that not many are huge fans of the way it is done there. Tagging ibarlow for input on this.

Link to build with all of this in it: https://drive.google.com/file/d/0BwqqgfwYmRg4NmdkZW12YTE2NGc/edit?usp=sharing

	Ian Barlow (:ibarlow)
	Comment 5 • 10 years ago

Thanks Josh. This looks good but needs a little work too. The core interaction of tapping a folder to drill down, and tapping back is correct, but we want to build on that a little. Adding a header row with an up icon and a "back to folder-name" string, as well as a title row indicating the current folder, will help a little with wayfinding. 

Sorry for the crude sketch, but hopefully it's enough to keep moving ahead http://cl.ly/image/40061C1U0F0y

	Lucas Rocha (:lucasr)
	Comment 6 • 10 years ago

Comment on attachment 8370914 [details] [diff] [review]
Add folder column to Hub schema

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

::: mobile/android/base/db/BrowserContract.java
@@ +304,4 @@
>          public static final String DESCRIPTION = "description";
>          public static final String IMAGE_URL = "image_url";
>          public static final String CREATED = "created";
> +        public static final String FOLDER = "folder";

I'd be more inclined to use 'path' instead of folder here. 'Folder' is an overloaded and kinda ambiguous term.

	Lucas Rocha (:lucasr)
	Comment 7 • 10 years ago

Comment on attachment 8370915 [details] [diff] [review]
Pass folder to content provider and filter on query

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

Given that you'll need to keep track of the titles of the folders as you navigate up and down in the hierarchy (based on ibarlow's feedback), I guess this code will have to look a lot like the BookmarksPanel stuff. In which case, maybe the extra column for filtering doesn't necessarily need to be called 'path' or 'folder'. Maybe 'group' or 'filter' is a more appropriate name for the column?

Also, think about how we're going to handle folder URLs on views that don't support folder navigation e.g. the panel grid view.

::: mobile/android/base/home/DynamicPanel.java
@@ +168,4 @@
>       */
>      private class PanelDatasetHandler implements DatasetHandler {
>          @Override
> +        public void requestDataset(String datasetId, String folder) {

I feel like dataset+folder should be encapsulated in a class like DatasetRequest. In practice, this code should follow a very similar approach than the FolderInfo bits in BookmarksListAdapter/BookmarksPanel. This will allow you, for example, to implement the stuff that ibarlow describes in his feedback (comment #5) as you'll be able to store the title of the target 'folder' in the DatasetRequest too.

@@ +235,5 @@
> +                selection = HomeItems.DATASET_ID + " = ? AND " + HomeItems.FOLDER + " IS NULL";
> +                selectionArgs = new String[] { mDatasetId };
> +            } else {
> +                selection = HomeItems.DATASET_ID + " = ? AND " + HomeItems.FOLDER + " = ?";
> +                selectionArgs = new String[] { mDatasetId, mFolder };

You can probably use DBUtils.concatenateWhere() and DBUtils.appendSelectionArgs() to make this code a bit cleaner.

::: mobile/android/base/home/PanelListView.java
@@ +78,4 @@
>              int urlIndex = cursor.getColumnIndexOrThrow(HomeItems.URL);
>              final String url = cursor.getString(urlIndex);
>  
> +            if (StringUtils.isFolderUrl(url)) {

I'd prefer this to be centrally handled in PanelLayout instead of doing that per view (if possible) i.e. the views shouldn't need to explicitly handle folder URLs. You can achieve that by wrapping the OnUrlOpenListener and catching the onUrlOpen calls from the views before they go 'up' to BrowserApp.

::: mobile/android/base/home/PanelLoader.java
@@ +4,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +package org.mozilla.gecko.home;
> +
> +public interface PanelLoader {

Why not just using the existing DatasetHandler interface? If you really think this is necessary, I'd rename this to something less generic looking like FolderLoader or something.

	Lucas Rocha (:lucasr)
	Comment 8 • 10 years ago

Comment on attachment 8370916 [details] [diff] [review]
Add back button support for folder heirarchy

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

Looks fine but you'll need to handle the title too when navigating 'up'.

::: mobile/android/base/home/PanelListView.java
@@ +43,3 @@
>          mAdapter = new PanelListAdapter(context);
>          setAdapter(mAdapter);
>          setOnItemClickListener(new ClickListener());

nit: add empty line before the setOnItemClickListener() call

@@ +43,4 @@
>          mAdapter = new PanelListAdapter(context);
>          setAdapter(mAdapter);
>          setOnItemClickListener(new ClickListener());
> +

nit: remove this empty line here.

@@ +99,5 @@
> +            if (action == KeyEvent.ACTION_UP && keyCode == KeyEvent.KEYCODE_BACK && mFolderUrl != null) {
> +                mFolderUrl = StringUtils.getParentFolderUrl(mFolderUrl);
> +                mLoader.load(StringUtils.getFolderFromUrl(mFolderUrl));
> +                return true;
> +            }

nit: add empty line here.

	Josh Dover [:jdover] Assignee
	Comment 13 • 10 years ago

This uses MultiTypeCursorAdapter in a similar way that BookmarksListAdapter does. This functionality could probably be generalized into a base class (FolderListAdapter) but there is quite a bit of different functionality between the two and would take some refactoring work. Lucas, do you want me to do that in this bug, file a follow up, or is it worth it at all?

Here is a test build for ibarlow: https://drive.google.com/file/d/0BwqqgfwYmRg4d0NKWkJTM0V0M28/edit?usp=sharing

	Lucas Rocha (:lucasr)
	Comment 15 • 10 years ago

Comment on attachment 8372002 [details] [diff] [review]
Couple datasetId and filter into DatasetRequest

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

Not sure I follow. Why is this change necessary exactly?

	Lucas Rocha (:lucasr)
	Comment 16 • 10 years ago

Comment on attachment 8372000 [details] [diff] [review]
Pass folder to content provider and filter on query

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

Had a look at the last patch to get a better sense about the direction of things here. There's no special need to use 'folder' terminology here. You don't even need the notion of paths in the urls if you're going to track the stack of filters in PanelListAdapter anyway. In this case, filter is the equivalent of parentId in the BookmarksPanel code. So, here's roughly how this could work (let me know if you find any obvious flaw with the proposal):

1. Items in root 'folder' have a null value in their 'filter' column. The parent stack will have a single element with filter == null.
2. Folder X have url in the form of filter://SOMEFILTER
3. Children of Folder X have a SOMEFILTER value in their filter column.

Tapping on Folder X will:
1. Add Folder X title and target filter (the one from the url) in the parent stack
2. Re-request the dataset with same ID but different filter (SOMEFILTER in this case)

I'm using the term folder here just to be clear but I'm not sure this is the best term? I'd go with 'filter' but I'm open to suggestions. Whatever term we agree on here, it should be used consistently everywhere. For instance, this patch adds a 'filter' column to the DB schema but then uses 'folder' everywhere in the code which feels a bit confusing.

::: mobile/android/base/home/DynamicPanel.java
@@ +180,4 @@
>              }
>  
>              final Bundle bundle = new Bundle();
> +            bundle.putString(DATASET_ID, request.datasetId);

Make DatasetRequest a Parcelable and pass it in the bundle directly. See how we do it for FolderInfo in BookmarkListAdapter.

@@ +247,4 @@
>          @Override
>          public Loader<Cursor> onCreateLoader(int id, Bundle args) {
>              final String datasetId = args.getString(DATASET_ID);
> +            final String filter = args.getString(FILTER);

If you make DatasetRequest a Parcelable you can simply do:

final DatasetRequest request = (DatasetRequest) args.getParcelable(DATASET_REQUEST);

@@ +254,5 @@
>          }
>  
>          @Override
>          public void onLoadFinished(Loader<Cursor> loader, Cursor cursor) {
> +            final DatasetRequest request = ((PanelDatasetLoader) loader).getRequest();

I'm not keen about these nested castings as they are hard to read. I'd go with a more verbose yet clearer version:
final PanelDatasetLoader datasetLoader = (PanelDatasetLoader) loader;
final DatasetRequest request = datasetLoader.getRequest();

@@ +263,5 @@
>          }
>  
>          @Override
>          public void onLoaderReset(Loader<Cursor> loader) {
> +            final DatasetRequest request = ((PanelDatasetLoader) loader).getRequest();

Ditto. Maybe move this into a private method in PanelLoaderCallbacks and reuse it in onLoaderReset and onLoadFinished?

private DatasetRequest getRequestFromLoader(Loader<Cursor> loader) {
    final PanelDatasetLoader datasetLoader = (PanelDatasetLoader) loader;
    return datasetLoader.getRequest();
}

::: mobile/android/base/home/PanelLayout.java
@@ +253,5 @@
> +
> +    private class PanelUrlOpenListener implements OnUrlOpenListener {
> +        @Override
> +        public void onUrlOpen(String url, EnumSet<Flags> flags) {
> +            if (StringUtils.isFolderUrl(url)) {

isFilterUrl instead?

::: mobile/android/base/home/PanelListView.java
@@ +36,5 @@
>          mViewConfig = viewConfig;
>          mAdapter = new PanelListAdapter(context);
>          setAdapter(mAdapter);
> +
> +        setOnItemClickListener(new ClickListener());

This change is unrelated and should go in a separate patch. Whatever simplification you apply to PanelListView should also be done in PanelGridView.

	Lucas Rocha (:lucasr)
	Comment 17 • 10 years ago

Comment on attachment 8372001 [details] [diff] [review]
Add back button support for folder hierarchy

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

::: mobile/android/base/home/PanelListView.java
@@ +87,5 @@
> +
> +            // If the user hit the BACK key, try to move to the parent folder.
> +            if (action == KeyEvent.ACTION_UP && keyCode == KeyEvent.KEYCODE_BACK && mFolderUrl != null) {
> +                mFolderUrl = StringUtils.getParentFolderUrl(mFolderUrl);
> +                mLoader.load(StringUtils.getFolderFromUrl(mFolderUrl));

You don't need the StringUtils bits if you're going to track the stack of folder/filters in the adapter anyway.

	Lucas Rocha (:lucasr)
	Comment 18 • 10 years ago

Comment on attachment 8372003 [details] [diff] [review]
Display folder rows and up navigation

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

This patch kinda of assumes we'll a fixed way of representing folders which might not be what we want here. For instance, add-ons might want to define their folder icons through the image_url column. So, I think we should assume (for now at least) that the views will all be of the same type. We can evolve that into folder-specific views if we feel we need to.

::: mobile/android/base/home/PanelListView.java
@@ +44,2 @@
>          mAdapter = new PanelListAdapter(context);
> +        mAdapter.setOnFolderChangeListener(this);

nit: I usually prefer having private inner classes for private listeners to avoid breaking encapsulation i.e. exposing a public method unnecessarily. Just like we're doing with the ClickListener() here.

	Lucas Rocha (:lucasr)
	Comment 19 • 10 years ago

Let's try to stick with the simplest possible implementation (our 'MVP') without making too many assumptions around how this "could work" in the future.

So, as a starting point, what we want to implement here is a way to navigate up and down dataset filters with all items using the same view type (PanelListRow). A 'filter' being any string placed in the 'filter' column in the DB that can be triggered by a special type of URL with the format filter://SOMESTRING. That's all.

I'd like to get some input from margaret and mfinkle on the terminology here. I'm personally leaning towards 'filter' as general way for accessing subsets of datasets. The fact that we'll end up using it to represent 'folders' is secondary. Using 'folder' (either on the DB schema and UI code) feels too restrictive and prone to be abused for other purposes by add-on developers.

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 20 • 10 years ago

Comment 19 sounds a lot like what we were discussing at the feature work week. Using "filter" terminology and a special "filter://" URI sounded good to me, then and now. It's simple, but should allow us to get the MVP ready to go. It's also useful for more than simulating a tree hierarchy too.

	:Margaret Leibovic Reporter
	Comment 21 • 10 years ago

I also like the term "filter", since it's a better way to represent what our code is actually doing. An add-on could create a folder UI using this filter functionality, but it could also do something like a "sections" UI for different sections of a news publication (for this reason I like Lucas's suggestion to let the add-on specify the icon for "folder" rows - and if they want a folder icon that matches our bookmarks UI, they could actually use a drawable:// URI to get at that).

	:Margaret Leibovic Reporter
	Comment 22 • 10 years ago

Comment on attachment 8372004 [details] [diff] [review]
Testing patch

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

::: mobile/android/chrome/content/browser.js
@@ +8347,5 @@
> +Cu.import("resource://gre/modules/Task.jsm");
> +
> +Task.spawn(function() {
> +    let storage = HomeProvider.getStorage("joshdata");
> +    storage.deleteAll();

I know this is just a test, but you should put a yield before this deleteAll call :)

	Josh Dover [:jdover] Assignee
	Comment 23 • 10 years ago

(In reply to Lucas Rocha (:lucasr) from comment #18)
> Comment on attachment 8372003 [details] [diff] [review]
> Display folder rows and up navigation
> 
> Review of attachment 8372003 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch kinda of assumes we'll a fixed way of representing folders which
> might not be what we want here. For instance, add-ons might want to define
> their folder icons through the image_url column. So, I think we should
> assume (for now at least) that the views will all be of the same type. We
> can evolve that into folder-specific views if we feel we need to.

Generalizing this to just be filters makes sense. Should we use the add-on defined icon here for the back navigation, or should we use the same folder with the back arrow as we do in Bookmarks?

	:Margaret Leibovic Reporter
	Comment 24 • 10 years ago

(In reply to Josh Dover from comment #23)
> (In reply to Lucas Rocha (:lucasr) from comment #18)
> > Comment on attachment 8372003 [details] [diff] [review]
> > Display folder rows and up navigation
> > 
> > Review of attachment 8372003 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This patch kinda of assumes we'll a fixed way of representing folders which
> > might not be what we want here. For instance, add-ons might want to define
> > their folder icons through the image_url column. So, I think we should
> > assume (for now at least) that the views will all be of the same type. We
> > can evolve that into folder-specific views if we feel we need to.
> 
> Generalizing this to just be filters makes sense. Should we use the add-on
> defined icon here for the back navigation, or should we use the same folder
> with the back arrow as we do in Bookmarks?

If we are going to let add-ons define the icon for the rows, I think we should also let them define icons for the back arrow row. However, we'd have to figure out where they would specify that, since it wouldn't be part of the normal HomeProvider data.

Or maybe we should just have ibarlow come up with a more generic "up" or "back" icon for that.

	Josh Dover [:jdover] Assignee
	Comment 25 • 10 years ago

Made DatasetRequest a Parcelable and fixed nits.

	Josh Dover [:jdover] Assignee
	Comment 26 • 10 years ago

Now displays filter links the same as other rows, which allows addons to supply their own icon for filter links. Filter link rows do not display the URL for the filter (this could have confused users).

Also does not assume that filter navigation is necessarily a folder tree, simply provides back support by keeping track of a navigation stack. Currently, the back list item uses our default folder with the back button as the icon, until we decided on what to do from Comment 24.

	:Margaret Leibovic Reporter
	Comment 29 • 10 years ago

Comment on attachment 8373708 [details] [diff] [review]
Testing patch

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

::: mobile/android/base/resources/raw/fake_home_items.json
@@ +3,5 @@
>      "dataset_id": "fake-dataset",
>      "url": "http://example.com/first",
>      "title": "First Example",
> +    "description": "This is an example",
> +    "folder": "y"

The changes in here aren't actually used with your testing patch... if instead of editing browser.js you want to use items from here to test, you can switch this query to use the fake URI:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/DynamicPanel.java#225

And now that the autoInstall stuff landed, you can actually just write an add-on to test if you wanted, instead of needing to write a patch like this.

Here's the clobbered together add-on I use for testing:
https://github.com/leibovic/home-demo/blob/master/bootstrap.js

	Lucas Rocha (:lucasr)
	Comment 30 • 10 years ago

Comment on attachment 8373702 [details] [diff] [review]
Pass filter to content provider and use in query

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

Here's a partial review. We still need to sort out some higher-level aspects of how we manage the parent stack.

::: mobile/android/base/home/FramePanelLayout.java
@@ +33,5 @@
>          addView(mChildView);
>      }
>  
>      @Override
> +    public void load(String filter) {

The filter should not be associated with the layout. It's something associated

::: mobile/android/base/home/PanelLayout.java
@@ +72,5 @@
>      public interface DatasetBacked {
>          public void setDataset(Cursor cursor);
>      }
>  
> +    public static class DatasetRequest implements Parcelable {

You need to implement the associated Parcelable CREATOR to enable the Android framework to load a persisted instance using a ClassLoader. See, for example, how FolderInfo does it.

@@ +136,5 @@
>       * in response to a dataset request.
>       */
> +    public final void deliverDataset(DatasetRequest request, Cursor cursor) {
> +        Log.d(LOGTAG, "Delivering dataset: " + request.datasetId + ", with filter: " + request.filter);
> +        // TODO: pass filter to view

Doesn't the request contain the filter already? What's this comment about?

@@ +233,5 @@
>              }
>          }
>      }
>  
> +    private void maybeSetDataset(View view, DatasetRequest request, Cursor cursor) {

The request argument is not being used here.

::: mobile/android/base/util/StringUtils.java
@@ +141,5 @@
> +        if (TextUtils.isEmpty(url)) {
> +            return false;
> +        }
> +
> +        return url.indexOf(FILTER_URL_PREFIX) == 0;

startsWith() is a bit easier to read.

@@ +149,5 @@
> +        if (TextUtils.isEmpty(url)) {
> +            return null;
> +        }
> +
> +        return url.replace(FILTER_URL_PREFIX, "");

I'd go with something slightly more efficient like: url.substring(FILTER_URL_PREFIX.length()).

@@ +152,5 @@
> +
> +        return url.replace(FILTER_URL_PREFIX, "");
> +    }
> +
> +    public static String getParentFilterUrl(String filterUrl) {

Unused, remove?

	Lucas Rocha (:lucasr)
	Comment 31 • 10 years ago

Comment on attachment 8373705 [details] [diff] [review]
Display filter up navigation

Clearing review request after discussion. We now have a tentative plan of action.

	Josh Dover [:jdover] Assignee
	Comment 34 • 10 years ago

These patches take the approach we talked about earlier today, keeping the filter stack logic contained within the layout, which allows the layout to handle all URL change and back events, keeping the views "dumb" about filters.

	Josh Dover [:jdover] Assignee
	Comment 35 • 10 years ago

Didn't notice your comments in Comment 30, will have to address those tomorrow.

	Lucas Rocha (:lucasr)
	Comment 36 • 10 years ago

Comment on attachment 8375231 [details] [diff] [review]
Add filter to dataset request

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

Looking good overall. Still needs some tweaks.

::: mobile/android/base/home/DynamicPanel.java
@@ +6,1 @@
>  package org.mozilla.gecko.home;

All changes to DynamicPanel look correct.

@@ +169,5 @@
>       */
>      private class PanelDatasetHandler implements DatasetHandler {
>          @Override
> +        public void requestDataset(DatasetRequest request) {
> +            Log.d(LOGTAG, "Requesting request: " + request.toString());

You don't need to call toString() explicitly here.

::: mobile/android/base/home/FramePanelLayout.java
@@ +37,5 @@
>      public void load() {
>          Log.d(LOGTAG, "Loading");
>  
>          if (mChildView instanceof DatasetBacked) {
> +            // TODO: get filter from ViewEntry

File a bug to extend the add-on API to allow views to define an initial filter string. Something like:

{ type: Home.panels.View.LIST,
  dataset: "somedatasetid",
  initialFilter: "somefilter" }

@@ +39,5 @@
>  
>          if (mChildView instanceof DatasetBacked) {
> +            // TODO: get filter from ViewEntry
> +            DatasetRequest request = new DatasetRequest(mChildConfig.getDatasetId(), null);
> +            Log.d(LOGTAG, "Requesting child request: " + request.toString());

No need to call toString() explicitly here.

::: mobile/android/base/home/PanelLayout.java
@@ +104,5 @@
> +                return new DatasetRequest[size];
> +            }
> +        };
> +
> +        private DatasetRequest(Parcel in) {

nit: move this up to be the first declared constructor.

@@ +147,5 @@
>       * in response to a dataset request.
>       */
> +    public final void deliverDataset(DatasetRequest request, Cursor cursor) {
> +        Log.d(LOGTAG, "Delivering request: " + request.toString());
> +        updateViewsWithDataset(request, cursor);

Does updateViewsWithDataset() actually need a DatasetRequest instance? It seems it can remain unchanged. Just pass request.getDatasetId() to it.

@@ +156,5 @@
>       * existing panel views.
>       */
>      public final void releaseDataset(String datasetId) {
>          Log.d(LOGTAG, "Resetting dataset: " + datasetId);
> +        updateViewsWithDataset(new DatasetRequest(datasetId, null), null);

Looks like code smell. You're creating a new object just to conform with the new API. In theory, datasetId should enough to update views as they shouldn't really need to track which filter is associated with the delivered cursor.

::: mobile/android/base/util/StringUtils.java
@@ +7,5 @@
>  
>  import android.net.Uri;
>  import android.text.TextUtils;
>  
>  public class StringUtils {

nit: add empty line here.

@@ +8,5 @@
>  import android.net.Uri;
>  import android.text.TextUtils;
>  
>  public class StringUtils {
> +    private static final String FILTER_URL_PREFIX = "filter://";

nit: add empty line here.

@@ +141,5 @@
> +        if (TextUtils.isEmpty(url)) {
> +            return false;
> +        }
> +
> +        return url.indexOf(FILTER_URL_PREFIX) == 0;

Use startsWith() instead.

@@ +149,5 @@
> +        if (TextUtils.isEmpty(url)) {
> +            return null;
> +        }
> +
> +        return url.replace(FILTER_URL_PREFIX, "");

Use substring(FILTER_URL_PREFIX.size()) instead.

	Lucas Rocha (:lucasr)
	Comment 37 • 10 years ago

Comment on attachment 8375232 [details] [diff] [review]
Refactor ViewEntry to track filter stack

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

The direction looks generally right.

::: mobile/android/base/home/FramePanelLayout.java
@@ +38,5 @@
>          Log.d(LOGTAG, "Loading");
>  
>          if (mChildView instanceof DatasetBacked) {
> +            String filter = mViewDetailMap.get(mChildView).getFilter();
> +            DatasetRequest request = new DatasetRequest(mChildConfig.getDatasetId(), filter);

The initial filter (used in the layout's load() call) shouldn't really come from the layout but from the ViewConfig instance.

::: mobile/android/base/home/PanelLayout.java
@@ +138,5 @@
>      }
>  
>      public PanelLayout(Context context, PanelConfig panelConfig, DatasetHandler datasetHandler, OnUrlOpenListener urlOpenListener) {
>          super(context);
> +        mViewDetailMap = new HashMap<View, ViewDetail>();

Maybe this should be a WeakHashMap to avoid leaking views?

@@ +217,5 @@
>       * given view.
>       */
>      protected final void disposePanelView(View view) {
>          Log.d(LOGTAG, "Disposing panel view");
> +        final int count = mViewDetailMap.size();

unused?

@@ +235,5 @@
>  
>              // Update any views associated with the given dataset ID
> +            if (TextUtils.equals(detail.getDatasetId(), request.datasetId)) {
> +                final View view = entry.getKey();
> +                Log.d("Boo", "entry: " + entry.toString());

Boo! :-)

@@ +259,5 @@
>      /**
>       * Represents a 'live' instance of a panel view associated with
>       * the {@code PanelLayout}.
>       */
> +    protected static class ViewDetail {

Why not keeping the ViewEntry class name? Just trying to understand the intent here.

@@ +272,5 @@
>          public String getDatasetId() {
>              return mViewConfig.getDatasetId();
>          }
> +
> +        /** Used to find the current filter that this view is displaying, or null if none */

Indent/format this comment by wrapping it in multiple lines.

@@ +273,5 @@
>              return mViewConfig.getDatasetId();
>          }
> +
> +        /** Used to find the current filter that this view is displaying, or null if none */
> +        public String getFilter() {

getFilter() -> getCurrentFilter()

	Josh Dover [:jdover] Assignee
	Comment 38 • 10 years ago

(In reply to Lucas Rocha (:lucasr) from comment #37)
> Comment on attachment 8375232 [details] [diff] [review]
> Refactor ViewEntry to track filter stack
> 
> Review of attachment 8375232 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The direction looks generally right.
> 
> ::: mobile/android/base/home/FramePanelLayout.java
> @@ +38,5 @@
> >          Log.d(LOGTAG, "Loading");
> >  
> >          if (mChildView instanceof DatasetBacked) {
> > +            String filter = mViewDetailMap.get(mChildView).getFilter();
> > +            DatasetRequest request = new DatasetRequest(mChildConfig.getDatasetId(), filter);
> 
> The initial filter (used in the layout's load() call) shouldn't really come
> from the layout but from the ViewConfig instance.

I think it makes more sense to set this when we create the ViewDetail on line 208 of PanelLayout.java. I added a todo to be fixed in bug 972503.

> 
> @@ +259,5 @@
> >      /**
> >       * Represents a 'live' instance of a panel view associated with
> >       * the {@code PanelLayout}.
> >       */
> > +    protected static class ViewDetail {
> 
> Why not keeping the ViewEntry class name? Just trying to understand the
> intent here.

Now that this class wasn't holding the actual View, I thought ViewEntry didn't make sense as it sounds like a wrapper around a view, while ViewDetail describes it.

	Lucas Rocha (:lucasr)
	Comment 42 • 10 years ago

Comment on attachment 8375725 [details] [diff] [review]
Add filter to dataset request

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

Awesome.

::: mobile/android/base/home/PanelLayout.java
@@ +96,5 @@
> +            dest.writeString(filter);
> +        }
> +
> +        public String toString() {
> +            return "{dataset: " + datasetId + ", " + filter + "}";

Maybe do ... + "filter: " + filter + "}" for clarity in the logs?

::: mobile/android/base/util/StringUtils.java
@@ +138,5 @@
>  
>          return null;
>      }
> +
> +    public static boolean isFilterUrl(String url) {

Given that you're not using any of these new StringUtils APIs in this patch, maybe you should move this change to the patch that calls them.

	Lucas Rocha (:lucasr)
	Comment 43 • 10 years ago

Comment on attachment 8375726 [details] [diff] [review]
Refactor ViewEntry to track filter stack

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

Looking pretty good, needs some extra tweaks.

::: mobile/android/base/home/FramePanelLayout.java
@@ +38,5 @@
>          Log.d(LOGTAG, "Loading");
>  
>          if (mChildView instanceof DatasetBacked) {
> +            String filter = mViewDetailMap.get(mChildView).getCurrentFilter();
> +            DatasetRequest request = new DatasetRequest(mChildConfig.getDatasetId(), filter);

You should only really change this once you implement bug 972503. The load() call always means you'll be using the view's initial filter. No need to peek at the filter stack at all. This will look like this:

DatasetRequest request = new DatasetRequest(mChildConfig.getDatasetId(), mChildConfig.getInitialFilter());

So, in the context of this bug, just leave it as null.

::: mobile/android/base/home/PanelLayout.java
@@ +204,5 @@
>                  throw new IllegalStateException("Unrecognized view type in " + getClass().getSimpleName());
>          }
>  
> +        final ViewDetail detail = new ViewDetail(viewConfig);
> +        // TODO: Push initial filter here onto ViewDetail

See my comment in FramePanelLayout. The initial filter should always come from the associated ViewConfig instance.

@@ +258,5 @@
>      /**
>       * Represents a 'live' instance of a panel view associated with
>       * the {@code PanelLayout}.
>       */
> +    protected static class ViewDetail {

Looking at this code now, I feel this should actually be named ViewState instead.

@@ +274,5 @@
> +
> +        /**
> +         * Used to find the current filter that this view is displaying, or null if none.
> +         */
> +        public String getCurrentFilter() {

Just for future reference, this will become something like this once you implement bug 972503:

public String getCurrentFilter() {
   if (mFilterStack.isEmpty()) {
       return mViewConfig.getInitialFilter();
   }

   return mFilterStack.peek();
}

	Lucas Rocha (:lucasr)
	Comment 44 • 10 years ago

Comment on attachment 8375728 [details] [diff] [review]
Change filters when an item with filter URL is clicked

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

Looks great but can be a lot simpler.

::: mobile/android/base/home/PanelLayout.java
@@ +291,5 @@
> +            throw new UnsupportedOperationException("PanelLayout must supply the child view that opened the url");
> +        }
> +
> +        @Override
> +        public void onUrlOpenFromView(String url, EnumSet<Flags> flags, View view) {

When I suggested that you'd have to associate a View with the OnUrlOpenListener, I didn't meant it in the API level but on instance level. What you need here is a lot simpler. Just add a private 'ViewConfig' member to PanelUrlOpenListener and instantiate it ((in createPanelView) like this:

((PanelView) view).setOnUrlOpenListener(new PanelUrlOpenListener(viewConfig));

@@ +294,5 @@
> +        @Override
> +        public void onUrlOpenFromView(String url, EnumSet<Flags> flags, View view) {
> +            if (StringUtils.isFilterUrl(url)) {
> +                mViewDetailMap.get(view).pushFilter(StringUtils.getFilterFromUrl(url));
> +                load();

So this will become:

final String filter = StringUtils.getFilterFromUrl(url);
mViewDetailMap.get(view).pushFilter(filter);
// Uses the ViewConfig private member I mentioned above:
mDatasetHandler.requestDataset(new DatasetRequest(mViewConfig.getDatasetId(), filter));

Remember, the load() is only supposed to be used when the fragment is first displayed in HomePager. Furthermore, load() is a layout-wide call but you only want to scope the request to the specific view that triggered this URL.

I'd probably add all this code into a private method like pushFilterOnView(View v) to make this code a bit cleaner.

If you what I describe above, you won't need to do any API changes to OnUrlOpenListener. Win.

	Lucas Rocha (:lucasr)
	Comment 45 • 10 years ago

Forgot to mention: please make sure you update/add in-code comments and documentation to match the new internal APIs (method signatures, the new DatasetRequest class, etc).

	Lucas Rocha (:lucasr)
	Comment 49 • 10 years ago

Comment on attachment 8377759 [details] [diff] [review]
Refactor ViewEntry to track filter stack

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

Nice.

::: mobile/android/base/home/PanelLayout.java
@@ +271,2 @@
>              mViewConfig = viewConfig;
> +            mFilterStack = new LinkedList<String>();

It would be nice if we only created this data structure when the view pushes a filter for the first time as most views will not be using the filter stack anyway. We can do this in a follow-up if you prefer.

@@ +279,5 @@
> +        /**
> +         * Used to find the current filter that this view is displaying, or null if none.
> +         */
> +        public String getCurrentFilter() {
> +            return mFilterStack.peek();

I assume peek() doesn't throw and simply returns null if the stack is empty?

@@ +283,5 @@
> +            return mFilterStack.peek();
> +        }
> +
> +        /**
> +         * Add a filter to the history stack for this view.

nit: Adds

	Lucas Rocha (:lucasr)
	Comment 50 • 10 years ago

Comment on attachment 8377760 [details] [diff] [review]
Change filters when an item with filter URL is clicked

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

Nice.

::: mobile/android/base/home/PanelLayout.java
@@ +311,5 @@
> +                mUrlOpenListener.onUrlOpen(url, flags);
> +            }
> +        }
> +
> +        private void pushFilterOnView(String filter) {

nit: I have a slight preference for having pushFilterOnView(ViewState, filter) as a private method in PanelLayout.

	Josh Dover [:jdover] Assignee
	Comment 51 • 10 years ago

Fixed nits

	Josh Dover [:jdover] Assignee
	Comment 52 • 10 years ago

Fixed nits

	Josh Dover [:jdover] Assignee
	Comment 53 • 10 years ago

I was able to implement this very similarly to the url listener inside of PanelLayout

	Lucas Rocha (:lucasr)
	Comment 54 • 10 years ago

Comment on attachment 8377840 [details] [diff] [review]
Use back button to pop filter stack

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

Nice. Just needs to let the keyevent through if there's no filter to pop.

::: mobile/android/base/home/PanelLayout.java
@@ +302,5 @@
> +
> +        public String popFilter() {
> +            if (getCurrentFilter() != null) {
> +                mFilterStack.pop();
> +            }

nit: add empty line here.

@@ +351,5 @@
> +
> +        @Override
> +        public boolean onKey(View v, int keyCode, KeyEvent event) {
> +            if (event.getAction() == KeyEvent.ACTION_UP && keyCode == KeyEvent.KEYCODE_BACK) {
> +                popFilterOnView(mViewState);

Shouldn't this return false if there's no filter to pop?

	Josh Dover [:jdover] Assignee
	Comment 55 • 10 years ago

(In reply to Lucas Rocha (:lucasr) from comment #54)
> @@ +351,5 @@
> > +
> > +        @Override
> > +        public boolean onKey(View v, int keyCode, KeyEvent event) {
> > +            if (event.getAction() == KeyEvent.ACTION_UP && keyCode == KeyEvent.KEYCODE_BACK) {
> > +                popFilterOnView(mViewState);
> 
> Shouldn't this return false if there's no filter to pop?

Good catch.

	Lucas Rocha (:lucasr)
	Comment 56 • 10 years ago

Comment on attachment 8378464 [details] [diff] [review]
Use back button to pop filter stack

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

Awesome. We should probably build a test add-on to exercise the filter stack. Please file a follow-up to work on UI part.

::: mobile/android/base/home/PanelLayout.java
@@ +319,5 @@
>  
>      /**
> +     * Pops filter from {@code ViewState}'s stack and makes request for previous filter value.
> +     *
> +     * @return whether filter changed

nit: whether the filter has changed

	:Margaret Leibovic Reporter
	Comment 57 • 10 years ago

(In reply to Lucas Rocha (:lucasr) from comment #56)
> Comment on attachment 8378464 [details] [diff] [review]
> Use back button to pop filter stack
> 
> Review of attachment 8378464 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Awesome. We should probably build a test add-on to exercise the filter
> stack. Please file a follow-up to work on UI part.

If you want to work a real-world example, you could help contribute to my dropbox add-on:
https://github.com/leibovic/dropbox-panel

Here are some API docs:
https://www.dropbox.com/developers/core/docs#files-GET

	Josh Dover [:jdover] Assignee
	Comment 58 • 10 years ago

Fixed comment & filed follow up for UI: bug 975055. 

Would you like me to make finish the dropbox add on before landing this?

	Lucas Rocha (:lucasr)
	Comment 59 • 10 years ago

(In reply to Josh Dover from comment #58)
> Created attachment 8379178 [details] [diff] [review]
> Use back button to pop filter stack
> 
> Fixed comment & filed follow up for UI: bug 975055. 
> 
> Would you like me to make finish the dropbox add on before landing this?

Nope, let's land this and work on the add-on in parallel.

	Ryan VanderMeulen [:RyanVM]
	Comment 60 • 10 years ago

At least the first the patch needs rebasing. Didn't get further than that.

	Ryan VanderMeulen [:RyanVM]
	Comment 62 • 10 years ago

https://hg.mozilla.org/integration/fx-team/rev/0106724dcbfd

	Carsten Book [:Tomcat]
	Comment 63 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/0106724dcbfd

	:Margaret Leibovic Reporter
	Comment 64 • 10 years ago

Setting P1 hub bugs to block hub v1 EPIC bug (targeting fx30 release).

Filter on epic-hub-bugs.