Closed Bug 955425 Opened 7 years ago Closed 7 years ago

Port |Bug 659788 - Add icons for Windows 7 jumplist taskbar items.|

Categories

(Instantbird :: Other, defect)

x86
Windows 7
defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: clokep, Assigned: clokep)

Details

Attachments

(1 file, 1 obsolete file)

*** Original post on bio 1989 at 2013-06-01 14:21:00 UTC ***

*** Due to BzAPI limitations, the initial description is in comment 1 ***
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1989 as attmnt 2472 at 2013-06-01 14:21:00 UTC ***

In bio 659788, they changed c-c to auto-generate ico files instead of directly including them in the build, we should port this and not duplicate icons.

> The patch does the following at build time:
> - extract the icons from the sprite sheet
> - add an icon header to each png to convert it into an ico
> - embed the icos in the EXE
>
> To do this it adds the MIT-licensed pypng library to the tree.

Note that we don't actually use sprite sheets for our icons, but I thought it made more sense to use the c-c png2ico than to try to mess with it to remove this functionality.
Attachment #8354239 - Flags: review?(florian)
Comment on attachment 8354239 [details] [diff] [review]
Patch

*** Original change on bio 1989 attmnt 2472 at 2013-06-01 18:24:27 UTC ***

I assume all the files in build/ are a straight copy from comm-central, and I haven't reviewed them. Let me know if this assumption is wrong.

>diff --git a/instantbird/app/Makefile.in b/instantbird/app/Makefile.in

>+# Extract the icons we care about embedding into the EXE
>+themedir = $(topsrcdir)/chat/themes

I think the code would be more readable if this was inlined, rather than a variable.

>+# Each icon is 18x18 in the toolbar, and we want a 16x16 icon here, so we cut
>+# off a pixel at each end.

Remove this comment?

>+libs::

I think you really want http://hg.mozilla.org/comm-central/rev/eb6026dff0b8, otherwise building on Windows may randomly fail.

>+	$(call png2ico,$(themedir)/available-16.png,0,0,16,available-16.ico)
>+	$(call png2ico,$(themedir)/away-16.png,0,0,16,away-16.ico)
>+	$(call png2ico,$(themedir)/offline-16.png,0,0,16,offline-16.ico)
>+
>+GARBAGE += write-message.ico address-book.ico

Please update this.

Otherwise, seems like a good improvement; I'm glad we can get rid of ico files in our tree :-).
Attachment #8354239 - Flags: review?(florian) → review-
Attached patch Patch v2Splinter Review
*** Original post on bio 1989 as attmnt 2473 at 2013-06-02 11:50:00 UTC ***

(In reply to comment #1)
> I assume all the files in build/ are a straight copy from comm-central, and I
> haven't reviewed them. Let me know if this assumption is wrong.
Yes, they're all direct copies.

> >+# Extract the icons we care about embedding into the EXE
> >+themedir = $(topsrcdir)/chat/themes
> I think the code would be more readable if this was inlined, rather than a
> variable.
I agree, I was just following the format in the Thunderbird patch. :)

> I think you really want http://hg.mozilla.org/comm-central/rev/eb6026dff0b8,
> otherwise building on Windows may randomly fail.
Good catch. I ported this changeset.
Attachment #8354240 - Flags: review?(florian)
Comment on attachment 8354239 [details] [diff] [review]
Patch

*** Original change on bio 1989 attmnt 2472 at 2013-06-02 11:50:03 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354239 - Attachment is obsolete: true
Comment on attachment 8354240 [details] [diff] [review]
Patch v2

*** Original change on bio 1989 attmnt 2473 at 2013-06-03 10:18:46 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354240 - Flags: review?(florian) → review+
*** Original post on bio 1989 at 2013-06-04 00:40:43 UTC ***

http://hg.instantbird.org/instantbird/rev/2b2069c68a86
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.5
You need to log in before you can comment on or make changes to this bug.