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.)

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

Review commit: https://reviewboard.mozilla.org/r/49281/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/49281/

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.

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/

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.

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

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

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.

cf comment #13

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.

Verified as fixed using: Device: ONE A2001 (Android 5.1.1) Build: Firefox for Android 48.0a2 (2015-05-25)