Closed Bug 1494891 Opened Last year Closed Last year

Mime type specific icons aren't displayed anymore for calendar attachments

Categories

(Calendar :: Dialogs, defect, minor)

Lightning 6.2
defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: MakeMyDay, Assigned: MakeMyDay)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

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.
Attachment #9013999 - Flags: review?(philipp)
Attachment #9013999 - Flags: approval-calendar-esr?(philipp)
Attachment #9013999 - Flags: approval-calendar-beta?(philipp)
Duplicate of this bug: 1309943
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-esr+
Attachment #9013999 - Flags: approval-calendar-beta?(philipp)
Attachment #9013999 - Flags: approval-calendar-beta+
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.
Keywords: checkin-needed
Pushed by mozilla@jorgk.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
Keywords: checkin-needed
Resolution: --- → FIXED
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.
Attachment #9013999 - Flags: approval-calendar-esr+
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
Flags: needinfo?(jorgk)
$ 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.
Flags: needinfo?(jorgk)
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.
Flags: needinfo?(jorgk)
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?
Flags: needinfo?(jorgk)
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.
Attachment #9013999 - Flags: approval-calendar-beta+
You need to log in before you can comment on or make changes to this bug.