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)

x86
macOS
defect
Not set
normal

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)

Attached image screenshot of the bug
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
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: nobody → amirn
(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)
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?
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)
I think we are getting to get rid of the shadow in future versions of the homescreen ?
Flags: needinfo?(21) → needinfo?(padamczyk)
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)
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
Depends on: 989731
Attached file 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.
Attachment #8400620 - Flags: review?(ran)
Attachment #8400620 - Flags: feedback?(crdlc)
(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 on attachment 8400620 [details] [review]
Github Pull Request

LGTM, thanks a lot
Attachment #8400620 - Flags: feedback?(crdlc) → feedback+
Thanks Cristian. Should I open a bug regarding comment 7?
yes, you should open a bug if you want, no problem :)
(In reply to Cristian Rodriguez (:crdlc) from comment #12)
> yes, you should open a bug if you want, no problem :)

created bug 991674
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.
(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
Attachment #8400620 - Flags: review?(ran) → review+
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?
Flags: needinfo?(james)
Flags: needinfo?(james) → needinfo?(jlal)
This is a gjslint issue, also reported here:
https://code.google.com/p/chromium/issues/detail?id=338301
Gregor, can you provide more information re comment 16?
Flags: needinfo? → needinfo?(anygregor)
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)
Thanks mhenretty :)
created blocker bugs 993364, 993368 for jshinting the relevant files
Depends on: 993368, 993364
landed: https://github.com/mozilla-b2g/gaia/commit/d64a687
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Depends on: 995949
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: