Closed Bug 1724071 Opened 3 years ago Closed 3 years ago

Account setup doesn't use provider display name for calendars

Categories

(Thunderbird :: Mail Window Front End, defect)

Thunderbird 91
Unspecified
All
defect

Tracking

(thunderbird_esr91+ fixed, thunderbird91- wontfix, thunderbird92? fixed)

RESOLVED FIXED
93 Branch
Tracking Status
thunderbird_esr91 + fixed
thunderbird91 - wontfix
thunderbird92 ? fixed

People

(Reporter: Fallen, Assigned: aleca)

References

Details

Attachments

(2 files)

Attached image image.png (deleted) —

When detecting calendars, the blue label seems to be using the "type" attribute from the provider. This might be a technical name and not a good fit for the dialog.

The Provider interfaces have a displayName we could use instead, for this purpose I'd probably go a step further and have a new shortName property. The base class should default this to displayName if not provided.

https://searchfox.org/comm-central/rev/6c2670e4a542f81b254966921f23f1d62904ea13/calendar/base/public/calICalendarProvider.idl#30

Would be great to track this for 91 so we don't expose the technical names for add-ons using autodetection.

good catch

OS: Unspecified → All
Version: unspecified → Thunderbird 91
Blocks: tb91found

I think the technical names (CalDAV/ICS) aren't a problem, but information some users may need. These are also for symmetry with the mail setup which shows e.g. IMAP/POP3/SMTP. But of course for add-ons we shouldn't display the EXT- something. I guess showing the add-on name should work?

This is for the gdata add-on?

Yes, in my case it is the new version of the gdata add-on using the WebExtensions API experiment. We can certainly use the technical name for caldav ics, we just need to set the displayName accordingly or introduce a shortName that is just "CalDAV" or "ICS". The type attribute was never intended to be user-facing.

For the record, here are the displayNames used currently: https://searchfox.org/comm-central/source/calendar/locales/en-US/chrome/calendar/calendar.properties#343

Assignee: nobody → alessandro

This is the code that prints the calendar provider type: https://searchfox.org/comm-central/rev/6820aaa407ab48a2c1b7a7477015e0dac1dc5daf/mail/components/accountcreation/content/accountSetup.js#2437-2440

we just need to set the displayName accordingly or introduce a shortName that is just "CalDAV" or "ICS"

I'm not too familiar with the Calendar code, could you guide me a little bit on how to properly update this?

Flags: needinfo?(philipp)

Also pinging Lasana as he's more experienced with the calendar.
Any advice on how to handle this based on what Philipp suggested?

Flags: needinfo?(lasana)

You will probably have to modify the return value of this function, specifically here:
https://searchfox.org/comm-central/source/calendar/base/modules/utils/calProviderDetectionUtils.jsm#137

to return what you want for the provider type. It might be more future proof to start returning the whole provider in case we decide we need to make additional decisions around it. Personally, I think we should use the displayName or change them if they are too awkward. Having a displayName and a shortName seems confusing.

Flags: needinfo?(lasana)

All right, I have a patch for this/
I'm returning the entire provider from https://searchfox.org/comm-central/rev/93487d295e3d3365c3082104ea980c7a0dfceeef/calendar/base/modules/utils/calProviderDetectionUtils.jsm#137 instead of just the type.

The onDetectionSuccess() and fillProviders() might need some proper optimization since we're basically creating maps of keys to then fetch again the provider objects, no great.
I didn't touch them for now as I wanted to be sure this approach is correct before tweaking the code too much.

Uploading the patch on Phabricator.

Flags: needinfo?(philipp)
Attachment #9236915 - Attachment description: Bug 1724071 - Use displayName for the calendar provider in the Account Setup. r=mkmelin → Bug 1724071 - Use displayName for the calendar provider in the Account Setup. r=darktrojan
Status: NEW → ASSIGNED
Target Milestone: --- → 93 Branch

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/442974aa7827
Use shortName for the calendar provider in the Account Setup. r=darktrojan

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED

Comment on attachment 9236915 [details]
Bug 1724071 - Use displayName for the calendar provider in the Account Setup. r=darktrojan

[Approval Request Comment]
Regression caused by (bug #): -
User impact if declined: Wrong and unhelpful calendar name printed during account setup
Testing completed (on c-c, etc.): on c-c
Risk to taking this patch (and alternatives if risky): low as this patch introduces a non translatable calendar shortName to use in the UI in order to properly represent the technology of the calendar (CalDAV, ICS, SQLite, etc).

Attachment #9236915 - Flags: approval-comm-beta?

Comment on attachment 9236915 [details]
Bug 1724071 - Use displayName for the calendar provider in the Account Setup. r=darktrojan

[Approval Request Comment]
Regression caused by (bug #): -
User impact if declined: Wrong and unhelpful calendar name printed during account setup
Testing completed (on c-c, etc.): on c-c
Risk to taking this patch (and alternatives if risky): low as this patch introduces a non translatable calendar shortName to use in the UI in order to properly represent the technology of the calendar (CalDAV, ICS, SQLite, etc).

Attachment #9236915 - Flags: approval-comm-esr91?

Comment on attachment 9236915 [details]
Bug 1724071 - Use displayName for the calendar provider in the Account Setup. r=darktrojan

[Triage Comment]
Approved for beta

Attachment #9236915 - Flags: approval-comm-beta? → approval-comm-beta+

Comment on attachment 9236915 [details]
Bug 1724071 - Use displayName for the calendar provider in the Account Setup. r=darktrojan

[Triage Comment]
Approved for esr91

Attachment #9236915 - Flags: approval-comm-esr91? → approval-comm-esr91+
Regressions: 1757750
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: