Feature/58 filter comments by status

[mzorz]

Addresses #58.

I aggregated all my notes about my thinking process down here under the title "Investigation" just in case you want to know. Otherwise do please check Notes and Next steps below.

Screenshots

Notes

1) for the UI part (spinner, etc.), tried to follow / mimic the code style that is used in StatsActivity 2) needed to add string res ids to the enum values in CommentStatus to get their UI string representation (these will need be added to all the other strings files for translation). 3) the empty view shows the generic expression “No comments”, I’ve changed it for something more specific like “No comments with this status found” in the case anything but ALL is selected in the Spinner. (these will also need translation as well) 4) on both APIs (xmlrpc and REST), the ALL filter returns the approved and unapproved|hold items rather than really returning any comments with all possible states (that is, including spam and trash), so I added code to mimick that behavior as well. 5) side note: found the file StatsUtils contains several convenience methods that maybe are useful not just for the Stats part, maybe can be refactored and generalised In particular, I was looking into logVolleyErrorDetails() which seems handy for any other user of the RESTapi rather than just Stats. 6) on error handling in general, I’ve seen lots of 40x constants everywhere in the code, seems to be a candidate for refactoring all these into an enum maybe? 7) What’s the strategy for growing/trimming/maintaining the database, and the comments table in particular?

For next steps

1) when you land on this screen, the code in develop deletes from the local database all those items that have been moved to the TRASH state in the server by calling ApiHelper.removeDeletedComments(blog);, while on the other hand I would tend to think the idea for this feature is to mimmick the behavior the web admin application has (i.e. showing the elements in TRASH when selecting the TRASH filter, etc.), so in order to be able to show the TRASH elements I ditched that part of the code so we keep trash elements. However, we should have some strategy to deal with it, like, for instance, maybe only deleting all trashed elements that still exist in our local database but don’t exist on the server anymore. For this, maybe we can ask for 100 elements, (which while arbitrary, I would think is a decent number for this purpose), then locally delete all items that do not exist in the returned set.

2) wondering if it makes sense to add other more granular events to AnalyticsTracker (and related classes) as we can determine now not only if the user has opened the Comments section but also know when they are interested in a particular status type. Would love to add that if you see it’s useful for sure. If you find it useful, please let me know which keyword should be used for the eventName in AnalyticsTrackerNosara.java

Looking forward to your comments! Thanks in advance

Investigation

So in considering this issue, first thing I thought was where to place this at, at a UI level:

a) I loved the pattern used in Notifications with the push-button-like tabs, should we use an UX similar to that one? (last tab to the right in the main view). If Comments states are finite (and less than 4 or 5), we could use that same design pattern. b) Another alternative is using the spinner selector we have in the StatsActivity on the action bar (seems a bit better as the title for the screen shows what the content is about) c) Otherwise, we should have the “sort by” option menu and submenu pattern on options (three dots at the right end of the action bar)

After considering it for a bit, b) is the one that better resonates to me, as it’s proper for filtering (which is the direct use case at hand), it doesn’t eat up screen real estate and it’s the recommended artifact in Android for filtering lists. In any case, any of the 3 require a similar effort so no problem if this needs be changed later (just let me know if you think there’s a better approach to UI in this regard).

Then onto the models, to identify which field(s) is(are) holding the type for Comments. Went on to check the adapter. (side note on code structuring: I first felt the need to create a new package under ui.comments called adapters as can be seen elsewhere, but then figured out you seem to only create a new package when you have more than one file/class to bundle together. Given we only have one adapter here, it seemed fine to leave it as it is.) Ok so, quickly found Comment has a getStatusEnum, so great, we have it already.

UNKNOWN,
UNAPPROVED,
APPROVED,
TRASH,  // <-- REST only
SPAM;

(BTW when checking inside that enum, saw the toRESTString and wondered why not using com.google.gson.annotations.SerializedName annotations for serializing this, but that surely merits a different discussion)

