1400072 - Starting Fennec in landscape and rotating to portrait causes top site overlap

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect, P1)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Screenshot

I'm guessing we're not updating our margin, or desired tile size values in our calculations. Or perhaps we are, but we never actually refresh the tile sizes (because it's set once in onCreateViewHolder? [1]).

(Note: my screenshot has a patch applied that changes the margin calculations but I can reproduce on my Nightly build)

[1]: http://searchfox.org/mozilla-central/rev/2c9a5993ac40ec1db8450e3e0a85702fa291b9e2/mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/topsites/TopSitesPageAdapter.java#96

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1400072: Move TOP_SITE_COLUMNS/ROWS -> TopSitesPage.

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

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Here's a crappy WIP patch that solves the problem but needs to be cleaned up. My high level concern is that I find the ownership of measurements to be wacky and unclear, e.g. the ActivityStreamPanel makes the measurements for the top sites items so we can calculate it once and pass it to all the pages. However, each view in the way caches it's own copy of these measurements (which is why this bug exists – one of these did not get updated) and has it's own way of being set and passing it to the child views/adapters/whatever. It'd be great if we could pass this in just in time for the methods that actually need it, e.g. bind & swapCursors, rather than caching it long term.

It really doesn't help that this code is poorly named (TopSitesPageAdapter & TopSitesPagerAdapter) and heavily nested in unexpected ways (RecyclerView contains a ViewPager contains RecyclerViews for each page?).

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Working on this now because it simplifies bug 1400014.

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1400072: Make num tiles a constant.

Since it's a constant, there's no reason to keep passing it around everywhere.
Also, the code that relied on it being a dynamic value is probably broken so
there's no reason to continue passing it around.

That being said, there is bug 1397854, which would have TopSites be 4 rows with
no pages, but this code is quite messy so I think it'd be worth trying to
refactor this code further before trying to implement that.

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

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1400072: Pass in tilesSize when needed, rather than caching.

This will help prevent the cache from going out of date.

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

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1400072: Cache tilesSize in swapCursor.

In this sense, it acts as a refresh function, binding all of our UI state at
the same time, helping to prevent bugs where the UI gets out of sync.

For example, this fixes this bug because now the tileSize gets updated when we
swap the cursor, not just when we the page is initially constructed.

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

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1400072: Rm unused variable tilesMargin.

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

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1400072: Specify card size in onBind instead of onCreate.

I believe this doesn't affect this bug because I think the ViewHolders are
recreated on rotation but for any other type of change, only bind will be
called so for correctness, we should update the size in bind.

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

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8908422 [details]
Bug 1400072: Move TOP_SITE_COLUMNS/ROWS -> TopSitesPage.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180056/diff/1-2/

Comment 10

[Chenxia Liu [:liuche]]

Comment on attachment 8908422 [details]
Bug 1400072: Move TOP_SITE_COLUMNS/ROWS -> TopSitesPage.

https://reviewboard.mozilla.org/r/180056/#review185644

Glad you split out this change from everything else.

Comment 11

[Chenxia Liu [:liuche]]

Comment on attachment 8908806 [details]
Bug 1400072: Make num tiles a constant.

https://reviewboard.mozilla.org/r/180422/#review185646

Seems reasonable, changing a constant value to explicitly be a constant.

Comment 12

[Chenxia Liu [:liuche]]

Comment on attachment 8908807 [details]
Bug 1400072: Pass in tilesSize when needed, rather than caching.

https://reviewboard.mozilla.org/r/180424/#review185648

Comment 13

[Chenxia Liu [:liuche]]

Comment on attachment 8908808 [details]
Bug 1400072: Cache tilesSize in swapCursor.

https://reviewboard.mozilla.org/r/180426/#review185652

Comment 14

[Chenxia Liu [:liuche]]

Comment on attachment 8908809 [details]
Bug 1400072: Rm unused variable tilesMargin.

https://reviewboard.mozilla.org/r/180428/#review185654

Not the last reference to this value so won't cause any lint errors, r+.

Comment 15

[Chenxia Liu [:liuche]]

Comment on attachment 8908810 [details]
Bug 1400072: Specify card size in onBind instead of onCreate.

https://reviewboard.mozilla.org/r/180430/#review185656

I assume that this was put in onCreate as an optimization so it only had to be called once instead of every time the view was bound, but this is fine to move because it seems like the assumption that it would only need to be bound on creation is incorrect.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c710d47517a2
Move TOP_SITE_COLUMNS/ROWS -> TopSitesPage. r=liuche
https://hg.mozilla.org/integration/autoland/rev/fea444196dee
Make num tiles a constant. r=liuche
https://hg.mozilla.org/integration/autoland/rev/ec4dcc7799ad
Pass in tilesSize when needed, rather than caching. r=liuche
https://hg.mozilla.org/integration/autoland/rev/45f33d2b5722
Cache tilesSize in swapCursor. r=liuche
https://hg.mozilla.org/integration/autoland/rev/cd4c24e98e04
Rm unused variable tilesMargin. r=liuche
https://hg.mozilla.org/integration/autoland/rev/62f03c05db78
Specify card size in onBind instead of onCreate. r=liuche

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/c710d47517a2
https://hg.mozilla.org/mozilla-central/rev/fea444196dee
https://hg.mozilla.org/mozilla-central/rev/ec4dcc7799ad
https://hg.mozilla.org/mozilla-central/rev/45f33d2b5722
https://hg.mozilla.org/mozilla-central/rev/cd4c24e98e04
https://hg.mozilla.org/mozilla-central/rev/62f03c05db78

Comment 18

[Bogdan Surd, QA [:bsurd, NI]]

Devices:
 - Asus ZenPad 8.0 Z380KL (Android 6.0.1);
 - Nexus 5 (Android 6.0.1);
 - HTC 10 (Android 7.0);
 - Note 4 (Android 5.0.1).

This issue was not reproducible on any of the devices I've tested on. Marking as verified.