Update linux theme with new icons

Note related bug 484064 for updating the Thunderbird app icon for Linux.

In email Kalle Pearson writes: >If you ask me, the best way would be to ship 16,22,32,48,128 and 256 >sizes of the icon "firefox" in the .png format and install them >in /usr/share/icons/hicolor/$size/apps/ >That would make it scale nicely.

Monreal adds: >We also need 24x24 and mventnor comments: >Don't use XPM (although everyone said this already :) ), it's heavily deprecated >in favour of PNG. >I'd imagine it's just a case of adding those new resolutions that Kalle >suggested then changing a couple of Makefiles. The XPMs really should be deleted >if at all possible.

We might remove the xpm files in bug 495250 along with some other legacy files that are no longer needed.

Just to clarify: does removing the xpm files change our backwards compatibility with older desktop environments at all? Or is packaging both xpm and png files simply redundant?

We don't work with GTK+ < 2.10, so we shouldn't worry about XPM if backwards compatibility is the only concern.

Anyone know what the deal is with this file: http://mxr.mozilla.org/mozilla-central/source/other-licenses/branding/firefox/document.png It's using the pre-firefox 1 branding, and an XP style page, but since it's not .icns or .ico, it seems like it might have at some point been (or is currently) related to linux.

So I screwed up and didn't realize that we needed 22x22 and 24x24, both of these are going to be a Bicubic Sharper reduction of the pixel polished 32x32. If we do an RC2 we can potentially get new files at these resolutions with manual pixel hinting.

