hook up 22/24/256px icons on Linux for gnome and KDE (was: Refresh Linux Firefox icons)

NEW
Unassigned

Status

()

Firefox
General
--
enhancement
8 years ago
3 years ago

People

(Reporter: ddahl, Unassigned)

Tracking

3.5 Branch
All
Linux
Points:
---
Bug Flags:
blocking-firefox3.5 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 11 obsolete attachments)

1.59 KB, image/png
Details
1.61 KB, image/png
Details
1.81 KB, image/png
Details
1.84 KB, image/png
Details
115.01 KB, application/x-zip-compressed
Details
114.70 KB, patch
Details | Diff | Splinter Review
114.02 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

8 years ago
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?

Comment 6

8 years ago
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.

Comment 9

8 years ago
(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?)

Comment 11

8 years ago
(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).

Comment 13

8 years ago
(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.
Created attachment 381918 [details]
22x22 no shadow

I'll attach 22 and 24 so that you can see what they look like with and without a shadow.
Created attachment 381919 [details]
22x22 with shadow
Created attachment 381920 [details]
24x24 no shadow
Created attachment 381921 [details]
24x24 with 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.

Comment 19

8 years ago
(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.
Created attachment 381970 [details]
App icon for linux, all resolutions

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.

Comment 22

8 years ago
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!!!)

Comment 23

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

Comment 24

8 years ago
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.
(Reporter)

Comment 25

8 years ago
Created attachment 382053 [details] [diff] [review]
WIP - trouble with nsWindow::SetIcon

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

Comment 26

8 years ago
Created attachment 382069 [details] [diff] [review]
Proper SetIcon()

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

Comment 27

8 years ago
Created attachment 382070 [details] [diff] [review]
v 0.1 Linux Icons for linux

I am going to run a tryserver build momentarily for viewing in action
Attachment #382070 - Flags: review?(dao)

Comment 28

8 years ago
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].

Comment 29

8 years ago
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.?
Attachment #382070 - Flags: review?(dao) → review-
(Reporter)

Comment 31

8 years ago
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.
(Reporter)

Comment 32

8 years ago
Created attachment 382076 [details] [diff] [review]
v0.2 Linux Icons - removed additional icons not needed in this patch

Ok, got rid of the extras images. What was i thinking?:)
Attachment #382070 - Attachment is obsolete: true
Attachment #382076 - Flags: review?
(Reporter)

Updated

8 years ago
Attachment #382076 - Flags: review? → review?(dao)
(Reporter)

Comment 33

8 years ago
Created attachment 382077 [details] [diff] [review]
v0.3 added 22x22 and 24x24 to nsWindow.cpp

this patch is also addressing comment 28
Attachment #382076 - Attachment is obsolete: true
Attachment #382077 - Flags: review?(dao)
Attachment #382076 - Flags: review?(dao)
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?
Attachment #382077 - Flags: review?(dao) → review-

Comment 35

8 years ago
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/.

Comment 36

8 years ago
> 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.
(Reporter)

Comment 37

8 years ago
Created attachment 382169 [details] [diff] [review]
v 0.4 removed changes to browser/app

reverted icon changes in browser/app
Attachment #382077 - Attachment is obsolete: true
Attachment #382169 - Flags: review?
(Reporter)

Updated

8 years ago
Attachment #382169 - Flags: review? → review?(mconnor)
(Reporter)

Comment 38

8 years ago
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'
(Reporter)

Comment 39

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

Comment 40

8 years ago
Created attachment 382197 [details] [diff] [review]
nsWindow.cpp changes for linux only

Updated

8 years ago
Attachment #382197 - Attachment is patch: true
Attachment #382197 - Attachment mime type: application/octet-stream → text/plain
(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.
Attachment #382169 - Flags: review?(mconnor) → review-
(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
(Reporter)

Comment 44

8 years ago
Created attachment 382224 [details] [diff] [review]
WIP patch - started over

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

Comment 45

8 years ago
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).

Comment 46

8 years ago
Uh, but since the 22 and 24 icons are checked in I suppose we have to use them...

Never mind me...
(Reporter)

Updated

8 years ago
Assignee: nobody → ddahl
(Reporter)

Comment 47

8 years ago
No worries. The WIP patch hnadles most of this stuff now. I have been in conversation with mconnor about it all.
(Reporter)

Comment 48

8 years ago
Created attachment 382514 [details] [diff] [review]
v 0.1.1 Linux Icons - Trunk Patch

Trunk patch - added placeholders for browser/app and browser/branding/unofficial
Attachment #382197 - Attachment is obsolete: true
Attachment #382224 - Attachment is obsolete: true
Attachment #382514 - Flags: review?(mconnor)
(Reporter)

Comment 49

8 years ago
Created attachment 382536 [details] [diff] [review]
v 0.1.2 Trunk Patch

ran pngcrush on each png, some of the old pngs were corrupt. weird. Borris made new ones
Attachment #382053 - Attachment is obsolete: true
Attachment #382069 - Attachment is obsolete: true
Attachment #382169 - Attachment is obsolete: true
Attachment #382514 - Attachment is obsolete: true
Attachment #382536 - Flags: review?(mconnor)
Attachment #382514 - Flags: review?(mconnor)
Flags: wanted1.9.0.x+
Flags: blocking-firefox3.5-
Summary: Refresh Linux Firefox icons → hook up 22/24/256px icons on Linux for gnome and KDE (was: Refresh Linux Firefox icons)
Flags: wanted1.9.0.x+ → wanted1.9.1.x+
(Reporter)

Comment 50

8 years ago
Created attachment 382779 [details] [diff] [review]
v 0.1 Branch / 191 patch

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

Updated

8 years ago
Attachment #382779 - Attachment description: v 0.1 Untested Branch / 191 patch → v 0.1 Branch / 191 patch
Attachment #382779 - Flags: review?(dao)
(Reporter)

Updated

8 years ago
Attachment #382536 - Flags: review?(mconnor) → review?(dao)
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?
(Reporter)

Comment 54

8 years ago
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?
(Reporter)

Comment 55

8 years ago
Created attachment 382965 [details] [diff] [review]
v 0.2 Trunk Patch
Attachment #382536 - Attachment is obsolete: true
Attachment #382965 - Flags: review?(dao)
Attachment #382536 - Flags: review?(dao)
(Reporter)

Comment 56

8 years ago
Created attachment 382966 [details] [diff] [review]
v 0.2 Branch / 191 Patch
Attachment #382779 - Attachment is obsolete: true
Attachment #382966 - Flags: review?(dao)
Attachment #382779 - Flags: review?(dao)
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.
(Reporter)

Comment 59

8 years ago
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
(Reporter)

Comment 64

8 years ago
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

Updated

8 years ago
Attachment #382965 - Flags: review?(dao)

Updated

8 years ago
Attachment #382966 - Flags: review?(dao)
Is there more left to do here for 1.9.1 or is this bug fixed?
status1.9.1: --- → ?
Flags: wanted1.9.1.x+
(Reporter)

Comment 68

8 years ago
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.
status1.9.1: ? → ---
(Reporter)

Updated

4 years ago
Assignee: ddahl → nobody
Severity: normal → enhancement
Hardware: x86 → All
Target Milestone: Firefox 3.5 → ---
You need to log in before you can comment on or make changes to this bug.