Closed Bug 1234328 Opened 4 years ago Closed 4 years ago

Indicate when an item is saved in Reader View

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- verified

People

(Reporter: antlam, Assigned: ahunt)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(7 files, 1 obsolete file)

Attached image prev_bookmarks_RLfolder2c.png (obsolete) —
Not sure about how possible this is, but if an item is saved in Reader View, it'd be useful to denote that. Especially if the RL is a folder in under Bookmarks - this would have the added benefit of showing what's probably "available offline".

There are specs in bug 1232866 about the layout. But I'll attach the UI here too. (focus on the item with the Reader View icon)
First, we'll want bug 1234331.

Right now we only save URLs for reading list items, so we don't know whether or not a user actually saved them from reader view.
Depends on: 1234331
Depends on: 1126244
Bug 1232866 has the spec for this design.
Updating designs.

Note: the icon on the far right follows the same design spec as previously mentioned in comment 2. I'll attach the image, but it will just take the spot that the star normally would occupy.
Attachment #8700738 - Attachment is obsolete: true
Attached file icon_downloaded.zip
Attaching assets for the down-arrow.
This means we can see the readercache availability not only inside bookmarks, but also
in most other parts of the awesomescreen, since we use TwoLinePageRow for most items
(history, bookmarks, topsites, search results etc).

Review commit: https://reviewboard.mozilla.org/r/40897/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40897/
Attachment #8731879 - Flags: review?(s.kaspari)
We will already switch to the currently open tab, so we need to ensure we
show the "switch to tab" indicator.

Review commit: https://reviewboard.mozilla.org/r/40901/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40901/
Attachment #8731881 - Flags: review?(s.kaspari)
Assignee: nobody → ahunt
Attachment #8731879 - Flags: review?(s.kaspari) → review+
Comment on attachment 8731879 [details]
MozReview Request: Bug 1234328 - Add readercache icon to TwoLinePageRow r?sebastian

https://reviewboard.mozilla.org/r/40897/#review37659

r+ with the changes below.

So far I only saw mocks with one icon on the right. But it should be possible that we show multiple (bookmark and reader cache in this case) - did antlam review/mock this? What about an item that is bookmarked, has a reader cache item available and I take a screenshot of it. Do we need a ranking of the icons here?

Also I saw Mike create a new layout for the item in the screenshots smart folder. Is this something we need too?

::: mobile/android/base/java/org/mozilla/gecko/home/TwoLinePageRow.java:47
(Diff revision 1)
>      private final ImageView mStatusIcon;
>  
>      private int mSwitchToTabIconId;
>  
>      private final FaviconView mFavicon;
> +    private final View mCached;

Let's be more explicit and make it more clear that this is the reader cache. Cache can mean a bunch of things in the context of a browser.

::: mobile/android/base/java/org/mozilla/gecko/home/TwoLinePageRow.java:267
(Diff revision 1)
>              ReaderModeUtils.getUrlFromAboutReader(url) : url;
>          mLoadFaviconJobId = Favicons.getSizedFaviconForPageFromLocal(getContext(), pageURL, mFaviconListener);
>  
>          updateDisplayedUrl(url);
> +
> +        mCached.setVisibility(hasCacheItem ? View.VISIBLE : View.INVISIBLE);

Shouldn't this be GONE instead of INVISIBLE?

::: mobile/android/base/resources/layout/two_line_page_row.xml:51
(Diff revision 1)
> +    <ImageView android:id="@+id/iscached"
> +               android:layout_width="wrap_content"
> +               android:layout_height="wrap_content"
> +               android:layout_gravity="center_vertical"
> +               android:src="@drawable/ic_downloaded"
> +               android:padding="16dp"/>

Let's set this to "gone" like status_icon_bookmark because this is the default.

It's probably more consistent to use an id like status_icon_readercache
Comment on attachment 8731880 [details]
MozReview Request: Bug 1234328 - Open readercached items into readermode r?sebastian

https://reviewboard.mozilla.org/r/40899/#review37663

Do we really want to have this everywhere? Clicking on a top sites item opens the reader view if I have a cached version?
Attachment #8731880 - Flags: review?(s.kaspari)
Comment on attachment 8731880 [details]
MozReview Request: Bug 1234328 - Open readercached items into readermode r?sebastian

https://reviewboard.mozilla.org/r/40899/#review37677

I briefly talked to antlam and he seems to be okay with opening in reader view everywhere we show the icon.

::: mobile/android/base/java/org/mozilla/gecko/home/BrowserSearch.java:347
(Diff revision 1)
> -                mUrlOpenListener.onUrlOpen(url, EnumSet.of(OnUrlOpenListener.Flags.ALLOW_SWITCH_TO_TAB));
> +                mUrlOpenListener.onUrlOpen(ReaderCacheHelper.getReaderUrlIfCached(getContext(), url),
> +                                           EnumSet.of(OnUrlOpenListener.Flags.ALLOW_SWITCH_TO_TAB));

This code is everywhere. I wonder if we could move it into the UrlOpenListener implementation - do we have multiple of that or is it just one anyways? It's likely that we forget to add this to every onUrlOpen() call manually (for example liuche's new panel probably doesn't have this).
Comment on attachment 8731881 [details]
MozReview Request: Bug 1234328 - Show "switch to tab" for readercached items too r?sebastian

https://reviewboard.mozilla.org/r/40901/#review37679
Attachment #8731881 - Flags: review?(s.kaspari) → review+
https://reviewboard.mozilla.org/r/40897/#review37659

> Shouldn't this be GONE instead of INVISIBLE?

I went for INIVISIBLE to keep a constant width for the bookmark titles.

I.e. :antlams preview has all the bookmark titles fading in the same position (regardless of offline status):
https://bug1234328.bmoattachments.org/attachment.cgi?id=8716353

Whereas if we use GONE we end up with varying widths:
https://bug1234328.bmoattachments.org/attachment.cgi?id=8732399

Should I instead hardcode the needed space, and draw the icon on top when needed? (I'm guessing there's some performance hit to calculating the space required for the icon if we use INVISISBLE, but I'm not quite sure what the best way of layouting the icon in that case would be.)
Verified as fixed using:
Device: Nexus 6 (Android 6.0)
Build: Firefox for Android 48.0a1 (2016-04-10)
You need to log in before you can comment on or make changes to this bug.