1084062 - Support read/unread state in reading list UI

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Reader View, defect)

Description

[:Margaret Leibovic]

We have a "read" column in the DB, but we don't have any UI to mark an article as read, or to hide it from the list after it's read.

The UX here may be tricky, since if we let users mark articles and hide them from the reading list, we should probably give them another way to get back to those articles. Otherwise they might as well just delete them.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

The Share overlay also had a UI change to add an "unread" indicator to the "Reading List" tab title. This came up because you can add to the reading list from the share overlay, and it would be nice to "remind" you that you did when you start Fennec.

I don't have a bug handy for that, but I think one was filed.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Mark Finkle (:mfinkle) from comment #1)

> I don't have a bug handy for that, but I think one was filed.

Bug 1044089

Comment 3

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Are there any side swiping capabilities for list items in panels currently? Might be a neat way to mark/unmark/delete.

I'm about to start on bug 1082110 so I'll consider this indicator as a part of that bug too. :)

(In reply to :Margaret Leibovic from comment #0)
> The UX here may be tricky, since if we let users mark articles and hide them
> from the reading list, we should probably give them another way to get back
> to those articles. Otherwise they might as well just delete them.

We could do something similar to what we have for the "# devices hidden" in the Sync Tabs panel at the very bottom. Except, it would be "# articles archived" or something similar.

Comment 4

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: spec_readinglist_mock2.png

Attaching spec of latest design from Bug 1082110, will unify with our other devices.

Comment 5

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: prev_readinglist_mock3.png

Talked to Robin and Darrin about this some more and looking at what we do on our other platforms, we want to keep this consistent.

Inspired by each other and email clients, we want to use type to show this status. We can follow up with more visual affordance later like an icon but I don't want to overly pack this UI for the time being.

Specs
===

Read: 
Title - Roboto, Medium #363B40, 14 sp
Body - Roboto, Regular #363B40, 12 sp

Unread:
Title - Roboto, Light #BFBFBF, 14 sp
Body - Roboto, Regular #BFBFBF, 12sp

Comment 6

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(ignore time estimate in the mock for now :) my bad!

Comment 7

[:Margaret Leibovic]

As part of the read/unread UI work, we can also implement the read/unread button in the reader mode toolbar that's included in the mocks for bug 1120004 (I'm going to punt on that for that bug).

Comment 8

[Barbara Bermes [:barbara] - NI please!]

This is something we've discussed to revisit and add to the current reading list for android

Comment 9

[:Margaret Leibovic]

Redirecting to Sebastian, since he can probably get to this before I can.

Comment 10

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

NI-ing Robin here for updated designs to keep it consistent with iOS.

Reader View controls were in https://bugzilla.mozilla.org/show_bug.cgi?id=1120004#c10

Comment 11

[Robin Andersen [:tecgirl]]

Anthony, for consistency's sake, iOS doesn't use a color to denote the menu is activated (the Aa), there's no highlight on the choice made, and the read/unread dot is blue. If orange is the active Android convention, then by all means keep it. I would also attempt to balance out the line widths on the toolbar menu, perhaps by making the "Add to Reading List" icon a little thinner or bumping up the line on the Read/Unread dot.

Comment 12

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: prev_readinglist_readunread.png

Noted! attaching new mock.

Comment 13

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: spec_readinglist_mock3.png

I just took the spec of the existing reading list panel and updated little stuff. Did this quickly so feel free to ping if I missed anything. 

Tablet and Mobile are pretty similar

Comment 14

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: RLpanel_read_icon.zip

We use the same (unread) filled orange circle icon in our Reader View controls. But the "unread" icon is different because this is on white. We could probably tie it together later but here they are so we're not blocked.

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: readinglist.png

This is how the current implementation looks like.