Ok so, as per the comments here it seems the UI will only show UNKNOWN, UNAPPROVED, APPROVED, and SPAM. A little doubt about the UNKNOWN one, so checking out its usages just to confirm this is not a kind of a keyword only used internally to represent uninitialised vars or something. Checked it out and it seems it’s the default value, but the only visible values seem to be SPAM, UNAPPROVED, and APPROVED (checking https://github.com/wordpress-mobile/WordPress-Android/blob/develop/WordPress/src/main/java/org/wordpress/android/ui/comments/CommentAdapter.java#L168 ), and it adds TRASH in the CommentDetailFragment status views. So, going to initialise the Spinner with these 4 values then, plus going to use UNKNOWN for “All” (that is apply no filter). [*] Later on, I would find out that TRASH is not served by xmlrpc as per the docs here http://codex.wordpress.org/XML-RPC/wp.getComments (approve, hold, spam are the only valid values for status filtering), so I’m setting this option aside for now.

Now, networking. Things are loaded in the UpdateCommentsTask, which makes the server take care of pagination. The method to call to retrieve fresh comments is updateComments() in CommentsListFragment. I question myself: does the API have a means to only get comments of a certain type? This would affect pagination of course. Checking again and we’re using offset and number params to fetch COMMENTS_PER_PAGE (30) amount of comments starting at offset. Checking the docs here http://codex.wordpress.org/XML-RPC/wp.getComments alright, we have the optional filter struct which may contain one of these statuses: [approve|hold|spam]. I figured out these would map to APPROVED, UNAPPROVED and SPAM respectively in our StatusEnum class.

So now the question I make to myself is, are we using the database as a cache for, say, have some sort of offline support or are we using it locally for something else? Digging here and there I noticed the pattern where ApiHelper also saves to the local database the data fetched, and then reading more I learned about this being an approach to mimick the Flux architecture. Playing around with airplane mode on, I realised there’s the bottom yellow stripe that reads NO CONNECTION but I can still goof around as the app presents the data it has saved locally. I realise that all sections (Stats, Posts, Pages) show a Toast “There is no network available”, except for Comments, so I guessed this is a bug. (I’m fixing it here as well, commit eb1ad29b13a82b7743087c7195a62ceb7fc41a2f).

Ok so, we have made a decision on the UI (use a spinner), we know which values to use for the filter, and we know the database apparently is used for offline browsing. Knowing that we have pagination, we need to deal with the cases where if the user is supposed to be seeing up to 30 items per page (if they exist), but a given type of item (say, SPAM) is only in the next page (or n pages behind), it needs to be shown no matter what. What makes sense here is to just refresh from the server using the filter option each time the user changes the filter, and it also makes sense to filter locally when there’s no network, to make UI consistent.

In order to accomplish these 2 things, the safest approach is to just save everything to the database, on each request to the server (refreshing, updating, adding what’s not there) - but that brings the problem of how large should our Comments table grow, or for how long should we keep records in that table. I wonder if you have any database purge strategy? (like limits on size, time or number of records).

So, with all this, the rest is code!

[khaykov]

Hi @mzorz ! I'm currently working on CommentsActivity as part of #3551 (also see #3687 and #3663). You had no way of knowing it, but the thing is - we can't use action bar when the comments list is displayed as a fragment.

Same goes for Stats. Something like this could work.

[mzorz]

Oh I see - ok will move that control out of the Activity and make the fragment a self contained thing with the filters spinner and list all within the fragment. Will push again and let you know. Thanks for checking!

[nbradbury]

will move that control out of the Activity and make the fragment a self contained thing with the filters spinner and list all within the fragment.

If you use a "spinner" like the one @khaykov suggests, let me save you some time and suggest you not use a real spinner.

I say this because I have a similar spinner in the reader which scrolls with the content. The reader uses a RecyclerView, so having the spinner scroll with the content required using a separate view type for the spinner. To say that spinners don't play well with being recycled would be an understatement :)

So instead I simply use a TextView that's styled to look like a spinner, and it displays a ListPopupWindow when tapped which looks like a spinner dropdown.

[khaykov]

@mzorz I already moved all the activity code into fragment. There are specific dual pane quirks so I had to change a lot of other stuff too, so If you could just add spinner to adapter I will have easier time merging changes later.

[mzorz]

@khaykov okay, I'll check your branch and see how to merge with the technique @nbradbury suggests to use here, sounds good?

[khaykov]

@mzorz Thanks! If you'll use @nbradbury's spinner everything should be smooth when merging, so don't worry too about it right now :)

[mzorz]

:+1: will try to get this addressed tomorrow, thanks guys!

[mzorz]

Guys, when looking into this I feel the need to repurpose @nbradbury's ReaderTagToolbar and make it more general, like having a base class then implement the current ReaderTagToolbar by extending such base class and implementing the toolbar I need by extending that same base class too, for instance.

No problems with that, but then there's this one thing bugging me: why is the filter being scrolled with the content? Is that on purpose? For the comments filtering I feel the filter selector should always be visible (otherwise you always have to scroll all the way back up to change the filter). And if you agree on this, then we could just build a component and use a LinearLayout (w/ vertical orientation) that simply has first a regular Spinner and then the RecyclerView to make all part of the same Fragment, then we can happily use it elsewhere. I'm sure there has to be some reason as to why it was made that way in the Posts section, but just wondering what your thoughts are before I get too far. cc @khaykov

[nbradbury]

Guys, when looking into this I feel the need to repurpose @nbradbury's ReaderTagToolbar and make it more general...

Makes perfect sense to me, as a separate PR.

No problems with that, but then there's this one thing bugging me: why is the filter being scrolled with the content? Is that on purpose?

It is on purpose, sorry :) I had a long back-and-forth with the designers on this exact subject, and in the end it was decided that the reader filter should scroll with the content. It actually works okay there once you realize you can tap the active tab to scroll to the top of the content, exposing the filter again. Not sure how well it'll work with the comment list, though. Would you like me to summon a designer to add their input?

