Closed Bug 1260149 Opened 8 years ago Closed 8 years ago

Show only readercache icon, or readercache and bookmark icon, for offline readermode items in search results

Categories

(Firefox for Android Graveyard :: Awesomescreen, defect)

defect
Not set
normal

Tracking

(firefox48 verified, firefox49 verified, fennec48+)

VERIFIED FIXED
Firefox 49
Tracking Status
firefox48 --- verified
firefox49 --- verified
fennec 48+ ---

People

(Reporter: ahunt, Assigned: ahunt)

References

Details

Attachments

(4 files)

Bug 1234328 adds a readercache icon.

When searching, we indicated bookmark results with a blue star. Do we want to show the readercache icon for offline bookmarks instead of, or in addition to, the bookmark star?

(The current patches in Bug 1234328 show both icons side-by-side, this is something we can fix after landing the main migration therefore I don't want to block on for now.)
Assignee: nobody → ahunt
Blocks: migrate-RL
I assume this is the screenshot this bug is about?
(In reply to Andrzej Hunt :ahunt from comment #0)
> Bug 1234328 adds a readercache icon.
> 
> When searching, we indicated bookmark results with a blue star. Do we want
> to show the readercache icon for offline bookmarks instead of, or in
> addition to, the bookmark star?

Since "available offline" is a subset of a users saved stuff, we should start by only showing the "available offline" icon. The asset is available in bug 1266899.
Also, note that the bug here is also that the position of the star has shifted to the left (even when there is no "Available offline" icon there). 

We should make sure it's back in its original position and not shifted over
Flags: needinfo?(ahunt)
tracking-fennec: --- → ?
Blocks: 1268199
Using two separate ImageViews seems a bit brittle and could be simplified, however we'll want to uplift this to 48 - I filed Bug 1268199 to take care of simplifying that without overcomplicating this bug.
Status: NEW → ASSIGNED
Flags: needinfo?(ahunt)
Attachment #8746150 - Flags: review?(liuche)
Comment on attachment 8746150 [details]
MozReview Request: Bug 1260149 - Only show single status icon in TwoLinePageRow r?liuche

https://reviewboard.mozilla.org/r/49281/#review46147

I don't think that combining these two into a single icon would be difficult to uplift, so I'd suggest just doing that here. However, this patch doesn't address the offset of the icons.

::: mobile/android/base/java/org/mozilla/gecko/home/TwoLinePageRow.java:208
(Diff revision 1)
>  
>      private void showBookmarkIcon(boolean toShow) {
>          final int visibility = toShow ? VISIBLE : GONE;
> +
> +        if (toShow && mReaderCached.getVisibility() == View.VISIBLE) {
> +            mStatusIcon.setVisibility(View.GONE);

You should also update the visibility of readercache icon to be GONE if you're going this route.
To clarify, to use a single ImageView in this bug, you should just manually set the drawable in a method updateIcon() or something without needing to go all the way and create the selectors and attrs you suggested in bug 1268199.
(In reply to Chenxia Liu [:liuche] from comment #7)
> To clarify, to use a single ImageView in this bug, you should just manually
> set the drawable in a method updateIcon() or something without needing to go
> all the way and create the selectors and attrs you suggested in bug 1268199.

That's... a lot simpler. And makes the existing code much simpler too. I've done that in my updated patch.
Comment on attachment 8746150 [details]
MozReview Request: Bug 1260149 - Only show single status icon in TwoLinePageRow r?liuche

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49281/diff/1-2/
Attachment #8746150 - Attachment description: MozReview Request: Bug 1260149 - Hide bookmark star if page is in reader view cache r?liuche → MozReview Request: Bug 1260149 - Only show single status icon in TwoLinePageRow r?liuche
Attachment #8746150 - Flags: review?(liuche)
Comment on attachment 8746150 [details]
MozReview Request: Bug 1260149 - Only show single status icon in TwoLinePageRow r?liuche

https://reviewboard.mozilla.org/r/49281/#review46783

Great! Thanks for making this one ImageView.
Attachment #8746150 - Flags: review?(liuche) → review+
https://hg.mozilla.org/integration/fx-team/rev/00ecd5d8bbe78c6f6969edaa54ab1ce0a722dbbc
Bug 1260149 - Only show single status icon in TwoLinePageRow r=liuche
https://hg.mozilla.org/mozilla-central/rev/00ecd5d8bbe7
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Tested using:
Device: Sony Xperia (Android 5.0.2)
Build: Firefox for Android 49.0a1 (2016-05-03)
tracking-fennec: ? → 48+
Comment on attachment 8746150 [details]
MozReview Request: Bug 1260149 - Only show single status icon in TwoLinePageRow r?liuche

Approval Request Comment
[Feature/regressing bug #]: Bug 1234328
[User impact if declined]: Two icons  (star+offline icon) shown side by side (without padding) for offline reader view items, this looks bad, and provides redundant information.
[Describe test coverage new/current, TreeHerder]: Manual testing, patch has been on nightly for a few weeks.
[Risks and why]: Medium risk: new logic to determine which icons are shown, we only use one ImageView now where the appropriate icon is displayed instead of hiding both icons as necessary.
[String/UUID change made/needed]: None.
Attachment #8746150 - Flags: approval-mozilla-aurora?
cf comment #13
Status: RESOLVED → VERIFIED
Comment on attachment 8746150 [details]
MozReview Request: Bug 1260149 - Only show single status icon in TwoLinePageRow r?liuche

Has been verified in nightly, polish a new feature, we still have the full beta cycle to fix the potential regressions, taking it in aurora.
Attachment #8746150 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attached image aNdl1S6t.jpg
Verified as fixed using:
Device: ONE A2001 (Android 5.1.1)
Build: Firefox for Android 48.0a2 (2015-05-25)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.