Closed Bug 1494891 Opened Last year Closed Last year
Mime type specific icons aren't displayed anymore for calendar attachments
Mime type specific icons aren't displayed anymore for calendar attachments in summary and event dialog. E.g. BEGIN:VCALENDAR PRODID:-//Mozilla.org/NONSGML Mozilla Calendar V1.1//EN VERSION:2.0 BEGIN:VEVENT CREATED:20180912T090539Z LAST-MODIFIED:20180928T075319Z DTSTAMP:20180928T075319Z UID:5b47fa17-f2fe-4d96-8cc2-19ce5be98238 SUMMARY:Wrong resp. missing attachment icon ATTACH;FMTTYPE=application/msword:http://example.com/templates/agenda.doc END:VEVENT END:VCALENDAR displayed an icon associated to application/msword in TB52 in the summary dialog and no icon in TB60. In the event dialog, in both versions the genric FF html icon with a black background color is displayed (which seem wrong, too). When attaching documents to an email, the mime type specific icons are displayed. Have the icon resources been moved?
I have no MS Word. I see on TB 52 and 60 the default browser icon.
You mean in the summary dialog (make the calendar you have added the item to read-only to open in with that dialog)? That is not specific to MS Word - you can exchange the ATTACH property in the example also with ATTACH;FMTTYPE=application/pdf:http://example.com/templates/agenda.pdf or ATTACH;FMTTYPE=application/vnd.oasis.opendocument.text:http://example.com /templates/agenda.odt with the same result.
Ah yes. Seems to be affected by bug 1403231. Unfortunately I don't know how to fix this.
This is just an issue, if this is not a file uri.
Assignee: nobody → makemyday
Status: NEW → ASSIGNED
This patch takes care. If the file was added using a cloud file provider, the displayed icon is still the one of the cloud provider instead of the file type - maybe we should change that to display both side by side, but that would be for another bug.
Comment on attachment 9013999 [details] [diff] [review] MissingOrWrongAttachmentIcons-V1.diff Review of attachment 9013999 [details] [diff] [review]: ----------------------------------------------------------------- Does it make sense to DRY this code? Seems pretty much copy/paste. I think this was on purpose back then, so it remains clear it is a remote cloudfile file. I'm fine with changing this though. We should keep in mind that a wrong icon could mislead the user into opening a wrong attachment, do you think the auto detection is safe enough?
Attachment #9013999 - Flags: review?(philipp)
Attachment #9013999 - Flags: review+
Attachment #9013999 - Flags: approval-calendar-esr?(philipp)
Attachment #9013999 - Flags: approval-calendar-beta?(philipp)
Yes, but we should do that for the entire code for displaying attachments in the summary dialog and the event dialog. This is more then I would like to do in this bug - I have filed bug 1500013 for this.
Pushed by email@example.com: https://hg.mozilla.org/comm-central/rev/1be95c892c35 Fix missing or wrong attachment icons in summary and event dialog. r=philipp
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → 6.6
Comment on attachment 9013999 [details] [diff] [review] MissingOrWrongAttachmentIcons-V1.diff Sorry, this patch doesn't apply to c-esr60 due to the list/richlist changes. It's not a simple rebase, so I can't do it.
What is exactly rejected when applying the patch? The ESR60 code looks quite the same to regard of this patch: https://dxr.mozilla.org/comm-esr60/source/calendar/base/content/dialogs/calendar-summary-dialog.js#193 https://dxr.mozilla.org/comm-esr60/source/calendar/lightning/content/lightning-item-iframe.js#2163
$ hg qpush applying 1be95c892c35 patching file calendar/lightning/content/lightning-item-iframe.js Hunk #1 FAILED at 2279 1 out of 1 hunks FAILED -- saving rejects to file calendar/lightning/content/lightning-item-iframe.js.rej patch failed, unable to continue (try -v) patch failed, rejects left in working directory errors during apply, please fix and qrefresh 1be95c892c35 Note the changes in lightning-item-iframe.js: ESR: listItem.setAttribute("image", "moz-icon://" + attachment.uri); Your patch: - image.setAttribute("src", "moz-icon://" + attachment.uri); + image.setAttribute("src", "moz-icon://" + attachment.uri.spec); The bigger change 15 lines down is worse.
Ok, that should be straight forward for backporting. What is >image.setAttribute("src" in cc now just needs to be >listItem.setAttribute("image", in esr60. The attached patch is an edited version of the cc patch since I don't have a local esr60 tree, but this should apply.
I missed one conversion.
Attachment #9018865 - Attachment is obsolete: true
Yes, V2 applies when V1 didn't. Generally, please get a copy of ESR60: hg clone https://hg.mozilla.org/releases/comm-esr60. We've discussed this before. Also, once you have that, it would be responsible to do a try push (same technique as for C-C) to obtain a build and verify that it is working before shipping it to 25 million people. I let Philipp decide whether he wants to approve this.
Comment on attachment 9018866 [details] [diff] [review] MissingOrWrongAttachmentIcons-esr60-V2.diff Feel free to ask Philipp again, but you should also do it, then. I cannot trigger a try push, since I don't have (and likely will not have) an esr60 on this system. If you prefer to have one, please trigger it, since you already have everything applied.
Attachment #9018866 - Flags: approval-calendar-esr?(philipp)
I really don't understand what the problem is: hg clone https://hg.mozilla.org/releases/comm-esr60 That takes ~2-3 minutes. Anyway, I can push this to try if you let me know which platform you want. From memory you're using Windows, right?
This runs flawlessly in the build from the try push. Since Philipp approved that once and there is no issue with the patch, we can carry forward the approval.
Comment on attachment 9018866 [details] [diff] [review] MissingOrWrongAttachmentIcons-esr60-V2.diff OK. However, we're still waiting for approval in bug 1496086 and bug 1476736 :-( - As per my post in the drivers list, I would like to wrap this up real soon now.
Attachment #9018866 - Flags: approval-calendar-esr?(philipp) → approval-calendar-esr+
Comment on attachment 9013999 [details] [diff] [review] MissingOrWrongAttachmentIcons-V1.diff Too late for 63/6.5 beta now.
TB 60.3, Cal 6.2.3: https://hg.mozilla.org/releases/comm-esr60/rev/09102328e68d7f16c4fa53d4825c5a31cdc20a90
Target Milestone: 6.6 → 6.2.3
You need to log in before you can comment on or make changes to this bug.