* I ignored the different font families for now because they are not available on all platform versions (Roboto light/regular: Android 4.1+, Roboto medium: Android 5+.

* I didn't use the images for the indicator but used a drawable XML (smaller file size)

* I added a new context menu item to mark an item as read/unread.

Comment 16

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8647586 [details]
readinglist.png

(In reply to Sebastian Kaspari (:sebastian) from comment #15)
> Created attachment 8647586 [details]
> readinglist.png
> 
> This is how the current implementation looks like.
> 

A*! looks great!

> * I ignored the different font families for now because they are not
> available on all platform versions (Roboto light/regular: Android 4.1+,
> Roboto medium: Android 5+.

I thought about this as I was prepping the previews too. I think ignoring it is fine. It was an older mock that I had so it was probably before I knew we didn't support those weights.

WFM!

> * I didn't use the images for the indicator but used a drawable XML (smaller
> file size)

That's a good idea. I was wondering if we could do that actually. so thanks for taking that initiative here, good call!
 
> * I added a new context menu item to mark an item as read/unread.

"Mark as unread/read" perhaps?

Comment 17

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Anthony Lam (:antlam) from comment #16)
> > * I added a new context menu item to mark an item as read/unread.
> 
> "Mark as unread/read" perhaps?

Alright. That's what I was going for and then I peeked what Gmail uses and then changed it. ;)

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1084062 - Support read/unread state in reading list UI. r?mhaigh

Bug 1084062 - Support read/unread state in reading list UI. r?mhaigh

Comment 19

[:Margaret Leibovic]

Nice! Richard might want to do a quick drive-by on the reading list DB interactions here. Just putting it on his radar.

Comment 20

[Martyn Haigh (:mhaigh)]

Comment on attachment 8647931 [details]
MozReview Request: Bug 1084062 - Support read/unread state in reading list UI. r?mhaigh

https://reviewboard.mozilla.org/r/16085/#review14489

Ship It!

::: mobile/android/base/home/HomeFragment.java:259
(Diff revision 1)
> +        if (itemId == R.id.mark_read) {

Not your doing - but I really wish these were else-ifs or a switch statement.

::: mobile/android/base/home/ReadingListRow.java:69
(Diff revision 1)
> +        title.setTextAppearance(getContext(), isUnread ? R.style.Widget_ReadingListRow_Title_Unread : R.style.Widget_ReadingListRow_Title_Read);

This and the next two are some mighty long lines - perhaps split them after the ":" ?

::: mobile/android/base/resources/drawable/reading_list_indicator_read.xml:12
(Diff revision 1)
> +        android:color="@color/disabled_grey" />

I prefer the style of having no space before the closing tag markup, this is also the default in IntelliJ.  We have conflicting styles across the codebase at the moment, but we should try to unify as we go.

Feel free to object if you prefer this style.

::: mobile/android/base/resources/layout/reading_list_row_view.xml:37
(Diff revision 1)
>      <TextView

Didn't antlam say to get rid of the read time?

Comment 21

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Martyn Haigh (:mhaigh) from comment #20)
> ::: mobile/android/base/home/HomeFragment.java:259
> (Diff revision 1)
> > +        if (itemId == R.id.mark_read) {
> 
> Not your doing - but I really wish these were else-ifs or a switch statement.

Yeah, but I think a switch statement doesn't add much readability to a long list of things necessarily. It introduces the chance of forgetting 'break' though. ;) But I like using them with enums because the IDE will nag you if you forget a case.


> ::: mobile/android/base/resources/drawable/reading_list_indicator_read.xml:12
> (Diff revision 1)
> > +        android:color="@color/disabled_grey" />
> 
> I prefer the style of having no space before the closing tag markup, this is
> also the default in IntelliJ.  We have conflicting styles across the
> codebase at the moment, but we should try to unify as we go.
> 
> Feel free to object if you prefer this style.

I prefer the space. :-) I've set up IntelliJ to use the 'Android' XML style and it actually adds the space when I let the IDE format the file. It seems like we do not have a XML style guide?


> ::: mobile/android/base/resources/layout/reading_list_row_view.xml:37
> (Diff revision 1)
> >      <TextView
> 
> Didn't antlam say to get rid of the read time?

It's completely disabled and hidden in code. There are still some open bugs to fix the reading time so I didn't touch this. But if possible I'd like to delete this because it makes the layout simpler. NI-ing antlam to decide (I'll open follow-up bugs if needed).

Comment 22

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Sebastian Kaspari (:sebastian) from comment #21)
> (In reply to Martyn Haigh (:mhaigh) from comment #20)
> > ::: mobile/android/base/resources/layout/reading_list_row_view.xml:37
> > (Diff revision 1)
> > >      <TextView
> > 
> > Didn't antlam say to get rid of the read time?
> 
> It's completely disabled and hidden in code. There are still some open bugs
> to fix the reading time so I didn't touch this. But if possible I'd like to
> delete this because it makes the layout simpler. NI-ing antlam to decide

I would keep it in for now. 

Although we don't currently have an immediate plan to use that in the UI yet, I think it might still be something we could make use of in the future. Currently, we have issues around localization, etc that's preventing us from moving forward with it.

Comment 23

[:Margaret Leibovic]

https://reviewboard.mozilla.org/r/16085/#review14559

::: mobile/android/base/db/LocalReadingListAccessor.java:196
(Diff revision 1)
> +        values.put(ReadingListItems.MARKED_READ_ON, 0);

Drive-by: shouldn't we put a timestamp here? Also, I imagine we should put a client id in the MARKED_READ_BY field. I would ping rnewman about this, since he's not responding to my needinfo :)

Comment 24

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to :Margaret Leibovic from comment #23)
> ::: mobile/android/base/db/LocalReadingListAccessor.java:196
> (Diff revision 1)
> > +        values.put(ReadingListItems.MARKED_READ_ON, 0);
> 
> Drive-by: shouldn't we put a timestamp here? Also, I imagine we should put a
> client id in the MARKED_READ_BY field. I would ping rnewman about this,
> since he's not responding to my needinfo :)

Good question! Right now I'm restoring the values that have been in the database before we marked the item as read. I guess it depends on whether we want to use these fields for 'unread' too. Going to ask Richard today. :)

Comment 25

[Sebastian Kaspari (:sebastian; :pocmo)]

> <•rnewman> sebastian: the only thing to toggle is IS_UNREAD to 1 
> <•rnewman> the other two fields can stick around until it's marked as read again

I'll update the patch accordingly.

Comment 26

[Sebastian Kaspari (:sebastian; :pocmo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbd7f06405bc

Comment 27

[Sebastian Kaspari (:sebastian; :pocmo)]

url:        https://hg.mozilla.org/integration/fx-team/rev/c6ebcbe6189ebca7d33243da8fb0b8a6c9440e63
changeset:  c6ebcbe6189ebca7d33243da8fb0b8a6c9440e63
user:       Sebastian Kaspari <[e-mail]>
date:       Fri Aug 14 10:17:40 2015 +0200
description:
Bug 1084062 - Support read/unread state in reading list UI. r=mhaigh

Comment 28

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Anthony Lam (:antlam) from comment #22)
> (In reply to Sebastian Kaspari (:sebastian) from comment #21)
> > (In reply to Martyn Haigh (:mhaigh) from comment #20)
> > > ::: mobile/android/base/resources/layout/reading_list_row_view.xml:37
> > > (Diff revision 1)
> > > >      <TextView
> > > 
> > > Didn't antlam say to get rid of the read time?
> > 
> > It's completely disabled and hidden in code. There are still some open bugs
> > to fix the reading time so I didn't touch this. But if possible I'd like to
> > delete this because it makes the layout simpler. NI-ing antlam to decide
> 
> I would keep it in for now. 
> 
> Although we don't currently have an immediate plan to use that in the UI
> yet, I think it might still be something we could make use of in the future.
> Currently, we have issues around localization, etc that's preventing us from
> moving forward with it.

Just a heads up: We should never keep code in the application that is not being used. If we decide to add a feature in the future, we can add the code. Keeping unused code or layouts makes maintenance harder.

Sebastian - Can you file a bug to clean it up?

Comment 29

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Mark Finkle (:mfinkle) from comment #28)
> Just a heads up: We should never keep code in the application that is not
> being used. If we decide to add a feature in the future, we can add the
> code. Keeping unused code or layouts makes maintenance harder.
> 
> Sebastian - Can you file a bug to clean it up?

Done! Bug 1196387.

Comment 30

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

https://hg.mozilla.org/mozilla-central/rev/c6ebcbe6189e

Comment 31

[Liz Henry (:lizzard) (relman/hg->git project)]

Release Note Request (optional, but appreciated)
[Why is this notable]: suggested by dev team
[Suggested wording]: Support read/unread state in reading list panel
[Links (documentation, blog post, etc)]:

Comment 32

[Teodora Vermesan (:TeoVermesan)]

Tested using:
Device: Nexus 4 (Android 5.1)
Build: Firefox for Android 43.0a1 (2015-09-17)

Open an article, add it to reading list, switch to reading list panel, the article is displayed with an orange bullet on the left side and black text (unread state). Long tap on it and from context menu choose "Mark as read", the article will be displayed with a grey bullet on the left side and grey text (read state). 

For example if I go to private browsing to the reading list panel, and long tap on an article and choose "Mark as read" or "Mark as unread", going back to normal browsing, the choice made in PB is applied in normal browsing. Is it expected?

Comment 33

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Teodora Vermesan (:TeoVermesan) from comment #32)
> For example if I go to private browsing to the reading list panel, and long
> tap on an article and choose "Mark as read" or "Mark as unread", going back
> to normal browsing, the choice made in PB is applied in normal browsing. Is
> it expected?

Yes. As far as I understand it PB only affects the content of tabs. All other settings and panels are considered global. But it's an interesting idea to not only have private tabs but switch the whole browser into a private mode. This is not happening right now afaik.