1832771 - Fix icons for Calendar dialogues, add missing msgcomposeWindow icons for Linux, remove unused AB icons

Status: RESOLVED FIXED

(Thunderbird :: General, enhancement)

Description

[BB]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/113.0.0.0 Safari/537.36 Edg/113.0.1774.35

Steps to reproduce:

The Mozilla platform allows setting an icon based on the icon= property of the window/dialogue. This is correctly configured for the main window, the compose window and the calendar alarm, as can be seen here:
https://searchfox.org/comm-central/search?q=icon%3D&path=.xhtml&case=false&regexp=false

All other calendar windows (and there are 19 more in calendar/base/content/dialogs) don't have an icon set. As a consequence, TB uses the standard application icon in the windows title bar and the Windows/Linux taskbar.

Some dialogues use this code
https://searchfox.org/comm-central/search?q=setDialogId&path=&case=false&regexp=false
which doesn't appear to have any effect, despite this comment:
"id is changed during execution to allow different Window-icons on this dialog"
https://searchfox.org/comm-central/rev/05041974dba9e6737285568976d468ce26334dc8/calendar/base/content/dialogs/calendar-summary-dialog.xhtml#29

We suggest to remove the non-working code and place an icon="calendar-general-dialog" property on all remaining 19 calendar dialogues.

See https://github.com/Betterbird/thunderbird-patches/blob/main/115/features/20-feature-configurable-icons.patch which also provides more code to achieve full icon configurability.

Comment 1

[BB]

Attachment: 1832771-fix-calendar-icons-OS-integration.patch

Changes in summary:

• All calendar dialogs got icon="calendar-general-dialog" apart from the alarm dialog which was already correct.
• Incorrect comment "id is changed during execution to allow different Window-icons on this dialog" removed.
• Removed abcardWindow and addressbookWindow from Makefile and packaging, they are no longer needed since the AB is in a tab now.
• Added calendar-general-dialog to Makefile and packaging.
• Provided calendar-general-dialog.ico for Windows
• Provided calendar-alarm-dialog.png and calendar-general-dialog.png for Linux. The alarm PNG was taken from a Linux distribution. They seem to add it.

setDialogId() can be removed since it sets dialog IDs and CSS relies on that, see:
https://searchfox.org/comm-central/rev/05041974dba9e6737285568976d468ce26334dc8/calendar/base/content/item-editing/calendar-item-panel.js#302

Comment 2

[BB]

Attachment: 1832771-fix-calendar-icons-OS-integration.patch

Make patch apply.

Comment 3

[Richard Marti (:Paenglab)]

Comment on attachment 9333345 [details] [diff] [review]
1832771-fix-calendar-icons-OS-integration.patch

Review of attachment 9333345 [details] [diff] [review]:
-----------------------------------------------------------------

r+
Thanks for this changes!

