Closed Bug 1084062 Opened 5 years ago Closed 4 years ago

Support read/unread state in reading list UI

Categories

(Firefox for Android :: Reader View, defect)

35 Branch
All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 43
Tracking Status
firefox43 --- verified
relnote-firefox --- 43+
fennec 43+ ---

People

(Reporter: Margaret, Assigned: sebastian)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 2 obsolete files)

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.
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.
(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
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.
Attached image spec_readinglist_mock2.png (obsolete) —
Attaching spec of latest design from Bug 1082110, will unify with our other devices.
Attached image prev_readinglist_mock3.png (obsolete) —
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
Attachment #8565561 - Attachment is obsolete: true
(ignore time estimate in the mock for now :) my bad!
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).
tracking-fennec: --- → ?
This is something we've discussed to revisit and add to the current reading list for android
tracking-fennec: ? → 42+
Assignee: nobody → margaret.leibovic
tracking-fennec: 42+ → 43+
Redirecting to Sebastian, since he can probably get to this before I can.
Assignee: margaret.leibovic → s.kaspari
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
Flags: needinfo?(randersen)
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.
Flags: needinfo?(randersen)
Status: NEW → ASSIGNED
Noted! attaching new mock.
Attachment #8565591 - Attachment is obsolete: true
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
Attached file 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.
Attached image 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.
Attachment #8647586 - Flags: feedback?(alam)
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?
Flags: needinfo?(s.kaspari)
Attachment #8647586 - Flags: feedback?(alam) → feedback+
(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. ;)
Flags: needinfo?(s.kaspari)
Bug 1084062 - Support read/unread state in reading list UI. r?mhaigh
Attachment #8647931 - Flags: review?(mhaigh)
Nice! Richard might want to do a quick drive-by on the reading list DB interactions here. Just putting it on his radar.
Flags: needinfo?(rnewman)
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?
Attachment #8647931 - Flags: review?(mhaigh) → review+
(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).
Flags: needinfo?(alam)
(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.
Flags: needinfo?(alam)
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 :)
(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. :)
> <•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.
Flags: needinfo?(rnewman)
url:        https://hg.mozilla.org/integration/fx-team/rev/c6ebcbe6189ebca7d33243da8fb0b8a6c9440e63
changeset:  c6ebcbe6189ebca7d33243da8fb0b8a6c9440e63
user:       Sebastian Kaspari <s.kaspari@gmail.com>
date:       Fri Aug 14 10:17:40 2015 +0200
description:
Bug 1084062 - Support read/unread state in reading list UI. r=mhaigh
(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?
(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.
https://hg.mozilla.org/mozilla-central/rev/c6ebcbe6189e
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Depends on: 1197171
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)]:
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?
(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.
You need to log in before you can comment on or make changes to this bug.