307103 - The Firefox Updater needs an icon

Status: VERIFIED FIXED

(Toolkit :: Application Update, enhancement)

Description

[u49640]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X; de-de) AppleWebKit/412.7 (KHTML, like Gecko) Safari/412.5
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b4) Gecko/20050904 Firefox/1.0+

i've just played with the Updater on my Mac and the App did use the default Icon for Apps.

It should use some kind of custom icon that shows its a FireFox Updater (maybe the FireFox Icon with an 
Arrow or something like this

Reproducible: Always

Comment 1

[Jo Hermans]

ref bug 307026

Comment 2

[u49640]

i've just noticed that the Windows Updater needs an icon to
--> All/All (dont know if the Other Versions show the icon too)

Comment 3

[Josh Aas]

Attachment: icon proposal v1.0

This is from Jasper Hauser.

Comment 4

[Josh Aas]

I think adding an icon to the updater app would make us look a little more professional on all platforms. The updater icon wouldn't have to change based on whether we're building with official branding or not, since it isn't tied to a brand. I like Jasper's proposal.

Comment 5

[Marcia Knous [:marcia]]

I think this is a good idea, especially for the Mac - anything we can do to improve the experience for users would be good, and Josh indicates on the mac side that this won't break anything. Nominating for consideration, at least for the Mac.

Comment 6

[Josh Aas]

Attachment: Mac OS X icns file of proposal v1.0

Comment 7

[Darin Fisher]

How about we do this cross-platform?

Comment 8

[Josh Aas]

I don't know how to do this for anything other than Mac. But it is the biggest problem on Mac, so maybe we should just do that if you don't know how to do it for Win and Linux.

Comment 9

[Josh Aas]

Attachment: Mac fix v1.0

This patch, with the Mac OS X icns file named "updater.icns" in the macbuild resources dir works well.

Comment 10

[Asa Dotzler [:asa]]

Kevin, can you sign off on this icon? we'll get the product team to review this asap.

Comment 11

[Kevin Gerich]

The 16x16 version could use some tweaking/sharpening to bring out the shapes of the arrows, but overall it looks good. Thanks Jasper!

Comment 12

[Josh Aas]

Kevin or Jasper, can you take care of the 16x16 version if you want to and post a new icns file ASAP?

Comment 13

[Jasper]

Attachment: new icns file with sharper 16px version

-

Comment 14

[Darin Fisher]

Can someone please attach a .ico version of this image so that I can prepare a  patch for the Windows updater?

Comment 15

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Windows version of attachment 200670

I'm no icon expert, but I think that this should do.

Comment 16

[Darin Fisher]

Attachment: Windows patch

This patch makes the icon appear down in the task bar and in the Alt+Tab dialog.

Comment 17

[Darin Fisher]

I bet we could probably implement a solution for Linux too, but I just haven't had a chance to look into that yet.  The updater UI for Linux is definitely lagging behind in UI appeal ;-)

Comment 18

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 200693 [details] [diff] [review]
Windows patch

>Index: progressui_win.cpp
>+  // Set dialog icon
>+  HICON hIcon = LoadIcon(GetModuleHandle(NULL), MAKEINTRESOURCE(IDI_DIALOG));
>+  if (!hIcon)
>+    return;
>+  SendMessage(hDlg, WM_SETICON, ICON_BIG, (LPARAM) hIcon);
>+


Why not...

if (hIcon)
  SendMessage(hDlg, WM_SETICON, ICON_BIG, (LPARAM) hIcon);

instead of returning early, because otherwise this change has the potential to change the behavior of this function in certain circumstances. I know it'll probably never happen, just seems unnecessary.

Comment 19

[Ben Goodger (use ben at mozilla dot org for email)]

r=ben with that change. 

Comment 20

[Darin Fisher]

You got it.  Thanks for the quick review, Ben!

Comment 21

[Darin Fisher]

Attachment: Windows patch (with revision) [fixed-on-trunk, fixed1.8]

Comment 22

[Josh Aas]

Mac icns file and Info.plist patch landed on trunk and branch.

Comment 23

[Mike Schroepfer]

Clearing fixed1.8 flag since windows side has not landed yet.

Comment 24

[Christian :Biesinger (don't email me, ping me on IRC)]

(why only set ICON_BIG on windows, not ICON_SMALL?)

as for gtk, maybe:
http://developer.gnome.org/doc/API/2.0/gtk/GtkWindow.html#gtk-window-set-icon-from-file
if the gtk 2.2 dependency is OK; otherwise, could use:
http://developer.gnome.org/doc/API/2.0/gtk/GtkWindow.html#gtk-window-set-icon
with calling the gdkpixbuf loading stuff yourself (http://developer.gnome.org/doc/API/2.0/gdk-pixbuf/gdk-pixbuf-file-loading.html)

If that was a request for me to implement it, I probably won't have time for that...

Comment 25

[Darin Fisher]

> (why only set ICON_BIG on windows, not ICON_SMALL?)

because that's sufficient.  windows renders the small icon using the value set for the big icon.  that's the behavior of WinXP at least.


> If that was a request for me to implement it, I probably won't have time for
> that...

It is low on my priority list, and yeah... was groking for help ;-)

Comment 26

[Darin Fisher]

Comment on attachment 200695 [details] [diff] [review]
Windows patch (with revision) [fixed-on-trunk, fixed1.8]

The windows change is fixed-on-trunk and fixed1.8

Comment 27

[Christian :Biesinger (don't email me, ping me on IRC)]

>  windows renders the small icon using the value set for the big icon.

that's not the documented behaviour of WM_SETICON...http://msdn.microsoft.com/library/default.asp?url=/library/en-us/winui/winui/windowsuserinterface/windowing/windows/windowreference/windowmessages/wm_seticon.asp

Comment 28

[Darin Fisher]

> >  windows renders the small icon using the value set for the big icon.
> 
> that's not the documented behaviour of

true, and so i tested this on windows98, and indeed the icon does not show up in the task bar.  patch coming up.

Comment 29

[Chase Phillips]

This patch looks to have made pacifica red:

Creating Resource file: module.res
/cygdrive/c/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/build/cygwin-wrapper rc.exe  -I/cygdrive/c/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/toolkit/mozapps/update/src/updater -r -D_IMPL_NS_GFX -D_IMPL_NS_MSG_BASE -D_IMPL_NS_WIDGET  -DOSTYPE=\"WINNT5.2\" -DOSARCH=\"WINNT\" -DBUILD_ID=2005102512 -I/cygdrive/c/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/toolkit/xre -I../../../../../dist/include/libmar -I../../../../../dist/include/libbz2 -I../../../../../dist/include -I../../../../../dist/include -I../../../../../dist/include/nspr    -I../../../../../dist/sdk/include -Fomodule.res /cygdrive/c/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/toolkit/mozapps/update/src/updater/module.rc
c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/toolkit/mozapps/update/src/updater/module.rc (81): error RC2176 : old DIB in updater.ico; pass it through SDKPAINT

Comment 30

[:Gavin Sharp [email: [e-mail]]]

Attachment: slightly tweaked Windows icon

You could try this icon to fix the bustage. I passed it through IconShop (Open, Export), an old Icon editing app, which resulted in a different header output. As I understand it, the "old DIB in updater.ico" error happens when icons use the newer ico format. Worth a shot, I guess.

Comment 31

[Darin Fisher]

The solution was to do "cvs admin -kb updater.ico"

Comment 32

[Marcia Knous [:marcia]]

Icon looks good using Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8) Gecko/20051025 Firefox/1.5.

Comment 33

[Josh Aas]

Leaving this open for a Linux fix.

Comment 34

[Nightrider]

It would be nice if some update feature such as the "Update Notifier" add-on could be implemented with the notification icons about status and new updates in the toolbar or somewhere else.

Comment 35

[Robert Strong (they/them - no direct email)]

The patch in bug 473337 fixes this so adding dependency.

Comment 36

[Mike Beltzner [:beltzner, not reading bugmail]]

Fixed by bug 473337, closing this out.

Comment 37

[Tony Chung [:tchung]]

Verified fix on and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090408 Shiretoko/3.5b4pre
and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090408 Minefield/3.6a1pre 
and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090408 Shiretoko/3.5b4pre