Account setup doesn't use provider display name for calendars
Categories
(Thunderbird :: Mail Window Front End, defect)
Tracking
(thunderbird_esr91+ fixed, thunderbird91- wontfix, thunderbird92? fixed)
People
(Reporter: Fallen, Assigned: aleca)
References
Details
Attachments
(2 files)
(deleted),
image/png
|
Details | |
(deleted),
text/x-phabricator-request
|
wsmwk
:
approval-comm-beta+
wsmwk
:
approval-comm-esr91+
|
Details |
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.
Would be great to track this for 91 so we don't expose the technical names for add-ons using autodetection.
Comment 1•3 years ago
|
||
good catch
Comment 2•3 years ago
|
||
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?
Reporter | ||
Comment 3•3 years ago
|
||
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 | ||
Updated•3 years ago
|
Assignee | ||
Comment 4•3 years ago
|
||
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?
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 5•3 years ago
|
||
Also pinging Lasana as he's more experienced with the calendar.
Any advice on how to handle this based on what Philipp suggested?
Comment 6•3 years ago
|
||
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.
Assignee | ||
Comment 7•3 years ago
|
||
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.
Assignee | ||
Comment 8•3 years ago
|
||
Updated•3 years ago
|
Assignee | ||
Updated•3 years ago
|
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
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 10•3 years ago
|
||
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).
Assignee | ||
Comment 11•3 years ago
|
||
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).
Comment 12•3 years ago
|
||
Comment on attachment 9236915 [details]
Bug 1724071 - Use displayName for the calendar provider in the Account Setup. r=darktrojan
[Triage Comment]
Approved for beta
Comment 13•3 years ago
|
||
bugherder uplift |
Thunderbird 92.0b4:
https://hg.mozilla.org/releases/comm-beta/rev/7d6eb8cbf08b
Comment 14•3 years ago
|
||
Comment on attachment 9236915 [details]
Bug 1724071 - Use displayName for the calendar provider in the Account Setup. r=darktrojan
[Triage Comment]
Approved for esr91
Comment 15•3 years ago
|
||
bugherder uplift |
Thunderbird 91.1.0:
https://hg.mozilla.org/releases/comm-esr91/rev/ea49dfbe6ad7
Description
•