[mzorz]

Great! No problem, this explanation clears it up. I'll proceed (most probably tomorrow) with refactoring the toolbar on another PR Thanks!

[nbradbury]

Hey @mattmiklic, we could use a designer's input here. Here's the tl;dr version:

• A filter for the Android app's comment list is being added (so the user can see only SPAM comments, UNAPPROVED comments, etc.)
• Normally this would be a standard dropdown in the ActionBar, but that may not work with the tablet UI
• So a dropdown above the comment list similar to the one above the Reader is being considered
• The one above the Reader scrolls with the content, so we're thinking the one above the comment list should do the same
• But the Reader is inside the "masterbar" tabs, and the user can tap the Reader tab again to scroll to the top of the list - which isn't available in the comment list (so once the filter scrolls out of the view, the user would have to scroll back to the top to change the filter again)

Given that smattering of bullet points, do you have any suggestions?

[khaykov]

Some thoughts - we can tap Toolbar to go to top. And on tablet we can do same thing as with Reader -tapping MySites tab will scroll whatever content fragment to top. And as additional feature we can add same functionality to MySite sidebar - when you tap Comments (or any other fragment) again the list will scroll to top.

Here are some screencasts to illustrate the idea: As activity - https://cloudup.com/cLKOLfkjXWn As fragment - https://cloudup.com/chkWS24mpRl

[mzorz]

I think ideally the filter needs to stay visible - but another nice possibilty to save screen real estate while having the filter toolbar handy at the same time would be to show it on scroll up, hide it on scroll down, with a fancy animation.

What do you guys think?

[mattmiklic]

but another nice possibilty to save screen real estate while having the filter toolbar handy at the same time would be to show it on scroll up, hide it on scroll down, with a fancy animation.

I like this idea, because it makes it easier to find the filter than if the user has to scroll all the way to the top of the list. That said, I don't think it's 100% necessary -- the main benefit of not having to scroll all the way up is that you don't lose your place in the list. But if you're filtering the list, the content of the list is going to change, anyway. So I think we could get by if we just made the toolbar tappable to return to the top of the list, as @khaykov suggested.

I went ahead and explored what @mzorz's idea might look like. Here, the filtering bar is in gray, and it has a higher elevation than the list below it, so it makes sense when the list scrolls underneath it.

