1159256 - 720p Task Switcher App Icon Renders Aliased

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System::Task Manager, defect)

Description

[Patryk Adamczyk [:patryk] UX]

Attachment: 2015-04-27-17-06-36.png

The rendering of the app icons in the task switcher looks really rough / pixely / aliased.

Comment 1

[Kevin Grandon :kgrandon]

I think we need to update the icon select code here: https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/cards_helper.js#L21

We could possibly use the same strategy that we use for home screens: https://github.com/mozilla-b2g/gaia/blob/master/shared/elements/gaia_grid/js/items/grid_item.js#L226

Comment 2

[Kevin Grandon :kgrandon]

I looked and I think we're just loading in *too* big of an image.

On a Z3 we render the icon at 4rem * 3.0 DPI = 120px.

We're currently selecting the largest image we have, which is the 284px image, instead of something a little more sane like 126px.

Should be a fairly simple fix.

Comment 3

[Kevin Grandon :kgrandon]

The identification overlay also has problems, expecting to render at 6rem * 3.0 DPI = 180. Instead of loading the 284px icon, we should probably load the 189px icon.

Comment 4

[GitHub Autolander Bot]

Attachment: [gaia] KevinGrandon:bug_1159256_card_icon_sizing > mozilla-b2g:master

Comment 5

[Kevin Grandon :kgrandon]

Comment on attachment 8598920 [details] [review]
[gaia] KevinGrandon:bug_1159256_card_icon_sizing > mozilla-b2g:master

Sam or Alive - either of you have time for a review? Thanks!

Comment 6

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8598920 [details] [review]
[gaia] KevinGrandon:bug_1159256_card_icon_sizing > mozilla-b2g:master

I hope Sam review this :)

Comment 7

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8598920 [details] [review]
[gaia] KevinGrandon:bug_1159256_card_icon_sizing > mozilla-b2g:master

R+ with a unit test for the CardsHelper.getIconURIForApp changes, and the size <= vs size < nit I noted in the PR. 

Thanks for the clean up in there also. I know Alive wants to get rid of this helper eventually - its a leftover from when I was refactoring the old card view - that should be pretty trivial at this point.

Comment 8

[Kevin Grandon :kgrandon]

(In reply to Sam Foster [:sfoster] from comment #7)
> Comment on attachment 8598920 [details] [review]
> [gaia] KevinGrandon:bug_1159256_card_icon_sizing > mozilla-b2g:master
> 
> R+ with a unit test for the CardsHelper.getIconURIForApp changes, and the
> size <= vs size < nit I noted in the PR. 

I replied on github why I think the current implementation is correct. Let me know if you think that's incorrect.

Comment 9

[GitHub Autolander Bot]

http://docs.taskcluster.net/tools/task-graph-inspector/#Zzc7Sa8NRUCMjH1PytuvkA

The pull request failed to pass integration tests. It could not be landed, please try again.

Comment 10

[Kevin Grandon :kgrandon]

Possible taskcluster issues, trying again.

Comment 11

[Kevin Grandon :kgrandon]

Trying again now that TC issues seem resolved.

Comment 12

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/c3308bcf82e7d1e6ce3f16afbd26d78d6b129437

Comment 13

[Sam Foster [:sfoster] (he/him)]

> > R+ with a unit test for the CardsHelper.getIconURIForApp changes, and the
> > size <= vs size < nit I noted in the PR. 
> 
> I replied on github why I think the current implementation is correct. Let
> me know if you think that's incorrect.

Ok yeah I follow now, thanks for the clarification. Maybe the logic could be clearer with explicit if/else in the loop, but its fine as-is IMO. A unit test would do a better job anyhow of conveying and confirming how its supposed to work. I've filed bug 1160202 for that.