::: mail/installer/package-manifest.in
@@ -204,5 @@
>  @RESPATH@/chrome/pdfjs.manifest
>  @RESPATH@/chrome/pdfjs/*
>  #ifndef XP_UNIX
> -@RESPATH@/chrome/icons/default/abcardWindow.ico
> -@RESPATH@/chrome/icons/default/addressbookWindow.ico

Please, could you remove this icons from tree?

Comment 4

[BB]

Attachment: 1832771-fix-calendar-icons-OS-integration-v2.patch

Sure can.

Comment 5

[BB]

We did a bit more research into the issue. Setting up the icons came from bug 1531836 which landed in Mozilla 67. When creating a new calendar entry in TB 68, the dialogue and the windows taskbar indeed had special icons. This function likely stopped working during de-XUL, since one of the patches in that bug was on xpfe/appshell/nsXULWindow.cpp (https://hg.mozilla.org/mozilla-central/rev/d8cdae952942) which no longer exists. We'll attach a second patch in a minute for some further clean-up.

Comment 6

[BB]

Attachment: 1832771-remove-non-functional-code.patch

Removes the code added here:
https://hg.mozilla.org/comm-central/rev/0ec9e8d998fc#l2.22

We noticed that the icons that were meant to be displayed were already removed in TB 91
https://hg.mozilla.org/comm-central/rev/274a17af2deb
and no one noticed that the calendar dialogs had dropped back to the standard application icon.

Comment 7

[Richard Marti (:Paenglab)]

Comment on attachment 9333377 [details] [diff] [review]
1832771-remove-non-functional-code.patch

Looks good. Thanks!

Comment 8

[BB]

Attachment: 1832771-fix-calendar-icons-OS-integration-v3.patch

We've built this on Linux now and noticed that the icons also needed to be added to mail/branding/branding-common.mozbuild, otherwise they don't end up in chrome/defaults/icons on the target system.

With all this in place, creating a new calendar event now displays the new general icon on our Linux distro which has a "taskbar".

Further detail: The alarm icon is actually from here:
https://searchfox.org/comm-esr68/source/calendar/base/themes/common/icons/calendar-alarm-dialog.png
before it got removed here:
https://hg.mozilla.org/comm-central/rev/274a17af2deb

Comment 9

[Alessandro Castellani [:aleca]]

Comment on attachment 9333402 [details] [diff] [review]
1832771-fix-calendar-icons-OS-integration-v3.patch

Please, don't self review your patches

Comment 10

[BB]

Comment on attachment 9333402 [details] [diff] [review]
1832771-fix-calendar-icons-OS-integration-v3.patch

Looking at this further, the calendar icons should not go into branding but rather mail/app/icons/ like SM has it:
https://searchfox.org/comm-central/source/suite/app/icons/windows

Also, a Linux icon for msgcomposeWindow is missing, but that can be done separately.

Comment 11

[BB]

Attachment: 1832771-fix-calendar-icons-OS-integration-v4.patch

Changes to the patch that received r+:

• removed abcardWindow.ico and addressbookWindow.ico as requested
• removed addressbook.ico which was also unused
• moved calendar PNGs from branding to mail/app/icons/gtk
• adjusted Makefile.in accordingly
• removed undefined (hence empty) DESKTOP_ICONS_SMALL

We'll submit a patch for to add some PNGs form msgcomposeWindow in another bug since this bug here is in Calendar.

Note that SM have a bunch of these:
https://searchfox.org/comm-central/rev/b71ddc1a8fc766bdf17b6ba7e98b2d010fafb0ea/suite/installer/package-manifest.in#452-454

Comment 12

[BB]

Attachment: 1832771-fix-calendar-icons-OS-integration-v5.patch

Make it apply.

Comment 13

[BB]

Attachment: 1832771-add-msgcomposeWindow-icons.patch

There was little work to add this. The PNGs were taken from the Windows icon file. Built, packaged and tested on Linux.

Comment 14

[Richard Marti (:Paenglab)]

Comment on attachment 9333465 [details] [diff] [review]
1832771-fix-calendar-icons-OS-integration-v5.patch

Thanks!

Comment 15

[Richard Marti (:Paenglab)]

Comment on attachment 9333477 [details] [diff] [review]
1832771-add-msgcomposeWindow-icons.patch

Thanks!

Maybe we should look if we need to change to our newest icons.

Comment 16

[BB]

There is room for improvement. Linux distros can typically handle SVGs to get very crisp icons even at large sizes. Sadly this Mozilla platform code doesn't support it:
https://searchfox.org/mozilla-central/rev/58914ffca60bca87ecbbc71928a05295f9bf9a27/widget/gtk/nsWindow.cpp#3498-3507

Based on bug 1531836 comment #0, this is not used in FF, so someone from the TB team would have to improve it.

Order for landing: 1832771-fix-calendar-icons-OS-integration-v5.patch needs to go before 1832771-add-msgcomposeWindow-icons.patch, the third patch can go in any order.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a06e8c9bf9cc
Fix calendar icons OS integration (and related clean-up). r=Paenglab
https://hg.mozilla.org/comm-central/rev/de521e0ddcde
Add msgcomposeWindow*.png icons for Linux. r=Paenglab
https://hg.mozilla.org/comm-central/rev/4afd838d6921
remove non-functional icon-setting code and adjust comments. r=Paenglab

Comment 18

[BB]

This seems to have caused some Windows build bustage. Likely due to the changes in this file:
https://hg.mozilla.org/comm-central/rev/a06e8c9bf9ccb10ab56a57e362fef8a7305caddc#l20.2
We'll have a look.

Comment 19

[BB]

We've got a fix.

Comment 20

[BB]

Attachment: 1832771-follow-up.patch

Sorry, this icon was removed in the last minute. The error actually is:
45:13.52 llvm-rc: Error in ICON statement (ID 32578):
45:13.52 no such file or directory

Comment 21

[Richard Marti (:Paenglab)]

Comment on attachment 9333519 [details] [diff] [review]
1832771-follow-up.patch

Did no clobber before testing, so haven't spotted this issue.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/81fcadefc32b
Follow-up: Remove deleted ADDRESSBOOK_ICO from splash.rc. r=Paenglab

Comment 23

[BB]

Comment on attachment 9333519 [details] [diff] [review]
1832771-follow-up.patch

Sorry about the bustage. Yes, a clobber would have found it, but that takes 45+ minutes on Windows (not using the most modern hardware).