In this mockup I aligned the spinner to the 72dp keyline, along with the "Comments" header and the comment text in the list. Using a different keyline might make more sense since you're working on the tablet version, which I haven't had a chance to look at in much detail.

With keylines visible:

Here's what the same style bar looks like in the Reader, though I've repositioned the spinner on the 16dp keyline, since we've got the settings icon there too.

I can flesh this out in more detail if needed. If this is a lot of extra work and you'd prefer to go with the simpler solution (just tap the toolbar to scroll up to the filter bar), I think that's fine, and this could be a nice enhancement to experiment with later.

[nbradbury]

If this is a lot of extra work and you'd prefer to go with the simpler solution (just tap the toolbar to scroll up to the filter bar), I think that's fine, and this could be a nice enhancement to experiment with later.

For the comment list, having the filter always visible should be less work. For the reader, it's going to be more since it'll require undoing a lot of code which enables the filter to scroll with the list. But I'm fine with that - I never liked having the reader filter scroll away in the first place :)

I'm not a fan of tapping the toolbar to scroll to the top. I'd use it all the time since I know it exists, but my guess is very few users would know it's possible.

[mattmiklic]

For the comment list, having the filter always visible should be less work.

Just in case my comments weren't clear, I'd be in favor of the "show it on scroll up, hide it on scroll down" idea, but not as keen on having it always visible, because of amount of space it takes away from the scrollable area.

[nbradbury]

Just in case my comments weren't clear, I'd be in favor of the "show it on scroll up, hide it on scroll down" idea, but not as keen on having it always visible, because of amount of space it takes away from the scrollable area.

Ah, yes - I did misunderstand. This will be even more work for the reader, but I think it's a better approach (in fact, it's how the reader filter used to work).

[mattmiklic]

Just had one other idea -- a fixed (always visible) second row might work, if we added it as a kind of second row to the existing toolbar (so it's not quite as tall as a full toolbar). I think, following the examples I found, it should be related to the toolbar color:

I'm following an example I found in Google Maps. This is from the web app, but it's still using Material Design.

[mattmiklic]

That option might look a little better, especially if it's fixed on screen, though I'm not sure how well it translates to the tablet view. I think either one would work fine.

[nbradbury]

I'm leaning towards that latter option simply because it's more obvious. The current Reader filter blends in with its surroundings too much, imo.

[nbradbury]

So...now I'll raise the awkward question I've raised elsewhere: is the tablet UI such a benefit to our users that it's worth making these changes to support it? I ask because I think non-tablet users would find the spinner in the toolbar to be easier to deal with, and the only reason we're not going that route is to support our upcoming tablet UI.

[mzorz]

Catching up here guys, so, if I'm understanding correctly:

Behavior:

• @mattmiklic would prefer to hide/show on scroll down/up (me: +1)
• @nbradbury would rather have the spinner always be visible
• I agree with @nbradbury tapping the toolbar to go to top isn't intuitive
• @nbradbury is split (quite correctly, I think) in between having an ideal behavior for phone users (spinner on toolbar) vs. tablets (spinner can't be on the toolbar)

Look and feel (design):

• all agree (including me) the latter is better

A solution

In terms of what's needed for tablets, my take is if we move the spinner to the fragment it'll be easier for reusability, as these will act as a unique component rather than as 2 different things. They are inextricably linked (one acts as the filter of the other, and both kind of "know" the data types they're handling). It makes sense to have them both in a custom view / component (let's call it "Filtered RecyclerView"). In terms of the view, I think we all agree the latter proposed by @mattmiklic will work.

The only thing that we need make behave different in one (handsets) or the other (tablets) is that handsets will also have the scroll up/down hide/show functionality as an aid to save visual space, that won't be necessary in the tablet scenarios (at least those that we are considering for now). We can even set a flag for it in the constructor/attributes and that's it, and make it look exactly as per @mattmiklic latter design.

This way, handset users will still find the filter spinner on top (not exactly on the action bar, but still will look to be part of the toolbar) and won't take any extra space if not necessary, and tablet users will feel the spinner is tightly related to the content being filtered, plus providing a context/title to what they're looking at. @khaykov @nbradbury @mattmiklic do you think this applies to the various scenarios where filtered lists would be needed? Looking forward to hearing your thoughts!

[nbradbury]

@mzorz I really appreciate your thoughtful responses here - thanks for taking the time to really think about the experience (something devs aren't well-known for :smile: ).

@nbradbury would rather have the spinner always be visible

I'm actually more in favor of having it auto-hide on scroll - that's the way it used to work in the reader, and I argued against dropping it.

The only thing that we need make behave different in one (handsets) or the other (tablets) is that handsets will also have the scroll up/down hide/show functionality as an aid to save visual space, that won't be necessary in the tablet scenarios

I think we should still auto-hide on tablets. Consider smaller tablets like the Nexus 7: we'd use the phone UI in portrait and the tablet UI in landscape, and I think we'd want the filter to behave the same either way.

...plus providing a context/title to what they're looking at.

That's an excellent point. The lack of a title when there's an ActionBar spinner is something that I've never been comfortable with.

[mattmiklic]

Going for the auto hide/show with the latest design sounds like a good plan to me. It's more in line with material design than what we're doing now in the reader, and more useful, too.

[mzorz]

Cool! thanks for your comments @nbradbury @mattmiklic

I'm actually more in favor of having it auto-hide on scroll - that's the way it used to work in the reader, and I argued against dropping it :)

oops sorry I missed this one! :)

I think we should still auto-hide on tablets

:100:

So, it seems we have an agreement, and I have work to do :)

