Open Bug 429725 Opened 12 years ago Updated 1 year ago

Icons in the Applications Prefpane

Categories

(Firefox :: Theme, defect, P3)

defect

Tracking

()

ASSIGNED

People

(Reporter: faaborg, Assigned: kliu)

References

Details

(Keywords: polish, Whiteboard: [has patch][needs review gavin][polish-easy][polish-visual][polish-p3][icon-namoroka][icon-complete])

Attachments

(3 files, 7 obsolete files)

Requesting blocking to pick up wanted, does anyone have cycles to work on this?
Flags: blocking-firefox3?
Adding some other potential suspects for helping us find an opportunistic fix, but agree that this is wanted-not-blocking.
Flags: wanted-firefox3+
Flags: blocking-firefox3?
Flags: blocking-firefox3-
I can take this if nobody else will; it's a very simple patch.  There are, however, a few questions I'd like to ask first...

1) Why is the generic file icon named "Bookmarks-folder.png"?  It's not a folder...

2) Are we doing this for Windows only?  If we wish to extend this to Mac/Linux, then the appropriate icons will need to land (e.g., AFAIK, there's no calendar icon for the Mac)

3) What's the ETA for the mail icon?
Assignee: nobody → kliu.bugzilla.3c9f
Attached patch Windows-only patch (obsolete) — Splinter Review
This is a Windows-only patch.  All that is needed here is for Alex to provide mail.png and mail-aero.png.

Not requesting review yet because I don't know if this is supposed to be Windows-only or not.  Set the review? flag if you decide that this is Windows-only.  Otherwise, let me know so that I could extend this to Mac or Linux (provided the icons, of course :)).
>1) Why is the generic file icon named "Bookmarks-folder.png"?  It's not a
>folder...

We often name icons based on how the icon was used as opposed to what it is a picture of (like "history" instead of "clock").  This means that when the same image is used in a different context we either need to duplicate the icon and change the name, or use an unusually named icon in the new context.  You can duplicate the default favicon in the preferences directory and name it genericContent.png if you think it would be nicer for people to be able to theme this differently.

>2) Are we doing this for Windows only?  If we wish to extend this to Mac/Linux,
>then the appropriate icons will need to land (e.g., AFAIK, there's no calendar
>icon for the Mac)

We should do this on all platforms, although I think linux is pretty much set right now.  I'll review the other platforms right now to see what we need.

>3) What's the ETA for the mail icon?

