Closed Bug 659788 Opened 10 years ago Closed 10 years ago

Add icons to Windows jumplist items


(Thunderbird :: OS Integration, defect)

Windows 7
Not set


(Not tracked)

Thunderbird 8.0


(Reporter: rain1, Assigned: rain1)



(3 files, 1 obsolete file)

Attached patch patch v1 (obsolete) — Splinter Review
Currently our Windows jumplist items use the default Thunderbird icon. We can do better. My proposal is to extract the icon from the toolbar sprite sheet and embed it into the EXE at build time.

The attached patch does precisely that. It does depend on pypng [1] though, and we're going to need to get it on at least the Windows builders to get the patch to work. We seem to have at least three options to do so:
1. Get Mozilla-Build to include it. It's pure python, so not a hassle at all to include.
2. Add it to the comm-central repo. It's MIT-licensed and build-only, so I don't really see an issue there.
3. Get people on Windows to easy_install it. easy_install pypng.

Mark, Gozer, opinions?

We could also use a more heavyweight library like the Python Imaging Library [2], but I don't really see the point in doing so.

Attachment #535214 - Attachment is patch: true
Attachment #535214 - Flags: ui-review?(bwinton)
Attachment #535214 - Flags: review?(mbanner)
Attached image Screenshot
Assignee: nobody → sid.bugzilla
Attachment #535228 - Attachment is patch: false
Attachment #535228 - Attachment mime type: text/plain → image/png
Comment on attachment 535214 [details] [diff] [review]
patch v1

Slick looking!  I haven't tested the UI, but assuming it works the way it looks like it works, ui-r=me!

Attachment #535214 - Flags: ui-review?(bwinton) → ui-review+
I filed a similar bug 661502 for Firefox. I think they might be interested to have this functionality too.
Ted, see comment 0, would lib pypng be something you would be willing to include in mozilla-build?
Sure, but we'd have to get out a new MozillaBuild release, which I haven't had time to do.
Now that we're about to include MozMill in the tree, I think it'd make sense to include this in the tree too, and maybe even combine the virtualenvs for the two.
(In reply to comment #7)
> Now that we're about to include MozMill in the tree, I think it'd make sense
> to include this in the tree too, and maybe even combine the virtualenvs for
> the two.

Assuming the code isn't massive, I think that could be reasonable to do.
Comment on attachment 535214 [details] [diff] [review]
patch v1

Just on file placement, I'd like to see the python script in c-c/build/ instead, so that SeaMonkey could in theory use this same approach.
Attachment #535214 - Flags: feedback-
Attached patch patch v2Splinter Review
All right, this moves pypng into the tree. It doesn't use a virtualenv though, just straightforward

The library's MIT-licensed and used only at build time, so adding it shouldn't be a problem I think. Do we need to update about:license for this?
Attachment #535214 - Attachment is obsolete: true
Attachment #535214 - Flags: review?(mbanner)
Attachment #550226 - Flags: review?(mbanner)
Comment on attachment 550226 [details] [diff] [review]
patch v2

>diff --git a/mailnews/base/src/nsMessengerWinIntegration.cpp b/mailnews/base/src/nsMessengerWinIntegration.cpp

>-#define IDI_MAILBIFF 32767
>+#define IDI_MAILBIFF 32576

If you are changing this here, you have to change it in the suite manifest as well.
Attachment #550226 - Flags: feedback-
Good point, thanks.
Comment on attachment 550226 [details] [diff] [review]
patch v2

This seems fine to me.

I think bringing the code in should be fine, but please check with licensing about the incorporation - more details here:
Comment on attachment 550226 [details] [diff] [review]
patch v2

(r+ assuming licensing is happy).
Attachment #550226 - Flags: review?(mbanner) → review+
Yes, this is fine.

Thanks Gerv!
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 8.0
You need to log in before you can comment on or make changes to this bug.