@khaykov please do let me know your thoughts as well as you've been giving tablets quite a thought already - thanks in advance

[khaykov]

Autohide sounds great!

@mattmiklic How do you see the latest design working on tablet?

[khaykov]

Ok, you all are probably tired from all this tablet UI talk, but based on the feedback I got, we could probably try a different approach. Instead of thinking about how to find a middle ground between smartphone and tablet UI, we should focus on how to make tablet UI work without compromising smartphone users experience, aka do what best for them and then try to adapt it to tablets. Tablet users are minority, so the tablet UI should be just a nice bonus feature, not something that might inconvenience majority.

And this brings us back to spinner problem - the best way to deal with it would be to put it into a toolbar, as originally suggested by @mzorz and how it is currently implemented at Stats screen.

What about tablets? Well, why not just restyle the toolbar in dual pane layout? It will be consistent with smartphone UI and won't look too out of place on tablets. Make it slide out/in on scroll and that's it.

Based on @mattmiklic design we could do something like this:

This does not look super fancy, but it won't affect smartphone users and would be quite simple to implement. Also, this will probably work with Media Browser too.

[mattmiklic]

Based on @mattmiklic design we could do something like this:

That's not too bad, I think it makes sense to consider the tablet UI separately, since it works a bit differently. Your screenshot is kind of reminiscent of the way tabs in Calypso collapse into a dropdown when there are a lot of options in the section navigation component:

What if it followed the design of this section navigation a bit more closely -- so the background is detached from the blue header, and it's the same width as the other cards. But give it a higher elevation (so a deeper shadow) and fix its position, so the other cards scroll below it. I may have time this week to put together a rough mockup if it's helpful. I think that would look better than having it attached to the blue header.

[khaykov]

Thank's @mattmiklic ! I think I got the idea.

Toolbar same width as cards:

A little bit wider:

[mattmiklic]

That's pretty much what was in my head (the first of the two). Seeing it, I'm not 100% sure if I like it though. I think the design of the tablet view of the app is something that really needs some more structured thinking, rather than trying to just figure out this idea completely standing on its own. So I would recommend, in this case, to use the existing filter design from the Reader, until we have a spot in our schedule to think about the design considerations for a tablet view in more detail.

[mzorz]

Taking all of this into consideration, I guess the tablet discussion will continue to take place on another issue / slack / elsewhere. I'm moving on with the phone implementation for this issue. Will let you know as I get some time to make progress on that - feel free to reference this ticket elsewhere if needed, to keep track of the conversation