I'll attach them
Attached image mail.png (obsolete) —
Attached image mail-aero.png (obsolete) —
OS: Windows XP → All
Summary: Windows icons in the Applications Prefpane → Icons in the Applications Prefpane
On OS X everything looks good except that a lot of content types are missing a generic icon, and we need to use icons for mailto and webcal.  I'm assuming the application content item is not applicable since apps are distributed through dmg's. 
On linux everything looks good except for mailto and webcal needing icons.  cc'ing the linux themers.
mail-aero.png would probably work ok on OS X (either as a placeholder or final icon) if Stephen doesn't have an icon ready.  The proto calendar icon is here https://bugzilla.mozilla.org/attachment.cgi?id=317753
Okay.  What would you like for me to use for the pinstripe generic icon?
(In reply to comment #9)
> On linux everything looks good except for mailto and webcal needing icons. 

The mail icon is in bug 418868, attachment 304998 [details]. For webcal, the "calendar" icon has already landed. http://mxr.mozilla.org/seamonkey/source/browser/themes/gnomestripe/browser/places/calendar.png

Is that all you need?
>What would you like for me to use for the pinstripe generic icon?

where does the the default favicon live in pinstripe?  I couldn't find it, but we should either use or duplicate that for generic content.
(In reply to comment #13)
> >What would you like for me to use for the pinstripe generic icon?
> 
> where does the the default favicon live in pinstripe?  I couldn't find it, but
> we should either use or duplicate that for generic content.
> 

http://mxr.mozilla.org/seamonkey/source/toolkit/themes/pinstripe/mozapps/places/defaultFavicon.png
Attached patch patch v2 for all platforms (obsolete) — Splinter Review
I should note that the generic icon will only be used for handlers without an extension or a MIME type (for example, webcal and mailto, if they weren't given special icons of their own, would fall into this category).  In your screenshot, the Windows Media handlers had generic icons, and those won't be replaced by this patch replaced (FWIW, you should not be seeing those generic icons for Windows Media handlers unless your system's file type handlers are misconfigured).  Getting rid of generic icons for handlers that do have extensions of MIME types would be a more invasive process and probably not something that we should try this late.
Attachment #317767 - Attachment is obsolete: true
Attachment #317808 - Flags: review?(gavin.sharp)
Attached file icons to land (obsolete) —
Attachment #317769 - Attachment is obsolete: true
Attachment #317770 - Attachment is obsolete: true
Attached image Proto Mailto Icon (obsolete) —
Would it be possible to include this icon for OS X? Or should I do that separately?
> Would it be possible to include this icon for OS X? Or should I do that
> separately?
> 

All that matters to me is that the icons are in place when the new applications.js goes in, and I don't care whether that's by you landing them before this bug lands or by the icons landing as a part of this bug.  So it's up to you how you want to do this.  :)  And if the icons are landed as a part of this bug, I'll change the mail icon to what you provide, since the current icon is just something that I stuck there so that there is an icon.
If you wouldn't mind to land it with this bug that would be most appreciated :) Thank you!
*checks bugmail*  *notices that Alex is landing the Windows mailto icons in bug 430759*

Hey, Alex, since you're landing the Windows mailto icons in the upcoming icon drop, could you also land the Mac icons for this bug too (see zip file I attached and the mail icon that Stephen attached)?  Then I could set 430759 to block this bug and consolidate the icon landings.
I don't have cvs access myself, the mail icons are landing over in the windows bug since I use a script to generate all of the files and perform some testing and additional compression.
Attached file icons to land, updated
Icons to land, either as a part of the 430759 landing or as a part of this bug's landing.  (removed the winstripe icons since that's in 430759 & added Stephen's icon)
Attachment #317809 - Attachment is obsolete: true
Attachment #317814 - Attachment is obsolete: true
Attached patch patch v2.1 (unbitrot) (obsolete) — Splinter Review
Minor update to the patch, now that the dust has cleared from bug 430759.
Attachment #317808 - Attachment is obsolete: true
Attachment #318077 - Flags: review?(gavin.sharp)
Attachment #317808 - Flags: review?(gavin.sharp)
Status: NEW → ASSIGNED
Whiteboard: [has patch][needs review gavin]
Comment on attachment 318077 [details] [diff] [review]
patch v2.1 (unbitrot)

>Index: browser/components/preferences/applications.js

>+#if defined(XP_WIN) || defined(XP_MACOSX)

Does the XUL preprocessor support this syntax? It doesn't appear to, from a quick skim of preprocessor.py.

Otherwise this looks fine.
Attachment #318077 - Flags: review?(gavin.sharp) → review-
(In reply to comment #24)
> Does the XUL preprocessor support this syntax?
> 

From skimming Preprocessor.py and Expression.py, you're right, it doesn't support that (or any sort of binary operators except for equality).  I don't think that I want to use #ifndef XP_UNIX because that does not necessarily mean Windows or Mac, so I guess the only option that I have would be something like:

#ifdef XP_WIN
#define SPECIFY_GENERIC_ICON_FOR_APP_HANDLERS
#elifdef XP_MACOSX
#define SPECIFY_GENERIC_ICON_FOR_APP_HANDLERS
#endif

#ifdef SPECIFY_GENERIC_ICON_FOR_APP_HANDLERS
// code
#endif

It seems mildly hackish; is there a better way?
Attached patch patch v2.2Splinter Review
Attachment #318077 - Attachment is obsolete: true
Attachment #318808 - Flags: review?(gavin.sharp)
Duplicate of this bug: 399999
Flags: wanted1.9.0.x?
Flags: wanted-firefox3.1?
Flags: wanted1.9.0.x?
Flags: wanted-firefox3.1? → wanted-firefox3.1+
Priority: -- → P2
Target Milestone: --- → Firefox 3.1
This was a tracking bug for Firefox 3 development, resolving.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
(In reply to comment #28)
> This was a tracking bug
>
Huh?

> resolving.
> 
Shouldn't be resolved as FIXED as it's not.  Did you want to WONTFIX?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
>Shouldn't be resolved as FIXED as it's not.  Did you want to WONTFIX?

While cleaning up about 150 bugs I messed up on this one, I thought this was a bug about the icons at the top of the preferences window.  We still need to get this fixed, hopefully for 3.1
Status: REOPENED → NEW
Status: NEW → ASSIGNED
this bug is eligible for bug 462081
Keywords: polish
Whiteboard: [has patch][needs review gavin] → [has patch][needs review gavin][polish-easy][polish-visual]
Gavin - any chance on getting a review on this any time soon?
Why the hardcoded references to images in /skin/ such as:
chrome://browser/skin/preferences/application.png
chrome://browser/skin/places/calendar.png
chrome://browser/skin/preferences/mail.png
chrome://browser/skin/preferences/generic.png
(In reply to comment #33)
> Why the hardcoded references to images in /skin/

Probably because it was simpler to implement that way for them. I have reworked this in the SeaMonkey version to not hardcode them, that work could be backported to the Firefox implementation.
if you decide to merge the images and use -moz-image-region let me know and I will give you the merged file (need to do it on my side since I'm using scripts to generate all the image files, so it is easier for us to update them later).
Target Milestone: Firefox 3.1 → Firefox 3
Target Milestone: Firefox 3 → ---
Comment on attachment 318808 [details] [diff] [review]
patch v2.2

This patch no longer applies cleanly, unfortunately. The approach from comment 34/comment 35 might be worth investigating if someone wants to bring this patch back up to date.
Attachment #318808 - Flags: review?(gavin.sharp)
This bug's priority relative to the set of other polish bugs is:
P3 - Polish issue that is in a secondary interface, occasionally encountered, or is not easily identifiable.

It would be good to get this right, but it's a secondary interface and people aren't currently noticing that the icons are wrong.
Whiteboard: [has patch][needs review gavin][polish-easy][polish-visual] → [has patch][needs review gavin][polish-easy][polish-visual][polish-p3]
Whiteboard: [has patch][needs review gavin][polish-easy][polish-visual][polish-p3] → [has patch][needs review gavin][polish-easy][polish-visual][polish-p3][icon-namoroka][icon-complete]
Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information
Priority: P2 → P3
You need to log in before you can comment on or make changes to this bug.