Closed Bug 505297 Opened 12 years ago Closed 12 years ago

OS/2 Thunderbird doesn't change main icon with official-branding

Categories

(Thunderbird :: Build Config, defect)

x86
OS/2
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b4

People

(Reporter: wuno, Assigned: wuno)

Details

(Whiteboard: )

Attachments

(1 file, 1 obsolete file)

User-Agent:       Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.9.1.1) Gecko/20090716 Firefox/3.5.1
Build Identifier: 

Tried to build a thunderbird-3beta3 on OS/2 with official-branding enabled. However, the application icon remained the same as without official-branding (not the big bird covering the envelope, but a little bird in front of an opened envelope). The reason appears to be that the "unbranded icon" is hard-coded in the resource file splashos2.rc in the mail/app directory. It seems that TB slipped somehow through, because the rc file for all other Mozilla applications include the right icons depending on the selection of official-branding.

Reproducible: Always
Version: unspecified → Trunk
Component: General → Build Config
QA Contact: general → build-config
this patch renames mozos2.ico to thunderbird-os2.ico and moves the file to mail/branding/nightly, adjustment of the defines to the resource editor and the Makefile in mail/branding/nightly
And, well, the icons used by thunderbird would need some love, too. Stefan, you are most experienced with OS/2 artwork. If you agree and have some spare time to overhaul 4-5 icons, I'd open a follow-up bug.
Attachment #389529 - Flags: review?(mozilla)
Comment on attachment 389529 [details] [diff] [review]
similar to other OS/2 mozilla-apps

>diff --git a/mail/app/Makefile.in b/mail/app/Makefile.in
...
>+RCFLAGS += -DTHUNDERBIRD_ICO=\"$(DIST)/branding/thunderbird.ico\"

This is already defined in the DEFINES at the top of the file. Does OS/2 not include the DEFINES when generating its rc?
(In reply to comment #1)
> And, well, the icons used by thunderbird would need some love, too. Stefan, you
> are most experienced with OS/2 artwork. If you agree and have some spare time
> to overhaul 4-5 icons, I'd open a follow-up bug.

Maybe this would be a bit early. The nightly icons can look ugly. Let's redo them once real branding for TB3 has chosen.

(In reply to comment #2)
> >+RCFLAGS += -DTHUNDERBIRD_ICO=\"$(DIST)/branding/thunderbird.ico\"
> 
> This is already defined in the DEFINES at the top of the file. Does OS/2 not
> include the DEFINES when generating its rc?

That's correct. Our resource compiler is pretty broken, and IIRC there were some weird format errors in the past when including all DEFINES. So we have to duplicate the defines that we need in RCFLAGS.

Walter, can you remind me: why do we need the OS/2 icon copied to app.ico? We do that for all Mozilla apps but it doesn't make sense to me any more. ;-)
Assignee: nobody → wuno
Status: NEW → ASSIGNED
Hardware: Other → x86
(In reply to comment #3)
> (In reply to comment #2)
> > >+RCFLAGS += -DTHUNDERBIRD_ICO=\"$(DIST)/branding/thunderbird.ico\"
> > 
> > This is already defined in the DEFINES at the top of the file. Does OS/2 not
> > include the DEFINES when generating its rc?
> 
> That's correct. Our resource compiler is pretty broken, and IIRC there were
> some weird format errors in the past when including all DEFINES. So we have to
> duplicate the defines that we need in RCFLAGS.

Can we therefore just a brief comment to that effect? Otherwise I can see someone erroneously removing that later.
(In reply to comment #3)
> (In reply to comment #1)
> > And, well, the icons used by thunderbird would need some love, too. Stefan, you
> > are most experienced with OS/2 artwork. If you agree and have some spare time
> > to overhaul 4-5 icons, I'd open a follow-up bug.
> 
> Maybe this would be a bit early. The nightly icons can look ugly. Let's redo
> them once real branding for TB3 has chosen.
ok, it seems, we've missed the new ones for TB2 already, but linux icons for unofficial builds were changed only end of May this year.

> Walter, can you remind me: why do we need the OS/2 icon copied to app.ico? We
> do that for all Mozilla apps but it doesn't make sense to me any more. ;-)
I've looked in mxr and for all mozilla app we do copy the icon to app.ico. I could imagine that it was like with document.ico that was just taken because it exists for windows. app.ico is used by some windows embedders. So, probably we won't need it, I could try the other day to remove that line from the makefile to see what happens. If it's not needed we should get rid of it for all apps in another bug.
Comment on attachment 389529 [details] [diff] [review]
similar to other OS/2 mozilla-apps

OK, let's do it like this then.
Attachment #389529 - Flags: review?(mozilla) → review+
(In reply to comment #4)

> Can we therefore just a brief comment to that effect? Otherwise I can see
> someone erroneously removing that later.

Nobody who is not building on OS/2 is supposed to fiddle around in OS/2-only parts of Makefiles. And over the last few years nobody has changed that part. So I don't really see the point.
OK, so we don't need the app.ico line. It was removed from browser in bug 504953. Could you please make a new patch without it?
Whiteboard:
sure, app.ico is used also by xulrunner and sunbird, shall I remove these references, too?
(In reply to comment #9)
> sure, app.ico is used also by xulrunner and sunbird, shall I remove these
> references, too?

Yes please, but can you do those in separate bugs?
I also removed the app.ico reference from other-licenses/branding/thunderbird/Makefile.in to be consistent between official and unofficial builds.
Attachment #389529 - Attachment is obsolete: true
Attachment #391195 - Flags: review?(mozilla)
Comment on attachment 391195 [details] [diff] [review]
fix removing app.ico references as well

Yes, looks good to me, thanks.
Attachment #391195 - Flags: review?(mozilla) → review+
Like all the other OS/2 bugs I wanted to push this, but every time I want to there is so much red on the tinderboxes that I don't dare to push this patch (even though it could be considered to be NPOTB)...
Keywords: checkin-needed
Sorry about the red, we're having problems with nightly builds at the moment. Additionally tests have been unusually orange/red today :-(

Anyway checked in: http://hg.mozilla.org/comm-central/rev/1d1f41ede485
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.0b4
You need to log in before you can comment on or make changes to this bug.