Closed
Bug 989983
Opened 10 years ago
Closed 10 years ago
Use un-manipulated app icons for Collection icon
Categories
(Firefox OS Graveyard :: Gaia::Everything.me, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: amirn, Assigned: amirn)
References
Details
User Story
Collection icon renders different size for app dragged from homescreen or added as webapp
Attachments
(3 files)
STR: 1. open a collection 2. long tap on a webapp and choose "save to homescreen" 3. long tap again and choose "add to top of collection" and set as the first app 4. drag the app added in step 2 to another collection 5. notice the collection icons render different sizes for the same app
Assignee | ||
Comment 1•10 years ago
|
||
This happens because E.me uses the renderedIcon provided by homescreen which is 64x64 and includes shadow: https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/everything.me/js/etmmanager.js#L182 Cristian, is it possible to get the original (no shadow) icon blob?
Flags: needinfo?(crdlc)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → amirn
Comment 2•10 years ago
|
||
(In reply to Amir Nissim (Everything.me) from comment #1) > This happens because E.me uses the renderedIcon provided by homescreen which > is 64x64 and includes shadow: > https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/everything. > me/js/etmmanager.js#L182 > > Cristian, is it possible to get the original (no shadow) icon blob? There's only one blob that will be rendered on the Grid and it includes the shadow (renderedIcon in the descriptor) https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/js/page.js#L325
Flags: needinfo?(crdlc)
Assignee | ||
Comment 3•10 years ago
|
||
I know it is currently not implemented, but can we add it? Otherwise it will be impossible to manipulate the icons properly for the collection icons. The main concern is memory usage right? are there any alternatives you can think of?
Comment 4•10 years ago
|
||
Vivien detected in Oslo that each icon costs ~50-60 Kbs in memory. I think that this icon could be generated on fly when the collection icon is created by means of IconRetriever which calculates the original blob without shadows given an icon https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/js/icon_retriever.js
Flags: needinfo?(21)
Comment 5•10 years ago
|
||
I think we are getting to get rid of the shadow in future versions of the homescreen ?
Flags: needinfo?(21) → needinfo?(padamczyk)
Comment 6•10 years ago
|
||
On the app icon - There will always be a shadow but very subtle. On the e.me icon collection icons - We will likely be getting rid of the shadow for v.2.
Flags: needinfo?(padamczyk)
Assignee | ||
Updated•10 years ago
|
User Story: (updated)
Summary: Collection icon renders different size for app dragged from homescreen or added as webapp → Use un-manipulated app icons for Collection icon
Assignee | ||
Comment 7•10 years ago
|
||
Crisitian, I have encountered an issue with icon_retriever: when trying to retrieve the icon for Music2 with: IconRetriever.get({ icon: Music2Icon, success: success, error: error }); neither the success nor the error callbacks are executed, because of this line: https://github.com/EverythingMe/gaia/blob/master/apps/homescreen/js/icon_retriever.js#L21 (the request is ongoing forever) after |make reset-gaia| you can see this log message: E/GeckoConsole( 7619): Content JS ERROR at app://homescreen.gaiamobile.org/js/icon_retriever.js:47 in retrieve: Got an exception when trying to load icon "app://music2.gaiamobile.org/style/icons/60/Music.png +" falling back to cached icon. Exception is: Is this a real bug or can we ignore it because Music2 is not a "real app"? Thanks.
Attachment #8400620 -
Flags: review?(ran)
Attachment #8400620 -
Flags: feedback?(crdlc)
Assignee | ||
Comment 8•10 years ago
|
||
Comment 9•10 years ago
|
||
(In reply to Amir Nissim (Everything.me) from comment #7) > Created attachment 8400620 [details] [review] > Github Pull Request > > Crisitian, I have encountered an issue with icon_retriever: > > when trying to retrieve the icon for Music2 with: > IconRetriever.get({ > icon: Music2Icon, > success: success, > error: error > }); > > neither the success nor the error callbacks are executed, because of this > line: > https://github.com/EverythingMe/gaia/blob/master/apps/homescreen/js/ > icon_retriever.js#L21 > > (the request is ongoing forever) > > after |make reset-gaia| you can see this log message: > E/GeckoConsole( 7619): Content JS ERROR at > app://homescreen.gaiamobile.org/js/icon_retriever.js:47 in retrieve: Got an > exception when trying to load icon > "app://music2.gaiamobile.org/style/icons/60/Music.png +" falling back to > cached icon. Exception is: > > Is this a real bug or can we ignore it because Music2 is not a "real app"? > > Thanks. Honestly, this is a bug because the error callback should be called although it is not a blocker because it fails when the path does not exist thought. Music2 is a real app with wrong manifest, "Music.png does not exist" https://github.com/mozilla-b2g/gaia/tree/master/test_apps/music2/style/icons
Comment 10•10 years ago
|
||
Comment on attachment 8400620 [details] [review] Github Pull Request LGTM, thanks a lot
Attachment #8400620 -
Flags: feedback?(crdlc) → feedback+
Assignee | ||
Comment 11•10 years ago
|
||
Thanks Cristian. Should I open a bug regarding comment 7?
Comment 12•10 years ago
|
||
yes, you should open a bug if you want, no problem :)
Assignee | ||
Comment 13•10 years ago
|
||
(In reply to Cristian Rodriguez (:crdlc) from comment #12) > yes, you should open a bug if you want, no problem :) created bug 991674
Comment 14•10 years ago
|
||
I don't know if it's a problem with my device. I get "TypeError: 'log' called on an object that does not implement interface Console." {file: "app://homescreen.gaiamobile.org/everything.me/js/everything.me.js" line: 559}]" Also, got a JavaScript Error: "A promise chain failed to handle a rejection." On Sunday I'll try on Amir's device and see what's what.
Assignee | ||
Comment 15•10 years ago
|
||
(In reply to Ran Ben Aharon (Everything.me) from comment #14) > I don't know if it's a problem with my device. I get "TypeError: 'log' > called on an object that does not implement interface Console." {file: > "app://homescreen.gaiamobile.org/everything.me/js/everything.me.js" line: > 559}]" > > Also, got a JavaScript Error: "A promise chain failed to handle a rejection." > > On Sunday I'll try on Amir's device and see what's what. Happens to me as well after updating gecko. Looks like bug 989619 since we are binding console.log: https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/everything.me/js/everything.me.js#L589 https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/everything.me/js/everything.me.js#L594
Updated•10 years ago
|
Attachment #8400620 -
Flags: review?(ran) → review+
Assignee | ||
Comment 16•10 years ago
|
||
Travis fails on a jslint error: ----- FILE : /home/travis/build/mozilla-b2g/gaia/apps/homescreen/everything.me/modules/Collection/Collection.js ----- Line 884, E:0002: Missing space before "(" I believe this error is invalid since jslint is not familiar with the Promise API syntax: Promise.then(handleSuccess).catch(handleError) and expects a space after the "catch" keyword. is it ok to land? if not how should I proceed? Line containing the lint error: https://github.com/EverythingMe/gaia/blob/989983-original-icons/apps/homescreen/everything.me/modules/Collection/Collection.js#L884 Travis log: https://travis-ci.org/mozilla-b2g/gaia/jobs/22100668
Flags: needinfo?
Updated•10 years ago
|
Flags: needinfo?(james)
Updated•10 years ago
|
Flags: needinfo?(james) → needinfo?(jlal)
Assignee | ||
Comment 17•10 years ago
|
||
This is a gjslint issue, also reported here: https://code.google.com/p/chromium/issues/detail?id=338301
Assignee | ||
Comment 18•10 years ago
|
||
Gregor, can you provide more information re comment 16?
Flags: needinfo? → needinfo?(anygregor)
Comment 19•10 years ago
|
||
We are trying to move away from gjslint towards jshint. The correct thing to do here would be to remove Collection.js from our jshint xfail.list [1], and then fix all the jshint errors in that code. Doing so would mean gjshint would not run on that file in master. (It would be even nicer to do this for all the files in your patch :D) 1.) https://github.com/mozilla-b2g/gaia/blob/master/build/jshint/xfail.list#L263
Flags: needinfo?(jlal)
Flags: needinfo?(anygregor)
Assignee | ||
Comment 20•10 years ago
|
||
Thanks mhenretty :) created blocker bugs 993364, 993368 for jshinting the relevant files
Assignee | ||
Comment 21•10 years ago
|
||
landed: https://github.com/mozilla-b2g/gaia/commit/d64a687
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•