1197907 - Add read/unread for Reader view

Status: RESOLVED WONTFIX

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

Description

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

From https://bugzilla.mozilla.org/show_bug.cgi?id=1120004#c10, we originally decided to add controls for the user to mark read/unread from inside Reader View.

Now that we have the indicators in the Reading List panel, we should probably update our controls in Reader View to allow the same thing.

Comment 1

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

Talked to Sebastian about this :)

Comment 2

[:Margaret Leibovic]

The main thing to watch out for here is that the reader view controls are shared with desktop Firefox, so you'll need to make sure you don't regress that. However, there is already CSS in place to change the set of buttons visible on desktop/Android, so this isn't uncharted territory.

You'll want to look here:
mxr.mozilla.org/mozilla-central/source/mobile/android/themes/core/aboutReaderControls.css
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/reader/content/aboutReader.html
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/reader/AboutReader.jsm

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

Oh, it thought this might be a native toolbar. Adding you as second mentor. ;)

Comment 4

[Pawel Golinski [:qiubit]]

Are those files:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/reader/content/aboutReader.html
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/reader/AboutReader.jsm

Really connected to Android's Reader View toolbar? After trying to make changes (e.g. totally deleting share button from html file and share button setup from jsm file) and building with Android Studio nothing on the toolbar changes, and Android Studio says these files are from outside the project when editing them...

Also, I'm having problems with finding a file where a toolbar layout is actually laid out (like the absolute position of buttons, e.g. that share button is to the right of font style button). Any hints as for how to find them?

Comment 5

[Pawel Golinski [:qiubit]]

After thinking about it for a while I think that a layout is laid out in html file, am I right? The question is - why do changes in AboutReader.jsm and aboutReader.html not cause rebuild in Android Studio? It's as if they don't matter, and after forcing rebuild nothing changes too (in Fennec I mean). 

However after running "./mach ide androidstudio" which also runs "./mach build" and THEN building in Android Studio I've finally noticed that changes in AboutReader.jsm were registered. So does that mean I have to run "./mach build" every time if I want to test a change in AboutReader.jsm or is there a simpler way, which would allow me to do all my work from Android Studio, or something that would save me from doing entire "./mach build" when I only need to rebuild one file?

Comment 6

[Pawel Golinski [:qiubit]]

So I just saw this: https://wiki.mozilla.org/Mobile/Fennec/Android/IDEs#Why_aren.27t_my_JavaScript_changes_being_noticed.3F which answers most of my questions. One more thing: how should I aquire .pngs for read/unread button?

Comment 7

[Pawel Golinski [:qiubit]]

And, more maybe even more important, how to aquire the information about article state (read/unread) in order to know which button to display on the toolbar? If I'm understanding it correctly, I would have to do it similairly to the add/delete from Reading List button on the toolbar - does that mean I need to make new signals to communicate with Java components (like "Reader:MarkedRead" and "Reader:MarkedUnread"), fire these signals whenever user changes state to read/unread from outside Reader View and make AboutReader respond to these signals by changing displayed button on the toolbar? Also, how to determine initial state of article (like when setting up the toolbar initially) - it seems that information about article being read/unread is only available in database now, does that mean I need to communicate with it when setting up Reader View (before drawing UI) in order to know whether loaded article is read or unread or maybe I should find other way to deal with that?

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

Hi Pawel,

seems like you already fixed your build problem. Well done! :)

