Closed Bug 1159256 Opened 5 years ago Closed 5 years ago

720p Task Switcher App Icon Renders Aliased

Categories

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

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: padamczyk, Assigned: kgrandon)

References

Details

(Keywords: polish, Whiteboard: [systemsfe])

Attachments

(2 files)

Attached image 2015-04-27-17-06-36.png
The rendering of the app icons in the task switcher looks really rough / pixely / aliased.
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
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.
Assignee: nobody → kgrandon
Status: NEW → ASSIGNED
Whiteboard: [systemsfe]
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 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!
Attachment #8598920 - Flags: review?(sfoster)
Attachment #8598920 - Flags: review?(alive)
Comment on attachment 8598920 [details] [review]
[gaia] KevinGrandon:bug_1159256_card_icon_sizing > mozilla-b2g:master

I hope Sam review this :)
Attachment #8598920 - Flags: review?(alive)
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.
Attachment #8598920 - Flags: review?(sfoster) → review+
(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.
Keywords: checkin-needed
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.
Possible taskcluster issues, trying again.
Keywords: checkin-needed
Trying again now that TC issues seem resolved.
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
> > 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.
You need to log in before you can comment on or make changes to this bug.