915923 - Show default app icon when none found

Status: RESOLVED FIXED

(DevTools Graveyard :: WebIDE, defect, P3)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Right now we're just leaving an empty square where the icon goes if there is no icon or it can't be loaded.  We should have a default.

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Note that once you install an app, it gets the default "rocket" icon on device.  Perhaps we should show the same in the App Manager?

Comment 2

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Hey, I'd like to take this.

What about using the "rocket.svg"? I've made the patch.

Comment 3

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Attachment: Added rocket.svg as default app icon

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8356607 [details] [diff] [review]
Added rocket.svg as default app icon

Review of attachment 8356607 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for looking into this!

I've added a few comments below.  Once you've resolved them, attach an updated patch and r? me again.

Also, in your next patch, make sure the commit message is in the right format[1].

[1]: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

::: browser/devtools/app-manager/content/projects.js
@@ +131,5 @@
>          icon = manifest.icons[size];
>        }
>      }
>      if (!icon)
> +      return "chrome://browser/skin/devtools/app-manager/rocket.svg"; 

This image is similar to, but not quite the same as, the default icon shown on the device.  You can find that here[2] in the Gaia project.

I would suggest copying that file into the same folder as this "rocket.svg" file, and probably naming it something like "default-app-icon.png" for clarity.

Also, this line has a trailing space at the end, so be sure to remove that next time around.

[2]: https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/style/images/[e-mail]

Comment 5

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Thanks for your guidance!

You were right, but just copying the png file to browser/themes/shared/devtools/app-manager wasn't enough, the build ignored the file, I had to edit the "jar.mn" files and add the file to list, so it's included in build.

Comment 6

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Attachment: Added default icon for apps with no icon

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8357267 [details] [diff] [review]
Added default icon for apps with no icon

Review of attachment 8357267 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great, good work! :)

r+ assuming you address my comment below.  Once you've uploaded a revised patch, you can mark that one r+ yourself.  Usually we'd run this on our try server to see if there were test failures, but there aren't tests for this sort of change.  So, after all that, add "checkin-needed" to this bug's "keywords" field, and someone will check in the patch for you.

Thanks again!

::: browser/themes/windows/jar.mn
@@ +294,5 @@
>          skin/classic/browser/devtools/app-manager/add.svg                   (../shared/devtools/app-manager/images/add.svg)
>          skin/classic/browser/devtools/app-manager/index-icons.svg           (../shared/devtools/app-manager/images/index-icons.svg)
>          skin/classic/browser/devtools/app-manager/rocket.svg                (../shared/devtools/app-manager/images/rocket.svg)
>          skin/classic/browser/devtools/app-manager/noise.png                 (../shared/devtools/app-manager/images/noise.png)
> +        skin/classic/browser/devtools/app-manager/default-app-icon.png      (../shared/devtools/app-manager/images/default-app-icon.png)

The Windows jar.mn actually has two sections to update, this one for "classic" and another for "aero", so you'll need to add this to both places.  Search for other app-manager images, and you'll see what I mean.

Comment 8

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Attachment: Added default icon for apps with no icon - fixed Windows jar.mn

Thanks for mentoring me on my first bug!

I learned a lot, hopefully I can contribute on more bugs.

Comment 9

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Attachment: Added default icon for apps with no icon - fixed Windows jar.mn

Forgot that empty line

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/846a2b559c35

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/846a2b559c35