975055 - Show current filter and back UI for Hub panels

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect, P1)

Description

[Josh Dover [:jdover]]

As a user navigates through filters, the UI should display the current filter they are in as well as a way to go back (other that just using the back button). This may be similar to the bookmarks panel UI.

Comment 1

[Lucas Rocha (:lucasr)]

I implemented a super-simple add-on that has uses the filter stack stuff btw. We can use it to test whatever UI changes are done in this bug.

https://github.com/lucasr/fennec-folders-panel

Comment 2

[Josh Dover [:jdover]]

Attachment: 01 - Show current filter / back list row

This patch uses a similar approach to MultiTypeCursorAdapter and BookmarksListAdapter to provide a header row with name of the current filter that acts as a back button.

I did not implement this as a subtype of MultiTypeCursorAdapter due to the way PanelItemView is currently being initialized with a static 'create' method which inflates its own view. This could be changed though that be out of the scope of this bug.

Comment 3

[Josh Dover [:jdover]]

Attachment: 02 - Keep filter title in stack for header row

This patch couples the Title of the filter item with the filter in the ViewState's stack so that the adapter can display the name assigned to the filter rather than the filter itself (which should be hidden from the user).

Comment 4

[Lucas Rocha (:lucasr)]

Comment on attachment 8380974 [details] [diff] [review]
01 - Show current filter / back list row

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

I like the general direction here. For now, I think I've given enough feedback for a new iteration on this patch.

::: mobile/android/base/home/PanelLayout.java
@@ +7,5 @@
>  
>  import org.mozilla.gecko.home.HomePager.OnUrlOpenListener;
>  import org.mozilla.gecko.home.HomeConfig.PanelConfig;
>  import org.mozilla.gecko.home.HomeConfig.ViewConfig;
> +import org.mozilla.gecko.home.PanelViewAdapter.FilterManager;

It seems you forgot to include FilterManager.java in your patch. In any case, I think you should move the FilterManager interface as an inner interface in PanelLayout (just like PanelView).