(In reply to comment #7) > Anyone know what the deal is with this file: > > http://mxr.mozilla.org/mozilla-central/source/other-licenses/branding/firefox/document.png I think it's unused. MIME icons in Linux are provided by the toolkit and have no app branding. (In reply to comment #8) > So I screwed up and didn't realize that we needed 22x22 and 24x24, both of > these are going to be a Bicubic Sharper reduction of the pixel polished 32x32. > If we do an RC2 we can potentially get new files at these resolutions with > manual pixel hinting. That should be fine, although I don't think an icon at that size would have a shadow underneath it if the shadow is in the 32x32 version, as that is what seems to happen with most icons on my desktop.

>That should be fine, although I don't think an icon at that size would have a >shadow underneath it if the shadow is in the 32x32 version, as that is what >seems to happen with most icons on my desktop. We also have all of the standard resolutions without a shadow. What resolutions on linux should be shadow free? (22, 24 and 16?)

(In reply to comment #10) > We also have all of the standard resolutions without a shadow. What > resolutions on linux should be shadow free? (22, 24 and 16?) Yeah, that seems right after looking at /usr/share/icons.

The (now outdated) tango styleguide does not really talk much about shadows but normally we have slight shadows on the 22/24px variant, just not on the 16px one. Note that 22/24px is commonly created from the same source artwork, the difference being a 1-pixel border added to the 24px variant (for historical reason e.g. KDE3 which used 22px for toolbars).

(In reply to comment #12) > The (now outdated) tango styleguide does not really talk much about shadows but > normally we have slight shadows on the 22/24px variant, just not on the 16px > one. Does this apply for app icons? In my entire Applications menu I only notice about 3 or 4 icons that have any sort of noticeable shadow.

I'll attach 22 and 24 so that you can see what they look like with and without a shadow.

(In reply to comment #13) > Does this apply for app icons? In my entire Applications menu I only notice > about 3 or 4 icons that have any sort of noticeable shadow. I don't think we have a strict policy. 16px has no shadow, 32px and up have a shadow. Jakub, Andreas: what about 22/24? The icons most similar to the firefox icon (internet category and generic web browser icon) both have a slight shadow in GNOME's default theme btw but I don't have any strong feelings either way.

(In reply to comment #18) > The icons most similar to the firefox icon (internet category and generic web > browser icon) both have a slight shadow in GNOME's default theme btw but I > don't have any strong feelings either way. Ah yes, I do see that. Looking at the uploaded icons, they both look nice, now I can't decide. :) Although I think the shadow is a bit too dark compared with the shadow of Epiphany's and the other icons? The existing Firefox icon at this size doesn't have a shadow.

This zip file contains the app icon at 16, 22, 24, 32, 48, 128 and 256

Great... I was skeptical about the icon refresh but those icons really look great in action. Even together with Tango :) I was going to vote for the 22px-with-shadow variant, but after seeing it in the menus there's something odd about it... the shadow seems to add to the tail, making it look thicker at the bottom and taking away some roundness. Maybe making the shadow slightly larger (to the sides) would help but right now I also like the no-shadow version.

Ah, also you need to add the suffixes to nsWindow::SetIcon (and remove the mentions of XPM hopefully) extensions[6][7] should become extensions[7][9] = {".png", "16.png", "22.png", "24.png", "32.png", "48.png", "128.png", "256.png"} and the code underneath it that break;s should be deleted (since even then it mentions XPM is deprecated!!!)

Er, maybe I have the numbers in extensions[][] mixed up... Never mind, I'm sure ddahl knows what to do :)

Actually, I am not sure what to do, I cannot get it to build if I add the new icon suffixes: NS_IMETHODIMP nsWindow::SetIcon(const nsAString& aIconSpec) { if (!mShell) return NS_OK; nsCOMPtr<nsILocalFile> iconFile; nsCAutoString path; nsTArray<nsCString> iconList; // Look for icons with the following suffixes appended to the base name. // The last two entries (for the old XPM format) will be ignored unless // no icons are found using the other suffixes. XPM icons are depricated. const char extensions[6][9] = { ".png", "16.png", "32.png", "48.png", "128.png", "256.png", ".xpm", "16.xpm" }; for (PRUint32 i = 0; i < NS_ARRAY_LENGTH(extensions); i++) { // Don't bother looking for XPM versions if we found a PNG. if (i == NS_ARRAY_LENGTH(extensions) - 2 && iconList.Length()) break; nsAutoString extension; extension.AppendASCII(extensions[i]); ResolveIconName(aIconSpec, extension, getter_AddRefs(iconFile)); if (iconFile) { iconFile->GetNativePath(path); iconList.AppendElement(path); } } // leave the default icon intact if no matching icons were found if (iconList.Length() == 0) return NS_OK; return SetWindowIconList(iconList); } i've tried "extensions[6][7]" after removing the 2 .xpms and adding the 2 new pngs. Also leaving in the xpms and adding 2 pngs, like this: "extensions[6][9]" It seems like the makefile cannot find the icons.

Just adding my work in progress patch, in case someone can tell what is wrong with nsWindow::SetIcon.

This works; it's just that C does two-dimensional arrays in a confusing way. And, seriously, kill XPM. :)

I am going to run a tryserver build momentarily for viewing in action

Is there a reason you're not including the 22x22 and 24x24? If this is what the final SetIcon will look like then you should change it to extensions[6][8].

And the rule seems to be extensions[x][y] where x is the number of strings in the array and y is the length of the longest string in the array + 1 (for the NUL).

Comment on attachment 382070 [details] [diff] [review] v 0.1 Linux Icons for linux browser/app/ and browser/branding/unofficial/ aren't expected to contain the Firefox icon. Why is this patch touching wizHeader.bmp & Co.?

I really did not know which icons to include - so I added all of them. Let me know which to include, and I will remove the additional icons.

Ok, got rid of the extras images. What was i thinking?:)

this patch is also addressing comment 28

Comment on attachment 382077 [details] [diff] [review] v0.3 added 22x22 and 24x24 to nsWindow.cpp Again, I don't see why you're patching files in browser/app/ and browser/branding/unofficial/. See http://mxr.mozilla.org/mozilla-central/source/browser/app/default32.png and http://mxr.mozilla.org/mozilla-central/source/browser/branding/unofficial/default32.png for instance -- no Firefox icon there. Shouldn't the 22 and 24 icons also be added in browser/app/Makefile.in?

As well as the 128 one. And the XPMs should be removed since they won't be used anymore. The Firefox icons only seem to be under other-licenses/branding/firefox/.

> I don't think we have a strict policy. 16px has no shadow, 32px and up have a > shadow. Jakub, Andreas: what about 22/24? Unless it poses a problem (such as in action icons 'add' and 'remove') we do have a shadow for the 22x22/24x24px size.

reverted icon changes in browser/app

very strange - I think we need to add a Minefield icon to browser/app/default256.png for this to build properly. cp: cannot stat `/home/ddahl/code/moz/mozilla-central/mozilla/browser/app/default256.png': No such file or directory make[2]: *** [export] Error 1 make[2]: Leaving directory `/home/ddahl/code/moz/mozilla-central/obj-i686-pc-linux-gnu-optimize/browser/app' make[1]: *** [export] Error 2 make[1]: Leaving directory `/home/ddahl/code/moz/mozilla-central/obj-i686-pc-linux-gnu-optimize/browser' make: *** [default] Error 2 make: Leaving directory `/home/ddahl/code/moz/mozilla-central/obj-i686-pc-linux-gnu-optimize/browser'

accoring to this push by mconnor, I am doing it all wrong: http://hg.mozilla.org/mozilla-central/rev/abe1b8b03969

(In reply to comment #38) > very strange - I think we need to add a Minefield icon to > browser/app/default256.png for this to build properly. Right, if we add sizes, we need unbranded icons in those sizes. http://hg.mozilla.org/mozilla-central/rev/abe1b8b03969 and http://hg.mozilla.org/mozilla-central/rev/be17081e57cf are only the first steps.

Comment on attachment 382169 [details] [diff] [review] v 0.4 removed changes to browser/app This replaces browser/branding/unofficial/content/icon16.png, for instance, but it shouldn't. It should replace other-licenses/branding/firefox/default16.png instead, except that mconnor did that already.

(In reply to comment #42) > [...] browser/branding/unofficial/content/icon16.png > [...] other-licenses/branding/firefox/default16.png err, working examples: browser/branding/unofficial/content/icon48.png vs. other-licenses/branding/firefox/content/icon48.png browser/branding/unofficial/default16.png vs. other-licenses/branding/firefox/default16.png

waiting on new icons for minefield / branch 22/24/256.

If the 22 and 24 icons are too much trouble, just get rid of them. GTK will downscale the 32 icon which is what these icons are likely to be anyway. And whenever you change the items in extensions[][] make sure you change the numbers as well (comment 29).

Uh, but since the 22 and 24 icons are checked in I suppose we have to use them... Never mind me...

No worries. The WIP patch hnadles most of this stuff now. I have been in conversation with mconnor about it all.

Trunk patch - added placeholders for browser/app and browser/branding/unofficial

ran pngcrush on each png, some of the old pngs were corrupt. weird. Borris made new ones

Just putting up a WIP. Getting build errors related thebes right now: http://pastebin.mozilla.org/656930

Comment on attachment 382536 [details] [diff] [review] v 0.1.2 Trunk Patch > ICON_FILES = \ > $(DIST)/branding/mozicon128.png \ >- $(DIST)/branding/mozicon50.xpm \ >- $(DIST)/branding/mozicon16.xpm \ >+ $(DIST)/branding/default256.png \ > $(DIST)/branding/document.png \ > $(NULL) Why is default256.png part of that group, rather than together with default16.png, default22.png etc.? >- const char extensions[6][7] = { ".png", "16.png", "32.png", "48.png", >- ".xpm", "16.xpm" }; >+ >+ const char extensions[9][8] = { ".png", "16.png", "32.png", "48.png", >+ "32.png", "48.png", "128.png", >+ "256.png" }; The numbers are messed up (22, 24, 32, 48), and unless I'm missing something, extensions[9][8] should be extensions[8][8].

Comment on attachment 382779 [details] [diff] [review] v 0.1 Branch / 191 patch >+ const char extensions[9][8] = { ".png", "16.png", "22.png", "24.png", >+ "32.png", "48.png", "256.png", > ".xpm", "16.xpm" }; 128.png isn't missing intentionally, is it?

This bug has morphed to adding support for just the additional resolutions, and Linux will now pick up the new icon at the current resolutions, right?

in response to comment 52: Seems like 128.png was never in the extensions array. Weird. I'll fix it now. in response to comment 53: It should - are you seeing something else in your testing?

Comment on attachment 382965 [details] [diff] [review] v 0.2 Trunk Patch > ifneq (,$(filter gtk2,$(MOZ_WIDGET_TOOLKIT))) > cp $(srcdir)/mozicon128.png $(DIST)/branding/mozicon128.png >- cp $(srcdir)/mozicon16.xpm $(DIST)/branding/mozicon16.xpm >- cp $(srcdir)/mozicon50.xpm $(DIST)/branding/mozicon50.xpm >+ cp $(srcdir)/default256.png $(DIST)/branding/default256.png > cp $(srcdir)/document.png $(DIST)/branding/document.png > endif > ifeq ($(MOZ_WIDGET_TOOLKIT),gtk2) > cp $(srcdir)/default16.png $(DIST)/branding/default16.png >+ cp $(srcdir)/default22.png $(DIST)/branding/default22.png >+ cp $(srcdir)/default24.png $(DIST)/branding/default24.png > cp $(srcdir)/default32.png $(DIST)/branding/default32.png > cp $(srcdir)/default48.png $(DIST)/branding/default48.png default256.png should come right after default48.png. > BROWSER_APP_FILES = \ > default16.png \ >+ default22.png \ >+ default24.png \ > default32.png \ > default48.png \ > mozicon128.png \ >- mozicon16.xpm \ >- mozicon50.xpm \ >+ default256.png \ > firefox.ico \ > document.ico \ > $(NULL) here too

>in response to comment 53: It should - are you seeing something else in your >testing? Just making absolutely sure, if we didn't get that part in before freeze I would have villagers coming after me with torches and pitchforks.

Alex: Actually, mconnor is doing some checking with distro contacts before we proceed, as this is more involved than I realized.

Distros with custom branding will have to add 22/24/264px versions of their icon or undo part of this patch... Am I missing something else?

Unless I'm on crack, if there's no match, the code handles that fine (if you dig way way down deep).

You'll get browser/app/default22.png if the branded default22.png is missing, won't you? By the way, it seems like browser/branding/unofficial/Makefile.in also needs an update...

This image was posted to my blog, not sure what is causing the title bar icon to be cropped: http://i43.tinypic.com/2aj9lp0.jpg

re: comment 63 - I assume that is a linux/theme issue. I have a similar looking Gnome theme and the icon looks normal.

I can't reproduce that either.

Follow up bug to remove some icons previously used on linux: bug 499226

Is there more left to do here for 1.9.1 or is this bug fixed?

I think this is fixed. I believe mconnor checked in the correct icons - the only followup that might make sense is removing any xpm linux icons and associated code that is deprecated - which should be a separate bug.

Please re-nom for status1.9.1 if this turns out not to be fixed.

I think this current bug is superseeded by bug 1399691. I see the 57 logo was generated in 22*22, 24*24, and 256*256px[2]. If I'm missing something, feel free to reopen. [1] https://hg.mozilla.org/mozilla-central/file/39f4a41bb708/browser/branding/official/