Feature/3704 new FilteredRecyclerView in Comments

[mzorz]

Addresses #3704

Related to #3643 and comes based on the discussion held on #3691.

Demo

Here’s a video of the working implementation:
https://www.dropbox.com/s/cebq28stv44t0u0/test.mp4?dl=0

Code

Implemented a FilteredRecyclerView component that expects a FilterCriteria set of elements through which to filter the elements in its adapter.
An example of its usage is implemented here for the Comments section of the WordPress Android app.

Made use of the native Spinner in this case, as it feels more the Android way. Will happily refactor and change it to use the ReaderTagToolbar if indicated to do so, as originally advised here by @nbradbury.

The FilteredRecyclerView has its own lifecycle, which should be self explanative through the use of this following interface

public interface Listener {
    FilterCriteria[] onLoadFilterCriteriaOptions();
    void onLoadData();
    void onFilterSelected(int position, FilterCriteria criteria);
    FilterCriteria onRecallSelection();
    String onShowEmptyViewMessage(EmptyViewMessageType emptyViewMsgType);
}

Design

As to the design, it’s been implemented following @mattmiklic ’s proposal here #3691 (comment)
Sizes have been considered following this:
https://www.google.com/design/spec/layout/metrics-keylines.html#metrics-keylines-keylines-spacing
Specifically, the toolbar has been set to a height of 48dp as per the subtitle section indicated there.

@mattmiklic Please note the down-pointing arrow is not implemented on purpose - we would need a 9-patch drawable to make it part of the background just like the drawable being used in other Spinners in the app (@drawable/spinner_background_ab_wordpress). Assuming you're the right person to ask for, can you provide these assets, please? If not please advise - I can probably help myself otherwise too.
Thanks in advance

Screenshots

Filter criteria selection

Hidden

Hiding

[nbradbury]

This is really nicely done! But of course since this is a review, I have to focus on the questionable parts :)

Some visual feedback first:

• The empty message "No comments with this status found" sounds really awkward. Maybe it could simply say, "No approved comments," "No spam comments," etc. (and "No comments" when the "all" list is empty).
• The empty message seems too low (it isn't centered vertically)
• I think the dropdown menu needs to appear below the spinner, like it does in the reader
• After changing the filter, the app waits for the comments to be retrieved before clearing the list and showing the desired comments. Can it immediately show comments cached from the previous fetch for that filter instead?

[nbradbury]

Minor nit: since moveToNext will move to the first row on a new cursor, this can be simplified to:

while (c.moveToNext()) {
     Comment comment = getCommentFromCursor(c);
     comments.add(comment);
}

[mzorz]

👍 changed this on both methods where the same approach was used

[mzorz]

Some visual feedback first:

• The empty message "No comments with this status found" sounds really awkward. Maybe it could simply say, "No approved comments," "No spam comments," etc. (and "No comments" when the "all" list is empty).

Done

The empty message seems too low (it isn't centered vertically)

oh, it is centered as it is part of the content area (the area starting immediately below the subtitle section/spinner toolbar). It scrolls up and down with the CoordinatorLayout.

I think the dropdown menu needs to appear below the spinner, like it does in the reader

Done

After changing the filter, the app waits for the comments to be retrieved before clearing the list and showing the desired comments. Can it immediately show comments cached from the previous fetch for that filter instead?

Done - also changed the wording for the toast in case there’s no network: as we are showing cached moments now, the user stays informed that what he’s seeing may not actually be fresh (check is made both on onResume and when the user changes the filter).

[nbradbury]

This is looking really solid! A few remarks:

• If I enable airplane mode and then go to the comment list, I first get a toast that there's no network available, then shortly after I get another toast that comments couldn't be refreshed.
• I think we can drop "showing cached comments" from error_refresh_comments_showing_cached. Simply saying comments couldn't be refreshed should suffice.
• I just noticed that if you view the detail of a trashed comment, you can trash it again - which removes it from the list of trashed comments until you next refresh. This bug didn't come up before because prior to this PR we didn't show trashed comments. What should happen is you're able to _un_trash the trashed comment. I think this can be a separate issue with a separate PR - just wanted to mention it.

[mzorz]

Glad you like it!

If I enable airplane mode and then go to the comment list, I first get a toast that there's no network available, then shortly after I get another toast that comments couldn't be refreshed.

Sounds good, will ditch the first toast

I think we can drop "showing cached comments" from error_refresh_comments_showing_cached. Simply saying comments couldn't be refreshed should suffice.

This one was on purpose: what I wanted to do is to inform the user that there is a problem retrieving comments, but given that content is being shown anyways (it's the content fetched from the local db) I want to make sure to give context to the user as to what is it they're seeing. Simply saying that "Comments couldn't be refreshed at this time" but then showing a list of comments anyway felt awkward to me (kind of asking the question to myself, we couldn't refresh but we are showing something, what is this something then?).

What do you think? I'll ditch it too if it doesn't sound too convincing.

Good catch on the third one! Will create another issue with that.

[mzorz]

first toast gone when no network available

[nbradbury]

What do you think? I'll ditch it too if it doesn't sound too convincing.

I guess I have two issues with that message. One is it seems a bit long, the other is the word "cached" is geek speak. We know what it means, but would someone non-tech understand it? If you think it's important to let the user know they're not seeing the most recent comments, something like "showing older comments" sounds more natural.

[mzorz]

💯

[mzorz]

Ok so, I think we're ready with this one, but will have to work on #3732 and have both ready in order to merge to develop, is that correct?

[nbradbury]

I think we're okay to merge this now. Develop is currently our v5.2 release, and code freeze isn't until March 7. That gives us plenty of time to tackle #3732, and I think we'd want to have this PR in develop before doing that.

[nbradbury]

In other words, this is good to go!

[mzorz]

👍 cool, I'll start working on #3732 next thing then! awesome to see this work make it into develop :)