@@ +141,5 @@
>      }
>  
>      public interface PanelView {
>          public void setOnUrlOpenListener(OnUrlOpenListener listener);
> +        public void setFilterManager(FilterManager manager);

Filters only make sense in the context of datasets. So we should only set a filtermanager if the panel view implements DatasetBacked.

@@ +377,5 @@
>          }
>      }
> +
> +    private class PanelFilterManager implements FilterManager {
> +        private ViewState mViewState;

final

@@ +384,5 @@
> +            mViewState = viewState;
> +        }
> +
> +        @Override
> +        public String getFilter() {

Use getCurrentFilter() too?

::: mobile/android/base/home/PanelListView.java
@@ +53,3 @@
>      private class PanelListItemClickListener implements AdapterView.OnItemClickListener {
>          @Override
>          public void onItemClick(AdapterView<?> parent, View view, int position, long id) {

This code has been moved to a separate class called PanelViewUrlHandler. You'll have to rebase.

@@ +53,4 @@
>      private class PanelListItemClickListener implements AdapterView.OnItemClickListener {
>          @Override
>          public void onItemClick(AdapterView<?> parent, View view, int position, long id) {
> +            if (mFilterManager.getFilter() != null) {

This semantics will probably break if the initial filter (defined by the view as proposed in bug 972503) is not null. Maybe this logic should be implemented behind a clearer API such as canGoBack()?

::: mobile/android/base/home/PanelViewAdapter.java
@@ +23,5 @@
>  	private final Context mContext;
>  	private final ItemType mItemType;
> +    private FilterManager mFilterManager;
> +
> +    public interface FilterManager {

This interface belongs to PanelLayout which is where we manage filter stacks anyway.

@@ +69,5 @@
> +    }
> +
> +    private View newView(Context context, int position, ViewGroup parent) {
> +        if (getItemViewType(position) == VIEW_TYPE_HEADER) {
> +            BookmarkFolderView view = (BookmarkFolderView) LayoutInflater.from(context)

Please ask ibarlow for a proper header design here. Let's only share code if it actually makes sense here. For instance, I expect this header to use the image URL defined in the database, in which case it won't make sense to BookmarkFolderView.

@@ +94,5 @@
> +        }
> +    }
> +
> +    private boolean isShowingHeader() {
> +        return mFilterManager != null ? mFilterManager.getFilter() != null : false;

Same here, I think this getFilter() != null should be something like canGoBack()

Comment 5

[Lucas Rocha (:lucasr)]

Comment on attachment 8380976 [details] [diff] [review]
02 - Keep filter title in stack for header row

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

I like it. You'll just need to rebase to account for PanelViewUrlHandler and sort out a cleaner way to represent the 'go-back item' in the adapter. I'd probably avoid using the term 'header' in PanelViewAdapter as it implies a specific kind of UI that might not apply to things like grids or something else.

::: mobile/android/base/home/PanelLayout.java
@@ +315,5 @@
>      }
>  
> +    static class FilterDetail {
> +        final String filter;
> +        final String title;

Make both public.

@@ +319,5 @@
> +        final String title;
> +
> +        public FilterDetail(String filter, String title) {
> +            this.filter = filter;
> +            this.title = title;

You'll need the image URL too.

@@ +368,1 @@
>              if (StringUtils.isFilterUrl(url)) {

You'll have to restructure this a bit to account for the new PanelViewUrlHandler. Maybe you'll have rename it to PanelViewItemHandler and change it to take a OnItemOpenListener instead of an OnUrlOpenListener?

::: mobile/android/base/home/PanelViewAdapter.java
@@ +82,5 @@
>  
>      private void bindView(View view, int position) {
>          if (getItemViewType(position) == VIEW_TYPE_HEADER) {
>              final BookmarkFolderView row = (BookmarkFolderView) view;
> +            row.setText(mFilterManager.getFilter().title);

We should try to just use a PanelItemView for the 'header' and use a CursorWrapper that spits the Filter detail as its first row? You'd then add a new type of PanelItemView that shows ArticleItemView plus some arrow on top or something?

Comment 6

[Josh Dover [:jdover]]

Attachment: 01 - Show current filter / back list row

Rebased to integrate with changes to PanelViewUrlHandler and fixed your comments.

Comment 7

[Josh Dover [:jdover]]

Attachment: 02 - Keep filter title in stack for header row

Did the cursor wrapper work in the next patch, could possibly squash these together?

Comment 8

[Josh Dover [:jdover]]

Attachment: 03 - Show up arrow for filter back row

Comment 9

[Josh Dover [:jdover]]

Attachment: Screenshot_2014-02-27-16-52-53.png

I think it's still a bit confusing that the row for going back looks mostly identical to the other rows. I think we should think about bolding the text and/or giving that row a grey/blue background to help distinguish it from the others.

Comment 10

[Lucas Rocha (:lucasr)]

Comment on attachment 8383393 [details] [diff] [review]
01 - Show current filter / back list row

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

Nice, just needs some more tweaks.

::: mobile/android/base/home/PanelGridView.java
@@ +58,5 @@
>          mUrlHandler.setOnUrlOpenListener(listener);
>      }
>  
> +    @Override
> +    public void setFilterManager(FilterManager manager) {

manager -> filterManager

@@ +59,5 @@
>      }
>  
> +    @Override
> +    public void setFilterManager(FilterManager manager) {
> +        mAdapter.setFilterManager(manager);

Set the FilterManager on the Url handler too? Just like in PanelListView.

::: mobile/android/base/home/PanelLayout.java
@@ +226,4 @@
>          view.setOnKeyListener(new PanelKeyListener(state));
>  
> +        if (view instanceof DatasetBacked) {
> +            ((DatasetBacked) view).setFilterManager(new PanelFilterManager(state));

I'm not a fan of these inline type castings because they're not very readable but I can live with it.

@@ +398,5 @@
> +
> +        @Override
> +        public boolean canGoBack() {
> +            // TODO: Return false if the current filter is the initial filter
> +            return getCurrentFilter() != null;

nit: enclose expression with ()'s

::: mobile/android/base/home/PanelListView.java
@@ +28,5 @@
>  
>      private final PanelViewAdapter mAdapter;
>      private final ViewConfig mViewConfig;
>      private final PanelViewUrlHandler mUrlHandler;
> +    private FilterManager mFilterManager;

Unused, remove.

@@ +55,5 @@
>          mUrlHandler.setOnUrlOpenListener(listener);
>      }
>  
> +    @Override
> +    public void setFilterManager(FilterManager manager) {

manager -> filterManager for extra clarity.

::: mobile/android/base/home/PanelViewAdapter.java
@@ +24,2 @@
>  	private final Context mContext;
>  	private final ItemType mItemType;

hmm, fix this indentation on a separate patch?

@@ +67,5 @@
> +    private View newView(Context context, int position, ViewGroup parent) {
> +        if (getItemViewType(position) == VIEW_TYPE_BACK) {
> +            BookmarkFolderView view = (BookmarkFolderView) LayoutInflater.from(context)
> +                .inflate(R.layout.bookmark_folder_row, parent, false);
> +            view.open();

We'll definitely need to change this view to allow add-ons to define the looks of their up/back item. So I'd like to avoid reusing BookmarkFolderView in the long-term. File a follow-up bug to implement how add-ons will define their panel up/back item. This will probably involve replacing BookmarkFolderView here with a new custom view (PanelBackItemView?).

@@ +84,5 @@
> +                position--;
> +            }
> +
> +            final Cursor cursor = getCursor(position);
> +            final PanelItemView row = (PanelItemView) view;

row -> item

@@ +90,5 @@
> +        }
> +    }
> +
> +    private boolean isShowingBack() {
> +        return mFilterManager != null ? mFilterManager.canGoBack() : false;

nit: enclose conditional with ()'s

::: mobile/android/base/home/PanelViewUrlHandler.java
@@ +35,1 @@
>      public void openUrlAtPosition(Cursor cursor, int position) {

Add comment explaining what this code is handling here.

@@ +35,2 @@
>      public void openUrlAtPosition(Cursor cursor, int position) {
> +        if (mFilterManager.canGoBack()) {

Maybe we should not assume filter manager is not null here to make this code a bit more future-proof. So, do:

if (mFilterManager != null && mFilterManager.canGoBack()) {
    ...
}

Comment 11

[Ian Barlow (:ibarlow)]

(In reply to Josh Dover from comment #9)
> Created attachment 8383401 [details]
> Screenshot_2014-02-27-16-52-53.png
> 
> I think it's still a bit confusing that the row for going back looks mostly
> identical to the other rows. I think we should think about bolding the text
> and/or giving that row a grey/blue background to help distinguish it from
> the others.

Perhaps -- before you try that though I'd like to add "Up to" in front of the title of the parent directory.

Comment 12

[Josh Dover [:jdover]]

(In reply to Ian Barlow (:ibarlow) from comment #11)
> (In reply to Josh Dover from comment #9)
> > Created attachment 8383401 [details]
> > Screenshot_2014-02-27-16-52-53.png
> > 
> > I think it's still a bit confusing that the row for going back looks mostly
> > identical to the other rows. I think we should think about bolding the text
> > and/or giving that row a grey/blue background to help distinguish it from
> > the others.
> 
> Perhaps -- before you try that though I'd like to add "Up to" in front of
> the title of the parent directory.

Ian here is a build with the "up to" rows: https://drive.google.com/a/mozilla.com/file/d/0BwqqgfwYmRg4OGZWR0w1MVByTjA/edit?usp=sharing
And an addon that let's you try it out: https://drive.google.com/file/d/0BwqqgfwYmRg4N19pcEdUUEZKR3M/edit?usp=sharing

Comment 13

[Ian Barlow (:ibarlow)]

Josh, I can't seem to install this add-on... Any suggestions?

Comment 14

[Lucas Rocha (:lucasr)]

Comment on attachment 8383394 [details] [diff] [review]
02 - Keep filter title in stack for header row

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

Looking pretty good. Just needs another round of fixes/tweaks.

::: mobile/android/base/home/PanelLayout.java
@@ +141,5 @@
>      }
>  
>      public interface PanelView {
> +        public void setOnItemOpenListener(OnItemOpenListener listener);
> +        public void setOnKeyListener(OnKeyListener listener);

Why did you need to add setOnKeyListener() to the PanelView interface? It's a bit redundant given that every PanelView is, well, a View anyway.

@@ +352,2 @@
>  
> +        if (current != previous) {

Maybe add an equals() method to FilterDetail that not only checks if it's the same instance but also if the instances have the same filter string?

@@ +352,5 @@
>  
> +        if (current != previous) {
> +            mDatasetHandler.requestDataset(new DatasetRequest(
> +                viewState.getDatasetId(),
> +                current == null ? null : current.filter));

nit: this indentation looks really weird :-) Maybe declare local variables to make it shorter?

final String datasetId = viewState.getDatasetId();
final String filter = (current != null ? null : current.filter);
mDatasetHandler.requestDataset(new DatasetRequest(datasetId, filter));

@@ +360,5 @@
>          }
>      }
>  
> +    public interface OnItemOpenListener {
> +        public void onItemOpen(String url, String title, EnumSet<OnUrlOpenListener.Flags> flags);

Using Flags from OnUrlOpenListener in OnItemOpenListener feels wrong. Just move the item handler bits from PanelViewItemHandler to PanelOnItemOpenListener.

::: mobile/android/base/home/PanelListView.java
@@ +42,5 @@
>          setAdapter(mAdapter);
>  
>          setOnItemClickListener(new PanelListItemClickListener());
>      }
>  

Copy onDetachedFromWindow() from PanelGridView here.

::: mobile/android/base/home/PanelViewAdapter.java
@@ +77,5 @@
>      }
>  
>      private void bindView(View view, int position) {
>          if (getItemViewType(position) == VIEW_TYPE_BACK) {
>              final BookmarkFolderView row = (BookmarkFolderView) view;

row -> backItem

Comment 15

[Lucas Rocha (:lucasr)]

Comment on attachment 8383395 [details] [diff] [review]
03 - Show up arrow for filter back row

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

Before I review this, has ibarlow decided to show the image of the clicked item as the 'up' image too? I thought we were considering having a fixed 'up' image defined by the add-on?

Comment 16

[Josh Dover [:jdover]]

Attachment: 01 - Use ViewState to get previous filter for UI

I have reordered and split these commits to make more sense from a review standpoint. There were changes in the design that were reflected later in the commits that outdated some of the work done in previous commits, this should fix that.

Comment 17

[Josh Dover [:jdover]]

Attachment: 02 - Show previous filter for back row item

Comment 18

[Josh Dover [:jdover]]

Attachment: 03 - Keep filter title in stack for header row

> Why did you need to add setOnKeyListener() to the PanelView interface? It's
> a bit redundant given that every PanelView is, well, a View anyway.

I think this makes it more consistent and obvious that this type needs this listener as they are tied together. I know it is redundant, but as View isn't an interface, I can't explicitly extend it or specify that PanelView's are also Views (except for the implication in the name). This makes things more consistent and type-safe.

Comment 19

[Josh Dover [:jdover]]

Attachment: 04 - Show up arrow for back row

> Before I review this, has ibarlow decided to show the image of the clicked
> item as the 'up' image too? I thought we were considering having a fixed
> 'up' image defined by the add-on?

I was a bit unclear on this part. Should the custom "back" icon/image be specified at the item level, or at the add-on level? Or do we want this at all?

Comment 20

[Ian Barlow (:ibarlow)]

(In reply to Josh Dover from comment #19)
> Created attachment 8385695 [details] [diff] [review]
> 04 - Show up arrow for back row
> 
> > Before I review this, has ibarlow decided to show the image of the clicked
> > item as the 'up' image too? I thought we were considering having a fixed
> > 'up' image defined by the add-on?
> 
> I was a bit unclear on this part. Should the custom "back" icon/image be
> specified at the item level, or at the add-on level? Or do we want this at
> all?

Sorry, I'm not sure I understand your question. If you're asking where the 'back' icon should come from, I think Firefox should provide a generic default one, but add-ons should have the ability to change it if desired.

If you're asking something different, let me know :)

Comment 21

[Lucas Rocha (:lucasr)]

Comment on attachment 8385689 [details] [diff] [review]
01 - Use ViewState to get previous filter for UI

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

This is a bit confusing. You're only using the previous filter to check whether you can pop or not. You're actually need the value. So, here's what I suggest:

Change popFilter() to return a boolean. Something like this:

// true if it has popped an item, false otherwise
public boolean popFilter() {
    if (mFilterStack == null || mFilterStack.size() <= 1) {
        return false;
    }

    mFilterStack.pop();
    return true;
}

More suggestions below.

::: mobile/android/base/home/PanelLayout.java
@@ -283,2 @@
>           */
> -        public String getCurrentFilter() {

Keep getCurrentFilter() as is.

@@ -293,5 @@
>           * Adds a filter to the history stack for this view.
>           */
>          public void pushFilter(String filter) {
> -            if (mFilterStack == null) {
> -                mFilterStack = new LinkedList<String>();

Keep this code and add the 'null' filter here (which will become the view's initial filter later)

@@ +308,2 @@
>          public String popFilter() {
> +            if (getPreviousFilter() != null) {

Replace with canPopFilter()

@@ +328,5 @@
>       * @return whether the filter has changed
>       */
>      private boolean popFilterOnView(ViewState viewState) {
> +        String previous = viewState.getPreviousFilter();
> +        String current = viewState.popFilter();

Do this instead:

if (viewState.popFilter()) {
    final String current = viewState.getCurrentFilter();
    mDatasetHandler.requestDataset(new DatasetRequest(viewState.getDatasetId(), current));
    return true;
} else {
    return false;
}

Comment 22

[Lucas Rocha (:lucasr)]

Comment on attachment 8385690 [details] [diff] [review]
02 - Show previous filter for back row item

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

Given that the back item will be defined by the add-on, we should approach this a bit differently. Create a separate custom view for the back item. Something almost analogous to BookmarkFolderView. Call it PanelBackItemView. The image used in PanelBackItemView will later be defined by the add-on (file a follow-up) but for now, just use a default image (as ibarlow for one). PanelBackItemView will have a method like updateFromFilter(FilterDetail). For now, updateFromFilter() will only take a string as you only introduce FilterDetail in the next patch.

I see now why you need getPreviousFilter(). My suggestions for the previous patch remain the (almost) same. You can getPreviousFilter() in this patch as it's the first patch that will use it. The only change I'd make in the previous patch is in the ViewDetail's popFilter() method. Add a canPopFilter() method like:

public boolean canPopFilter() {
    return (mFilterStack != null && mFilterStack > 1);
}

And then implement popFilter() using it:

public boolean popFilter() {
    if (!canPopFilter()) {
        return false; 
    }

    mFilterStack.pop();
    return true;
}

More suggestions below.

::: mobile/android/base/home/PanelLayout.java
@@ +403,5 @@
> +        }
> +
> +        @Override
> +        public boolean canGoBack() {
> +            return getPreviousFilter() != null;

This will become:

return mViewState.canPopFilter();

Comment 23

[Lucas Rocha (:lucasr)]

Comment on attachment 8385694 [details] [diff] [review]
03 - Keep filter title in stack for header row

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

Clearing review until the previous patches are updated.

Comment 24

[Lucas Rocha (:lucasr)]

Comment on attachment 8385695 [details] [diff] [review]
04 - Show up arrow for back row

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

Clearing review until the previous patches are updated.

Comment 25

[Josh Dover [:jdover]]

Attachment: 01 - Use ViewState to get previous filter for UI

Changed popFilter and current filter logic to be more understandable as per lucasr's comments.

Comment 26

[Josh Dover [:jdover]]

Attachment: 02 - Show previous filter for back row item

(In reply to Lucas Rocha (:lucasr) from comment #22)
> Comment on attachment 8385690 [details] [diff] [review]
> 02 - Show previous filter for back row item
> 
> Review of attachment 8385690 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Given that the back item will be defined by the add-on, we should approach
> this a bit differently. Create a separate custom view for the back item.
> Something almost analogous to BookmarkFolderView. Call it PanelBackItemView.
> The image used in PanelBackItemView will later be defined by the add-on
> (file a follow-up) but for now, just use a default image (as ibarlow for
> one). PanelBackItemView will have a method like
> updateFromFilter(FilterDetail). For now, updateFromFilter() will only take a
> string as you only introduce FilterDetail in the next patch.

I think your suggestion from comment #5 makes more sense in that we can reuse the existing PanelItemView by simply prepending a row to the cursor if there is a filter. This means we don't have to duplicate code for making a similar view. This also makes the logic simpler within the adapter as well, as we don't have to do checks every where for offsetting the index in getView/bindView.

Later in the patches there is code for having a "default image" in PanelItemView that defaults to the up arrow for BackItemViews. While this does require modification of the class, I think this extension of PanelItemView could be useful in the future for the other view types as well.

Comment 27

[Josh Dover [:jdover]]

Attachment: 03 - Keep filter title in stack for header row

Comment 28

[Josh Dover [:jdover]]

Attachment: 04 - Show up arrow for back row

Comment 29

[Josh Dover [:jdover]]

Comment on attachment 8386987 [details] [diff] [review]
03 - Keep filter title in stack for header row

Wrong reviewer, dang muscle memory ;-)

Comment 30

[Lucas Rocha (:lucasr)]

Comment on attachment 8386982 [details] [diff] [review]
01 - Use ViewState to get previous filter for UI

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

Look good with the suggested changes (and unrelated changes reverted). Please post a new patch.

::: mobile/android/base/home/PanelLayout.java
@@ +266,5 @@
>       * the {@code PanelLayout}. Is responsible for tracking the history stack of filters.
>       */
>      protected static class ViewState {
>          private final ViewConfig mViewConfig;
> +        private LinkedList<String> mFilterStack;

Unrelated change?

@@ +282,3 @@
>           */
>          public String getCurrentFilter() {
> +            if (mFilterStack == null || mFilterStack.isEmpty()) {

Why do you need to check for emptiness here? Your code ensures that when mFilterStack is created, it will contain at least the initial filter ('null' for now)

@@ +297,5 @@
>  
> +                // Initialize with a null filter.
> +                // TODO: use initial filter from ViewConfig
> +                mFilterStack.push(null);
> +            }

nit: add empty line here.

Comment 31

[Lucas Rocha (:lucasr)]

Comment on attachment 8386987 [details] [diff] [review]
03 - Keep filter title in stack for header row

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

This is looking pretty good. Just giving f+ for the pending l10n issues.

::: mobile/android/base/home/PanelLayout.java
@@ +294,1 @@
>          }

Just add a new getter method for the item handler instead of exposing the associated ViewConfig directly.

@@ +324,4 @@
>  
>                  // Initialize with a null filter.
>                  // TODO: use initial filter from ViewConfig
> +                String top = getResources().getString(R.string.home_filter_top);

Double-check with ibarlow if that's what we want here. I thought he had suggested to use the panel's title instead? I'm fine with either.

I'm not sure showing 'Up to' + 'Top' is very l10n friendly because 'Top' is the word 'top' not the name of the previous item. Maybe, just for the 'root' filter case, we should use 'Up to top' to allow this string to be translated as a whole instead of translating 'Up to' and 'top' separately. We need input from a l10n person here.

This wouldn't be an issue if we showed the panel title instead of 'top' I think.

@@ +363,5 @@
>      /**
>       * Pushes filter to {@code ViewState}'s stack and makes request for new filter value.
>       */
> +    private void pushFilterOnView(ViewState viewState, FilterDetail filterDetail) {
> +        viewState.pushFilter(filterDetail);

nit: add empty line here.

@@ +376,5 @@
>       * @return whether the filter has changed
>       */
>      private boolean popFilterOnView(ViewState viewState) {
>          if (viewState.popFilter()) {
> +            final FilterDetail current = viewState.getCurrentFilter();

nit: add empty line here.

@@ +406,5 @@
>              } else {
> +                EnumSet<OnUrlOpenListener.Flags> flags = EnumSet.noneOf(OnUrlOpenListener.Flags.class);
> +                if (mViewState.getViewConfig().getItemHandler() == ItemHandler.INTENT) {
> +                    flags.add(OnUrlOpenListener.Flags.OPEN_WITH_INTENT);
> +                }

Nice.

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +324,5 @@
>       as alternate text for accessibility. It is not visible in the UI. -->
>  <!ENTITY home_reading_list_hint_accessible "TIP: Save articles to your reading list by long pressing the reader mode button when it appears in the title bar.">
>  
> +<!ENTITY home_filter_top "Top">
> +<!ENTITY home_filter_up "Up to">

Use format string parameter here: "Up to %formatS" (see other strings using similar parameters for reference). This string will be translated and the order of the words might be different depending on the language.

::: mobile/android/base/strings.xml.in
@@ +296,5 @@
>    <string name="home_reading_list_empty">&home_reading_list_empty;</string>
>    <string name="home_reading_list_hint">&home_reading_list_hint2;</string>
>    <string name="home_reading_list_hint_accessible">&home_reading_list_hint_accessible;</string>
> +  <string name="home_filter_top">&home_filter_top;</string>
> +  <string name="home_filter_up">&home_filter_up;</string>

The 'Up to' string doesn't seem to be used anywhere in this patch.

Comment 32

[Lucas Rocha (:lucasr)]

Comment on attachment 8386986 [details] [diff] [review]
02 - Show previous filter for back row item

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

This is looking great except for the way BackItem is implemented. I understand how tempting it is to reuse some code here but I think this is not very future-proof. Here are few reasons why I'm not so keen about this approach:

1. It adds a private item type in a spot that is meant to match the add-on's public API i.e. the PanelItemView stuff.
2. The necessary cursor mangling is kinda smelly. You're clearly bending BackItemView to conform to the PanelItemView's contract. The 'input' for a back item doesn't really need to be a cursor following the home provider schema. It's more like a FilterDetail-backed view. Even if we go with this approach, the cursor merging would have to happen in the adapter's swapCursor() call so that you don't need to create a new MatrixCursor instance every time the back item becomes visible in the list/grid.  
3. It will inflate a view for a description but it will never use it. And we'll always have to comply with this just because the back item is a PanelItemView.
4. It assumes the back item will always be just like an article item but that will not be necessarily true. We might, for example, want to use a different layout when showing the back item in a PanelGridView with image items. With this approach, we'd have to implement these variations directly in PanelViewAdapter, which is not ideal.
5. The last patch is pretty much about bending the PanelItemView API to cover the backitem case, which is, again, kinda smelly.

I originally suggested this approach in comment #5 because I was assuming the back item would just be the usual item with an extra marker on top of the image or something. But this is not the case in the back item design anymore. In other words, even though the back view almost feels like the same kind of thing than an panel item view, it's not. I'd really like to keep things separate so that the API of panel items and back items can evolve independently without them stepping on each other.  

So, here's what I suggest:

- Create a custom PanelBackItemView that inflates a panel_back_item_view.xml (with an icon + text)
- PanelBackItemView has a updateFromFilter(FilterDetail) method to update its content.
- PanelBackItemView's constructor will only take a context for now but later it will get a ViewConfig instance from with it will take the back item settings (back image URL, maybe back item text?). This will also allow us to vary the PanelBackItemView's layout depending on the panel view and item types if we need to.

Adapter-wise this will look similar to what you originally wrote with BookmarkFolderView.

::: mobile/android/base/home/PanelLayout.java
@@ +308,5 @@
> +            if (mFilterStack == null || mFilterStack.size() < 2) {
> +                return null;
> +            } else {
> +                return mFilterStack.get(1);
> +            }

This could become:

if (!canPopFilter()) {
    return null;
}

return mFilterStack.get(1);

Comment 33

[Lucas Rocha (:lucasr)]

Comment on attachment 8386988 [details] [diff] [review]
04 - Show up arrow for back row

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

Clearing review until the previous patches are updated.

::: mobile/android/base/home/PanelViewAdapter.java
@@ +103,5 @@
>              { "_id", HomeItems.DATASET_ID, HomeItems.URL, HomeItems.TITLE, HomeItems.DESCRIPTION,
>                HomeItems.IMAGE_URL, HomeItems.FILTER, HomeItems.CREATED });
>          FilterDetail filter = mFilterManager.getPreviousFilter();
> +        String upTo = mContext.getResources().getString(R.string.home_filter_up);
> +        backCursor.addRow(new String[] { null, null, null, upTo + " " + filter.title, null, filter.imageUrl, null, null });

See my comment about using string format parameters here.

Comment 34

[Josh Dover [:jdover]]

Attachment: 02 - Show previous filter for back row item

Okay I totally see how that was a bending of the API and semantics of 'item' to make the back row work, switched to a new view called PanelBackItemView.

Still to do here: use the panel's title as the 'up to' label for going back to the root filter. I'm not sure where the adapter or ViewState should get the title from the PanelConfig as ViewConfigs do not know of the Panel they belong to. Input on this would definitely be helpful.

Also to consider is that technically the panel is static, and there could be two different views in side of the panel, both with filter navigation. Does displaying the the filter title (ie "Up to NY Times") make sense if they are still technically in the panel?

Comment 35

[Josh Dover [:jdover]]

Attachment: 03 - Show filter title in back row

Comment 36

[Josh Dover [:jdover]]

Attachment: 04 - Show up arrow for back row

Comment 37

[Josh Dover [:jdover]]

Attachment: 05 - Add backImageUrl to allow for view-level custom back icons

This allows an addon to configure a custom icon to be used for the back row in place of the default "up" arrow:

Home.panels.register("my-panel", {
  title: "My Panel",
  layout: Home.panels.Layout.FRAME,
  views: [{
    type: Home.panels.View.LIST,
    dataset: "my-data",
    backImageUrl: "http://example.com/back-image.png"
  }]
});

Comment 38

[Lucas Rocha (:lucasr)]

Comment on attachment 8388929 [details] [diff] [review]
02 - Show previous filter for back row item

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

This looks great, thanks.

(In reply to Josh Dover from comment #34)
> Still to do here: use the panel's title as the 'up to' label for going back
> to the root filter. I'm not sure where the adapter or ViewState should get
> the title from the PanelConfig as ViewConfigs do not know of the Panel they
> belong to. Input on this would definitely be helpful.
> 
> Also to consider is that technically the panel is static, and there could be
> two different views in side of the panel, both with filter navigation. Does
> displaying the the filter title (ie "Up to NY Times") make sense if they are
> still technically in the panel?

Good point. Maybe we should stick with the 'Up to top' approach. We just need to make sure this string is translated as a whole to avoid l10n issues. Check with ibarlow if that's the wording we want.

::: mobile/android/base/home/PanelBackItemView.java
@@ +11,5 @@
> +import android.view.LayoutInflater;
> +import android.widget.LinearLayout;
> +import android.widget.TextView;
> +
> +class PanelBackItemView extends LinearLayout {

Nice.

::: mobile/android/base/home/PanelLayout.java
@@ +278,5 @@
>       * the {@code PanelLayout}. Is responsible for tracking the history stack of filters.
>       */
>      protected static class ViewState {
>          private final ViewConfig mViewConfig;
> +        private LinkedList<String> mFilterStack;

This change seems unrelated.

::: mobile/android/base/home/PanelViewAdapter.java
@@ +42,3 @@
>      @Override
> +    public int getCount() {
> +        return super.getCount() + (isShowingBack() ? 1 : 0);

Just realized that there's a race here in how the adapter is updated. Very similar to one I fixed in the bookmarks panel before. The filter stack will be updated (pop/push) first (and this might affect the number of items in the adapter immediately) then the cursor is updated once the panel loader returns and delivers the results. There are 2 problems with this:

1. The number of items in the adapter might change once the a filter is pushed/popped but no matching notifyDatasetChanged() is made on the adapter. We'll crash if a layout round happens before the new cursor is delivered.
2. There's a temporary mismatch between the stack state and the current cursor while a new cursor is being loaded.

We'll have to change the way we load and deliver cursor in such a way that the both the stack and the cursor are updated at the same time. See how BookmarksPanel updates the stack. Let's land the core feature first though. You can fix this issue in a follow-up bug (please file it).

::: mobile/android/base/resources/layout/panel_back_item.xml
@@ +13,5 @@
> +              android:paddingBottom="5dip"
> +              android:paddingLeft="10dip"
> +              android:paddingRight="10dip"
> +              android:minHeight="@dimen/page_row_height"
> +              android:gravity="center_vertical"/>

Cool.

Comment 39

[Lucas Rocha (:lucasr)]

Comment on attachment 8388930 [details] [diff] [review]
03 - Show filter title in back row

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

Looks great with the suggested change. I'm only concerned about the 'Up to &formatS;' string with the separate 'Top'. Let's talk to a l10n person about it.

::: mobile/android/base/home/PanelLayout.java
@@ +288,5 @@
>          public ViewState(ViewConfig viewConfig) {
>              mViewConfig = viewConfig;
>          }
>  
> +        public ViewConfig getViewConfig() {

Keep the getDatasetId() method as is. Just add a getItemHandler() here that returns mViewConfig.getItemHandler(). No need to expose ViewConfig directly here and the caller code will look a little less verbose.

::: mobile/android/base/home/PanelViewAdapter.java
@@ +6,5 @@
>  package org.mozilla.gecko.home;
>  
>  import org.mozilla.gecko.home.HomeConfig.ItemType;
>  import org.mozilla.gecko.home.PanelLayout.FilterManager;
> +import org.mozilla.gecko.home.PanelLayout.FilterDetail;

Unused?

Comment 40

[Lucas Rocha (:lucasr)]

Comment on attachment 8388931 [details] [diff] [review]
04 - Show up arrow for back row

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

Nice and clean.

Comment 41

[Lucas Rocha (:lucasr)]

Comment on attachment 8388933 [details] [diff] [review]
05 - Add backImageUrl to allow for view-level custom back icons

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

No need to make PanelBackItemView depend on FilterManager API when all it needs is an image url.

Let's move this patch to a separate bug. I'd like to keep this bug focused on the core feature only.

::: mobile/android/base/home/HomeConfig.java
@@ +649,5 @@
>              if (mItemHandler == null) {
>                  throw new IllegalArgumentException("Can't create ViewConfig with null item handler");
>              }
> +
> +            // XXX: mBackImageUrl is optional.

This comment is unnecessary.

@@ +678,5 @@
>  
>              json.put(JSON_KEY_TYPE, mType.toString());
>              json.put(JSON_KEY_DATASET, mDatasetId);
>              json.put(JSON_KEY_ITEM_TYPE, mItemType.toString());
>              json.put(JSON_KEY_ITEM_HANDLER, mItemHandler.toString());

Only put JSON_KEY_BACK_IMAGE_URL if mBackImageUrl is not empty. So:

if (!TextUtils.isEmpty(mBackImageUrl)) {
    json.put(JSON_KEY_BACK_IMAGE_URL, mBackImageUrl);
}

::: mobile/android/base/home/PanelBackItemView.java
@@ +37,5 @@
> +                   .into(image);
> +       } else {
> +            Picasso.with(getContext())
> +                   .load(imageUrl)
> +                   .into(image);

Once you get the backImageUrl in the constructor here, you can simply do:

Picasso.with(getContext())
       .load(backImageUrl)
       .placeholder(R.drawable.folder_up)
       .into(image);

If backImageUrl is null, the placeholder is automatically used.

::: mobile/android/base/home/PanelViewAdapter.java
@@ +65,5 @@
>      }
>  
>      private View newView(Context context, int position, ViewGroup parent) {
>          if (getItemViewType(position) == VIEW_TYPE_BACK) {
> +            return new PanelBackItemView(context, mFilterManager);

Change the adapter to get a ViewConfig in addition to ItemType. Provide the backImageUrl directly to the PanelBackItemView constructor here:

new PanelBackItemView(context, mViewConfig.getBackImageUrl());

Comment 42

[Lucas Rocha (:lucasr)]

Jeff, Francesco, we need some l10n input here.

Certain home panels might support folder-like navigation. So, while in a child 'folder', we'll show an 'Up to PARENT_FOLDER_NAME' item at the top. The translatable string for this is 'Up to &formatD' (which is fine I think?). When the parent is the root folder though, we use 'Top' as a predefined folder name. 'Top' is separate translatable string. This means we'd show 'Up to Top' in the UI.

Given that 'Top' is a noun, not an arbitrary folder name, would this approach cause l10n problems? Would it be better to have a 'Up to top' string that can be translated as a whole?

Comment 43

[Francesco Lodolo [:flod]]

Just to see if I'm understanding it correctly. In the current scenario we have 2 strings (inventing key names)

> moveUpToFolder = Up to &formatD
> topFolder = Top

&formatD could either be the name of the parent folder, or "Top" when the parent folder is root. 

Is "Top" displayed somewhere else besides being used as a replacement in the first string?

I think 3 strings give us more flexibility at relatively low cost.

> moveUpToFolder = Up to &formatD
> moveUpToRoot = Up to Top
> topFolder = Top

Or drop topFolder completely if it's not used elsewhere. 

To give you an example, in Italian I would probably translate these strings like this
> moveUpToFolder = Passa a &formatD
> moveUpToRoot = Passa alla cartella principale
> topFolder = Cartella principale

"Passa a Cartella principale" (using only 2 strings) would still work, but it would also sound less natural.

Comment 44

[Josh Dover [:jdover]]

(In reply to Lucas Rocha (:lucasr) from comment #38) 
> ::: mobile/android/base/home/PanelLayout.java
> @@ +278,5 @@
> >       * the {@code PanelLayout}. Is responsible for tracking the history stack of filters.
> >       */
> >      protected static class ViewState {
> >          private final ViewConfig mViewConfig;
> > +        private LinkedList<String> mFilterStack;
> 
> This change seems unrelated.

The Deque data structure only allows access to the first and last elements, in order to use .get() in getPreviousFilter() I need to use a LinkedList.
 
> We'll have to change the way we load and deliver cursor in such a way that
> the both the stack and the cursor are updated at the same time. See how
> BookmarksPanel updates the stack. Let's land the core feature first though.
> You can fix this issue in a follow-up bug (please file it).

Bug 982172 filed.

Comment 45

[Josh Dover [:jdover]]

Attachment: 03 - Show filter title in back row

Fixes comments and changes strings to what flod recommended.

Marking patch 05 as obsolete so it can be addressed in bug 982190.

Comment 46

[Francesco Lodolo [:flod]]

Can you add localization comments explaining what Top is in this context, and also what will replace the variable?

Comment 47

[Lucas Rocha (:lucasr)]

Comment on attachment 8389307 [details] [diff] [review]
03 - Show filter title in back row

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

Awesome.

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +329,5 @@
>       as alternate text for accessibility. It is not visible in the UI. -->
>  <!ENTITY home_reading_list_hint_accessible "TIP: Save articles to your reading list by long pressing the reader mode button when it appears in the title bar.">
>  
> +<!ENTITY home_move_up_to_filter "Up to &formatS;">
> +<!ENTITY home_move_up_to_root "Up to Top">

This should probably be 'Up to top' instead. Double-check with ibarlow.

Comment 48

[Lucas Rocha (:lucasr)]

Forgot to mention: please add localization comments as suggested by flod.

Comment 49

[Lucas Rocha (:lucasr)]

Comment on attachment 8388929 [details] [diff] [review]
02 - Show previous filter for back row item

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

Great.

Comment 50

[Josh Dover [:jdover]]

(In reply to Lucas Rocha (:lucasr) from comment #47)
> Comment on attachment 8389307 [details] [diff] [review]
> 03 - Show filter title in back row
> 
> Review of attachment 8389307 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Awesome.
> 
> ::: mobile/android/base/locales/en-US/android_strings.dtd
> @@ +329,5 @@
> >       as alternate text for accessibility. It is not visible in the UI. -->
> >  <!ENTITY home_reading_list_hint_accessible "TIP: Save articles to your reading list by long pressing the reader mode button when it appears in the title bar.">
> >  
> > +<!ENTITY home_move_up_to_filter "Up to &formatS;">
> > +<!ENTITY home_move_up_to_root "Up to Top">
> 
> This should probably be 'Up to top' instead. Double-check with ibarlow.

After talking with ibarlow, he brought up that the good point that "up to top" in a browser will be easily confused to scrolling to the top of the page. We decided that the confusion of having more than one view say "Up to <panel name>" is less than confusing "Up to top".

With that decision, I am still unclear on the best way to pass this title down to the ViewConfig? Should that be a property that is assigned when it is given to a PanelConfig?

Comment 51

[Josh Dover [:jdover]]

Attachment: 05 - Use panel title for root filter name

This changes to use the panel title as the name of the root filter. The way I passed the information was to add a 'mPanelTitle' field to the ViewConfig that is only used internally and is not exposed via JSON to addons.

Giving the ViewConfig the knowledge of this data is definitely hand-wavy, even though the code to handle it is quite minimal. Another possible route could be an information service that does know which PanelConfig each ViewConfig belongs to, and given a ViewConfig, could return data about the associated PanelConfig.

Comment 52

[Lucas Rocha (:lucasr)]

Comment on attachment 8390870 [details] [diff] [review]
05 - Use panel title for root filter name

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

This can be a lot simpler (see suggestion below) unless I'm missing something.

This patch seems to be applied on top of the previous version of this patch, right? Please send a new one that applies on top of this patch series with patch we're discarding.

::: mobile/android/base/home/PanelLayout.java
@@ +327,5 @@
>                  mFilterStack = new LinkedList<FilterDetail>();
>  
>                  // Initialize with a null filter.
>                  // TODO: use initial filter from ViewConfig
> +                mFilterStack.push(new FilterDetail(null, mViewConfig.getPanelTitle()));

Why not just add a mPanelConfig member to PanelLayout (it already gets it in the constructor anyway) and simply do mPanelConfig.getTitle() here?

Comment 53

[Josh Dover [:jdover]]

Attachment: 05 - Use panel title for root filter name

Ah yes didn't see that. This makes me much happier.

Comment 54

[Josh Dover [:jdover]]

Attachment: 03 - Show filter title in back row

Combined patches 3 + 5

Comment 55

[Josh Dover [:jdover]]

Attachment: 04 - Show up arrow for back row

Comment 56

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing.

Comment 57

[Josh Dover [:jdover]]

Attachment: 01 - Use ViewState to get previous filter for UI

Rebased

Comment 58

[Josh Dover [:jdover]]

Attachment: 02 - Show previous filter for back row item

Rebased

Comment 59

[Josh Dover [:jdover]]

Attachment: 03 - Show filter title in back row

Rebased

Comment 60

[Josh Dover [:jdover]]

Attachment: 04 - Show up arrow for back row

Rebased

Comment 61

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/65ad223741c7
https://hg.mozilla.org/integration/fx-team/rev/36929ec248c0
https://hg.mozilla.org/integration/fx-team/rev/38ed4a6f5879
https://hg.mozilla.org/integration/fx-team/rev/f5a1388c84ab

Comment 62

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/65ad223741c7
https://hg.mozilla.org/mozilla-central/rev/36929ec248c0
https://hg.mozilla.org/mozilla-central/rev/38ed4a6f5879
https://hg.mozilla.org/mozilla-central/rev/f5a1388c84ab

Comment 63

[:Margaret Leibovic]

Attachment: patch for aurora

Note to release managers: I'm requesting uplift for a series of Firefox Hub bugs. The main fixes we need are in bugs at the end of the series, but trying to rebase those patches proved difficult and risky, so I think we should just uplift the dependecies.

Note to sheriffs: I have a local patch series that I can land on aurora when these bugs all get approval.

I also updated this specific patch to remove the string addition (with this patch we'll just say the folder name, rather than "Up to <folder name>"), but this doesn't really matter because we're not officially supporting filters for Fx30.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): dependency for initial Firefox Hub release (promoted feed add-ons in fx30)
User impact if declined: panels won't update when data changes
Testing completed (on m-c, etc.): baked on m-c
Risk to taking this patch (and alternatives if risky): only affects dynamic panels, need an add-on to trigger this
String or IDL/UUID changes made by this patch: none

Comment 64

[:Margaret Leibovic]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3f1c2649de35

Comment 65

[:Margaret Leibovic]

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

Filter on epic-hub-bugs.