1025812 - Last added pin to Top Sites Grid has the name grayed out

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Ioana Chiorean]

Attachment: Screenshot

OS: Android 4.4 
Device: Nexus 4

From bug 1025062 we had that at the last added site to top sites grid you will have the name grayed out till u have a full pinned grid. 

while searching a regression for that bug I found the one for this :
last good: 05/07
first bad: 05/08
Pushlog:http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5bbc85136202&tochange=2a03b34c8953

Comment 1

[Aaron Train [:aaronmt]]

Either bug 997660 or bug 996657.

Comment 2

[Lucas Rocha (:lucasr)]

Attachment: Part 1: Factor out type updates into separate method (r=rnewman)

Comment 3

[Lucas Rocha (:lucasr)]

Attachment: Part 2: Refresh TopSitesGridItemView's drawable state when type changes (r=rnewman)

Comment 4

[Lucas Rocha (:lucasr)]

Comment on attachment 8451081 [details] [diff] [review]
Part 1: Factor out type updates into separate method (r=rnewman)

Update mType and compound drawables in a single spot. Prep work for the actual fix.

Comment 5

[Lucas Rocha (:lucasr)]

Comment on attachment 8451082 [details] [diff] [review]
Part 2: Refresh TopSitesGridItemView's drawable state when type changes (r=rnewman)

In bug 997660, I changed the grid item's drawable state to depend on its type instead of the URL/title. So, we must refresh the drawable state whenever the type changes.

Comment 6

[Richard Newman [:rnewman]]

Comment on attachment 8451082 [details] [diff] [review]
Part 2: Refresh TopSitesGridItemView's drawable state when type changes (r=rnewman)

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

::: mobile/android/base/home/TopSitesGridItemView.java
@@ +294,5 @@
>              return false;
>          }
>  
>          mType = type;
> +        refreshDrawableState();

Is there a good reason to do this here, rather than after futzing with mTitleView on line 301?

Comment 7

[Lucas Rocha (:lucasr)]

(In reply to Richard Newman [:rnewman] from comment #6)
> Comment on attachment 8451082 [details] [diff] [review]
> Part 2: Refresh TopSitesGridItemView's drawable state when type changes
> (r=rnewman)
> 
> Review of attachment 8451082 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/home/TopSitesGridItemView.java
> @@ +294,5 @@
> >              return false;
> >          }
> >  
> >          mType = type;
> > +        refreshDrawableState();
> 
> Is there a good reason to do this here, rather than after futzing with
> mTitleView on line 301?

Doing this whenever the type changes is more future-proof because the drawable state only depends on type now.

Comment 8

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/integration/fx-team/rev/d61143130bd1
https://hg.mozilla.org/integration/fx-team/rev/cf89a2c171e4

Comment 9

[Lucas Rocha (:lucasr)]

Comment on attachment 8451081 [details] [diff] [review]
Part 1: Factor out type updates into separate method (r=rnewman)

Approval Request Comment
[Feature/regressing bug #]: bug 988366
[User impact if declined]: Top sites grid will show items in an incorrect state i.e. greyed out title for normal grid cells.   
[Describe test coverage new/current, TBPL]: Local tests only. Let's bake this in Nightly for a couple days and then uplift.
[Risks and why]: Low, just changes the code to update the drawable state of each grid item.
[String/UUID change made/needed]: n/a

Comment 10

[Lucas Rocha (:lucasr)]

Comment on attachment 8451082 [details] [diff] [review]
Part 2: Refresh TopSitesGridItemView's drawable state when type changes (r=rnewman)

Approval Request Comment
[Feature/regressing bug #]: bug 988366
[User impact if declined]: Top sites grid will show items in an incorrect state i.e. greyed out title for normal grid cells.   
[Describe test coverage new/current, TBPL]: Local tests only. Let's bake this in Nightly for a couple days and then uplift.
[Risks and why]: Low, just changes the code to update the drawable state of each grid item.
[String/UUID change made/needed]: n/a

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d61143130bd1
https://hg.mozilla.org/mozilla-central/rev/cf89a2c171e4

Comment 12

[Cristina Madaras, QA [:CristinaM]]

Verified as fixed in build:
Nightly 33.0a1 (2014-07-09)

Devices:
Motorola Razr (Android 4.0.4)
Galaxy Nexus (Android 4.2.1)

Comment 13

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8451081 [details] [diff] [review]
Part 1: Factor out type updates into separate method (r=rnewman)

Aurora+

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a2ee1d8b0f0c
https://hg.mozilla.org/releases/mozilla-aurora/rev/534d0fd2f008

Comment 15

[Cristina Madaras, QA [:CristinaM]]

Verified as fixed in:
Firefox for Android 32.0a2 (2014-07-13)
Device:
Galaxy Nexus (Android 4.2.1)