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

VERIFIED FIXED in Firefox 48

Status

()

Firefox for Android
Awesomescreen
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: ahunt, Assigned: ahunt)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 verified, firefox49 verified, fennec48+)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Assignee)

Description

2 years ago
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)

Updated

2 years ago
Assignee: nobody → ahunt
Blocks: 1234314
Created attachment 8744525 [details]
Screenshot_20160422-123445.png

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)

Updated

2 years ago
tracking-fennec: --- → ?
(Assignee)

Comment 4

2 years ago
Created attachment 8746150 [details]
MozReview Request: Bug 1260149 - Only show single status icon in TwoLinePageRow r?liuche

Review commit: https://reviewboard.mozilla.org/r/49281/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49281/
Attachment #8746150 - Flags: review?(liuche)
(Assignee)

Updated

2 years ago
Blocks: 1268199
(Assignee)

Comment 5

2 years ago
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.
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED
Flags: needinfo?(ahunt)
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.
(Assignee)

Comment 8

2 years ago
(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.
(Assignee)

Comment 9

2 years ago
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+
(Assignee)

Comment 11

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/00ecd5d8bbe78c6f6969edaa54ab1ce0a722dbbc
Bug 1260149 - Only show single status icon in TwoLinePageRow r=liuche

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/00ecd5d8bbe7
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Created attachment 8748621 [details]
Screenshot_2016-05-04-16-06-27.png

Tested using:
Device: Sony Xperia (Android 5.0.2)
Build: Firefox for Android 49.0a1 (2016-05-03)

Updated

2 years ago
tracking-fennec: ? → 48+
(Assignee)

Comment 14

2 years ago
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
status-firefox49: fixed → 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+

Comment 17

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/b6dd4aaea641
status-firefox48: affected → fixed
Created attachment 8756256 [details]
aNdl1S6t.jpg

Verified as fixed using:
Device: ONE A2001 (Android 5.1.1)
Build: Firefox for Android 48.0a2 (2015-05-25)
status-firefox48: fixed → verified
You need to log in before you can comment on or make changes to this bug.