(In reply to Pawel Golinski [:qiubit] from comment #6)
> One more thing: how should I aquire .pngs for read/unread button?

For the native Android UI we decided to use drawable XMLs: reading_list_indicator_unread.xml and reading_list_indicator_read.xml. However we have some PNGs attached to bug 1084062 too. It's okay to use them as long as we remove the XML files and use the PNGs for the native UI too (Can be in a follow-up bug).


(In reply to Pawel Golinski [:qiubit] from comment #7)
> And, more maybe even more important, how to aquire the information about
> article state (read/unread) in order to know which button to display on the
> toolbar? If I'm understanding it correctly, I would have to do it similairly
> to the add/delete from Reading List button on the toolbar - does that mean I
> need to make new signals to communicate with Java components (like
> "Reader:MarkedRead" and "Reader:MarkedUnread"), fire these signals whenever
> user changes state to read/unread from outside Reader View and make
> AboutReader respond to these signals by changing displayed button on the
> toolbar? Also, how to determine initial state of article (like when setting
> up the toolbar initially) - it seems that information about article being
> read/unread is only available in database now, does that mean I need to
> communicate with it when setting up Reader View (before drawing UI) in order
> to know whether loaded article is read or unread or maybe I should find
> other way to deal with that?

Unfortunately I don't know this component very much and Margaret is on PTO currently. But your approach sounds good. You'll probably need three evens from the JS side (MarkRead, MarkUnread, ReadStatusRequest [Something like that]) and from the Java side just an event type to notify about Read/Unread changes. Actually MarkRead and MarkUnread could be just one event with a boolean indicating whether we mark the item as read or unread. And I think it's perfectly fine to let the reader view render itself and query&display the read/unread state asynchronously.

On the Java side it seems like ReadingListHelper handles a bunch of related events and it has a readingListAccessor instance to modify the database. Let's start here and get it working in a first version. After that we can move things around and make it better. For the final review I'd wait for Margaret anyways. :)

Comment 9

[Pawel Golinski [:qiubit]]

Attachment: Bug 1197907 - Proof of concept

OK, I think I managed to get it to work now - here's how my |hg diff| looks right now. I'm intending to refactor most of this code now - any tips regarding that? (like what would you do differently, what is wrong with the code now, etc...) It was really challenging for me to understand how everything works so I guess there's a lot of things that can be changed about it now that I've finally got it to work.

About those .pngs - I guess we need to use them instead of xml (which uses Android API, right?), unless there is a way to draw it in JavaScript (entire Reader Mode is in JS and HTML...)

One more thing - there's a problem when user deletes an article it's reading - the "read/unread" state is undefined then (and I don't think it even exists in database) and "read/unread" indicator doesn't change when user does that. What should I do in that case?

Comment 10

[:Margaret Leibovic]

Comment on attachment 8656920 [details] [diff] [review]
Bug 1197907 - Proof of concept

I am finally back from vacation! I'm adding this to my review queue and will try to get to it tomorrow. Thanks for the patch!

Comment 11

[Pawel Golinski [:qiubit]]

Just some quick thoughts - in final version I plan to use only "Reader:ReadStatusChanged" and "Reader:ReadStatusChangeRequest" instead of verbose "Reader:MarkRead", "Reader:MarkUnread", "Reader:MarkedRead", "Reader:MarkedUnread" (status will be in message field), that will shorten the code by quite a margin. Also, I'm reallly confused when it comes to Reading List item ID - I don't understand when they are set and where can I fetch them in order to execute "Reader:ReadStatusChangeRequest" (the provided markAsRead() and markAsUnread() functions use this ID instead of URL, and actually fetching the ID somehow would mean that I don't need to modify files accessing the database which would simplify the final patch, I would need the ID in Reader.js when sending the message to Java).

Comment 12

[:Margaret Leibovic]

(In reply to Pawel Golinski [:qiubit] from comment #11)
> Just some quick thoughts - in final version I plan to use only
> "Reader:ReadStatusChanged" and "Reader:ReadStatusChangeRequest" instead of
> verbose "Reader:MarkRead", "Reader:MarkUnread", "Reader:MarkedRead",
> "Reader:MarkedUnread" (status will be in message field), that will shorten
> the code by quite a margin.

Sounds good! Reducing the number of messages is good for reducing complexity.

> Also, I'm reallly confused when it comes to
> Reading List item ID - I don't understand when they are set and where can I
> fetch them in order to execute "Reader:ReadStatusChangeRequest" (the
> provided markAsRead() and markAsUnread() functions use this ID instead of
> URL, and actually fetching the ID somehow would mean that I don't need to
> modify files accessing the database which would simplify the final patch, I
> would need the ID in Reader.js when sending the message to Java).

Good question. As you've discovered, reader view doesn't know about reading list IDs, and an article in reader view isn't necessarily in your reading list.

I think it's fine to mark items as read based on the URL, but we should make sure that you can't save duplicate URLs to your reading list. We should already have similar logic for bookmarks, since you can use the bookmark star to bookmark/unbookmark a URL.

How does this patch behave when an article isn't in your reading list? Anthony, is this a use case we've thought about? It would be weird to change the UI state if the article isn't even in your reading list. Maybe we should hide this button if the article isn't in your reading list?

Comment 13

[:Margaret Leibovic]

Comment on attachment 8656920 [details] [diff] [review]
Bug 1197907 - Proof of concept

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

I just did a quick review, but this generally looks good. I'd like to see a final version of the patch before doing a full review (and also hear back from Anthony about the UX for articles that aren't in your reading list).

::: mobile/android/base/ReadingListHelper.java
@@ +105,5 @@
>                  handleReadingListStatusRequest(callback, message.getString("url"));
>                  break;
>              }
> +            case "Reader:ReadStatusRequest": {
> +                Log.e("ReadingListHelper", "Got Reader:ReadStatusRequest");

Please remove this logging (and all other debug logging) for the final version of the patch.

::: mobile/android/base/db/LocalReadingListAccessor.java
@@ +202,5 @@
>          context.getContentResolver().registerContentObserver(mReadingListUriWithProfile, false, observer);
>      }
>  
>      @Override
> +    public void markAsRead(ContentResolver cr, String uri) {

Rather than create separate method signatures, could we update the other use of markAsRead/markAsUnread to take a URL parameter, and then work to make sure you can't add a URL to your reading list more than once?

Comment 14

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

(In reply to :Margaret Leibovic from comment #12)
> How does this patch behave when an article isn't in your reading list?
> Anthony, is this a use case we've thought about? It would be weird to change
> the UI state if the article isn't even in your reading list. Maybe we should
> hide this button if the article isn't in your reading list?

My thoughts exactly. 

Also, if you mark as unread from inside Reader View, it should stay unread until you leave, and re-open that same article.

Hope that helps!

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8656920 [details] [diff] [review]
Bug 1197907 - Proof of concept

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

(In reply to Pawel Golinski [:qiubit] from comment #11)
> Just some quick thoughts - in final version I plan to use only
> "Reader:ReadStatusChanged" and "Reader:ReadStatusChangeRequest" instead of
> verbose "Reader:MarkRead", "Reader:MarkUnread", "Reader:MarkedRead",
> "Reader:MarkedUnread" (status will be in message field), that will shorten
> the code by quite a margin.

Nice!

(In reply to Pawel Golinski [:qiubit] from comment #11)
> Also, I'm reallly confused when it comes to
> Reading List item ID - I don't understand when they are set and where can I
> fetch them in order to execute "Reader:ReadStatusChangeRequest" (the
> provided markAsRead() and markAsUnread() functions use this ID instead of
> URL, and actually fetching the ID somehow would mean that I don't need to
> modify files accessing the database which would simplify the final patch, I
> would need the ID in Reader.js when sending the message to Java).

These IDs are related to Android and its SQLite storage. If these are the same views we use on desktop and use URLs in other places (How is adding/remove articles done?) then I guess that's okay here too. I wrote markAsRead/Unread using IDs because I only needed them for the native UI and there we always have the IDs. We can later, in a follow-up bug, try to use the URL methods there too - if it makes sense.

(In reply to Pawel Golinski [:qiubit] from comment #9)
> About those .pngs - I guess we need to use them instead of xml (which uses
> Android API, right?), unless there is a way to draw it in JavaScript (entire
> Reader Mode is in JS and HTML...)

Yes, that's absolutely fine. As soon as we are using the PNGs here, let's use them everywhere and remove the XML. Can be in a follow-up bug too.

(In reply to Pawel Golinski [:qiubit] from comment #9)
> One more thing - there's a problem when user deletes an article it's reading
> - the "read/unread" state is undefined then (and I don't think it even
> exists in database) and "read/unread" indicator doesn't change when user
> does that. What should I do in that case?

I guess you are going to have the same problem when the user is switching to reader mode without ever adding the page to the reading list. However if it's not part of the reading list anymore (deleted) then there's no way of tracking read/unread states, so let's just hide it.

Comment 16

[Pawel Golinski [:qiubit]]

Attachment: Bug 1197907 - Add read/unread for Reader view

OK, here's a patch with verbose code deleted. A few things about it:

1. I didn't delete old markAsRead(), markAsUnread() yet, should I do this? In my opinion it would be better to do that in a follow-up bug because this patch is already really complex.

2. The button is now hidden if the article isn't on reading list, as requested.

3. As for UI, I decided to use custom made .svgs (with color data grabbed from existing drawables). That avoids using extra files for different resolution phones/tablets. These .svgs cannot be reused for Reading List however (where drawables are used) because we have different background colors for Reading List and Reader Mode toolbar, and if I'm understanding this correctly, the inside circle of read state indicator should be in the same color as the background (white looked weird on the toolbar). Do you think I should solve that problem in some other way?

Comment 17

[:Margaret Leibovic]

(In reply to Pawel Golinski [:qiubit] from comment #16)
> Created attachment 8666369 [details] [diff] [review]
> Bug 1197907 - Add read/unread for Reader view
> 
> OK, here's a patch with verbose code deleted. A few things about it:
> 
> 1. I didn't delete old markAsRead(), markAsUnread() yet, should I do this?
> In my opinion it would be better to do that in a follow-up bug because this
> patch is already really complex.

Sure, I agree that sounds like a good follow-up.

> 2. The button is now hidden if the article isn't on reading list, as
> requested.

Thanks.

> 3. As for UI, I decided to use custom made .svgs (with color data grabbed
> from existing drawables). That avoids using extra files for different
> resolution phones/tablets. These .svgs cannot be reused for Reading List
> however (where drawables are used) because we have different background
> colors for Reading List and Reader Mode toolbar, and if I'm understanding
> this correctly, the inside circle of read state indicator should be in the
> same color as the background (white looked weird on the toolbar). Do you
> think I should solve that problem in some other way?

Do you have some screenshots you can share? If the inside of the circle should be the same color as the background, could you just use a transparent background?

Comment 18

[:Margaret Leibovic]

Comment on attachment 8666369 [details] [diff] [review]
Bug 1197907 - Add read/unread for Reader view

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

Thanks for updating your patch! I want to look at this a bit more, but I think I have enough feedback to address that I should share it now and look at an updated version of the patch.

::: mobile/android/base/ReadingListHelper.java
@@ +283,5 @@
> +
> +        ThreadUtils.postToBackgroundThread(new Runnable() {
> +            @Override
> +            public void run() {
> +                if (status)

Nit: Please use braces for all if/else statements, to match our code style.

::: mobile/android/base/db/LocalReadingListAccessor.java
@@ +103,5 @@
> +
> +        final Cursor c = cr.query(mReadingListUriWithProfile,
> +                                  new String[] { ReadingListItems._ID },
> +                                  ReadingListItems.URL + " = ? OR " + ReadingListItems.RESOLVED_URL + " = ? " +
> +                                  "AND " + ReadingListItems.IS_UNREAD + " = 0",

It seems like it would be more intuitive to just query for the IS_UNREAD status and return that. Is there a reason why you chose to do it this way?

@@ +222,5 @@
> +            json.put("status", true);
> +            GeckoAppShell.sendEventToGecko(
> +                    GeckoEvent.createBroadcastEvent("Reader:ReadStatusChanged", json.toString()));
> +        } catch (JSONException e) {
> +            Log.e(LOG_TAG, "Failed to mark reading list item as read");

This error message isn't quite accurate... in this case, we failed to send a message to JS with the update.

@@ +256,5 @@
> +                        GeckoAppShell.sendEventToGecko(
> +                                GeckoEvent.createBroadcastEvent("Reader:ReadStatusChanged", json.toString()));
> +                        json = new JSONObject();
> +                        json.put("url", resolvedUri);
> +                        json.put("status", true);

Instead of making a a new JSON object, you could just use the existing object and update the url field. Can you explain why you need to send this message twice with different url values?

@@ +274,5 @@
> +    public void markAsUnread(ContentResolver cr, String uri) {
> +        uri = ReaderModeUtils.stripAboutReaderUrl(uri);
> +
> +        final ContentValues values = new ContentValues();
> +        values.put(ReadingListItems.IS_UNREAD, 1);

Should we also reset the MARKED_READ_BY and MARKED_READ_ON fields in this case?

@@ +319,5 @@
> +                        json = new JSONObject();
> +                        json.put("url", resolvedUri);
> +                        json.put("status", false);
> +                        GeckoAppShell.sendEventToGecko(
> +                                GeckoEvent.createBroadcastEvent("Reader:ReadStatusChanged", json.toString()));

Same question about double messages applies here.

::: mobile/android/themes/core/images/icon-read.svg
@@ +1,4 @@
> +<?xml version="1.0" encoding="UTF-8" standalone="no"?>
> +<!-- Created with Inkscape (http://www.inkscape.org/) -->
> +
> +<svg

I haven't looked through this file closely, but you should make sure it matches our SVG style guidelines:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/SVG_Guidelines

::: toolkit/components/reader/AboutReader.jsm
@@ +125,5 @@
>    this._isReadingListItem = -1;
>    this._updateToggleButton();
>  
> +  this._isRead = -1;
> +  this._updateReadButton();

Since the button defaults to hidden in the html, you don't neeed to call _updateReadButton here.

@@ +297,5 @@
> +      button.setAttribute("hidden", true);
> +    } else {
> +      button.removeAttribute("hidden");
> +      if (this._isRead == 1) {
> +        button.classList.add("on");

I find "on" to be a very un-intuitive class name here. What about "read", or even "isRead"?

@@ +371,5 @@
> +    if (!this._article)
> +      return;
> +
> +    if (this._isReadingListItem != 1) {
> +      this._updateReadButton();

How would we ever end up in this case? Shouldn't the mark as read button be hidden if the item isn't a ready list item?

Comment 19

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to :Margaret Leibovic from comment #18)
> @@ +274,5 @@
> > +    public void markAsUnread(ContentResolver cr, String uri) {
> > +        uri = ReaderModeUtils.stripAboutReaderUrl(uri);
> > +
> > +        final ContentValues values = new ContentValues();
> > +        values.put(ReadingListItems.IS_UNREAD, 1);
> 
> Should we also reset the MARKED_READ_BY and MARKED_READ_ON fields in this
> case?

I talked to rnewman about this in bug 1084062 comment#25:

> <•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

Comment 20

[Pas]

Hi, I am new to fixing bugs and I would like to work on the bug please.

Comment 21

[:Margaret Leibovic]

(In reply to Pas from comment #20)
> Hi, I am new to fixing bugs and I would like to work on the bug please.

We are considering retiring our reading list feature, so I don't think we should spend time working on this bug.

If you're looking for a bug to start, here's a good list:
http://www.joshmatthews.net/bugsahoy/?mobileandroid=1&simple=1

Sorry about that!