1028691 - [Collection app] Background images sometimes in poor quality

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Everything.me, defect)

Description

[Ran Ben Aharon [:ranbena] EverythingMe]

Often newly added Smart Collections have a poor quality background image. When reopening the SC, it updates to a good quality image.

Comment 1

[Jason Smith [:jsmith]]

Need a screenshot of the poor quality wallpaper.

Comment 2

[Ran Ben Aharon [:ranbena] EverythingMe]

Attachment: Screenshot.png

Right sorry. Having trouble with getting screenshots on the device for some reason..

Comment 3

[Jason Smith [:jsmith]]

(In reply to Ran Ben Aharon (Everything.me) from comment #2)
> Created attachment 8444108 [details]
> Screenshot.png
> 
> Right sorry. Having trouble with getting screenshots on the device for some
> reason..

Ok - we'll probably want to get a screenshot of the good quality image too as a point of comparison.

Comment 4

[Ran Ben Aharon [:ranbena] EverythingMe]

Attachment: Pull Request

Since the collection.background can be updated by two processes - a) Collection create b) Collection view, the former requests a 170x170 image which is then displayed as the Collection content bg. Then gets overwritten with the full size bg.

The fix simply keeps a record if the current bg image is full sized or not and displays only if it is.

Comment 5

[Ran Ben Aharon [:ranbena] EverythingMe]

Kevin, I'd like to add a unit test with this patch though I think it'll require stubbing the api call. Not sure how to do that.

Comment 6

[Kevin Grandon :kgrandon]

Comment on attachment 8444430 [details] [review]
Pull Request

James - mind if I pass this off to you? Totally swamped, thanks!

Comment 7

[James Lal [:lightsofapollo]]

Comment on attachment 8444430 [details] [review]
Pull Request

The code itself looks okay and seems to work but I think an integration test like https://github.com/mozilla-b2g/gaia/blob/master/apps/verticalhome/test/marionette/collection_test.js is a better bet then a unit test and should be fairly easy to implement (though I would start splitting up those tests)

Comment 8

[Johan Lorenzo [:jlorenzo]]

Attachment: Smart Collection - High res

Comment 9

[Johan Lorenzo [:jlorenzo]]

The difference is slightly notable. This looks like a blocker to me. Do you confirm Jason?

Comment 10

[Peter Dolanjski [:pdol]]

Patryk, would you consider this a blocker?

Comment 11

[Jason Smith [:jsmith]]

(In reply to Johan Lorenzo [:jlorenzo] from comment #9)
> The difference is slightly notable. This looks like a blocker to me. Do you
> confirm Jason?

I'll wait for Patryk to comment, but given the VD refresh priority needed for 2.0 I'm leaning towards yes.

Comment 12

[Ran Ben Aharon [:ranbena] EverythingMe]

Attachment: comparison.jpg

Attaching a screenshot comparison. It's very noticeable imo.

Comment 13

[Preeti Raghunath(:Preeti)]

poor ux hence blocking

Comment 14

[Ran Ben Aharon [:ranbena] EverythingMe]

Comment on attachment 8444430 [details] [review]
Pull Request

Updated the PR. No change to code, only added tests.

James, I went for unit tests eventually because it seems more suitable to me in this specific case. Lmk if you have any questions about the tests implementation.

Comment 15

[James Lal [:lightsofapollo]]

Comment on attachment 8444430 [details] [review]
Pull Request

LGTM - can you rebase/land this patch?

Comment 16

[Ran Ben Aharon [:ranbena] EverythingMe]

Landed on master: https://github.com/mozilla-b2g/gaia/commit/0486f10834bb0a582b0551f31981759f114d9e72

Comment 17

[Ran Ben Aharon [:ranbena] EverythingMe]

Kevin, should I uplift to v2.0?

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Please do, it needs rebasing anyway.

Comment 19

[Ran Ben Aharon [:ranbena] EverythingMe]

Landed on v2.0: https://github.com/mozilla-b2g/gaia/commit/34f4b94233a0e03ffbb3b84dc0ed1cc52df1df1a

Comment 20

[Elie]

Attachment: VIDEO0098_Compress.MP4

This issue has been successfully verified on Flame 2.0:
Gaia-Rev        8d1e868864c8a8f1e037685f0656d1da70d08c06
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/c756bd8bf3c3
Build-ID        20141201000201
Version         32.0
Device-Name     flame
FW-Release      4.4.2


This issue has been successfully verified on Flame 2.1:
Gaia-Rev        ccb49abe412c978a4045f0c75abff534372716c4
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/18fb67530b22
Build-ID        20141201001201
Version         34.0
Device-Name     flame
FW-Release      4.4.2