Closed Bug 1511945 Opened 6 years ago Closed 6 years ago

DeCOMtaminate FileLink

Categories

(Thunderbird :: FileLink, enhancement)

Product:
Thunderbird
Component:
FileLink
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 67.0

People

(Reporter: darktrojan, Assigned: darktrojan)

References

Details

Attachments

(5 files, 3 obsolete files)

1511945-cloudfile-decom-3.diff
239.25 KB, patch
Fallen
: feedback+
Details | Diff | Splinter Review
1511945-cloudfile-schema-1.diff
7.90 KB, patch
Fallen
: review+
jorgk-bmo
: approval-comm-beta+
jorgk-bmo
: approval-comm-esr60+
Details | Diff | Splinter Review
Screen Shot 2019-02-14 at 00.38.10.png
79.57 KB, image/png
Details
1511945-cloudfile-decom-4.diff
249.96 KB, patch
Paenglab
: review+
Paenglab
: ui-review+
Details | Diff | Splinter Review
1511945-cloudfile-decom-5.diff
247.47 KB, patch
Details | Diff | Splinter Review
Now that we have a shiny new API, we can tidy up some of the mess behind the scenes and get rid of the XPCOM stuff.
Assignee: nobody → geoff
Status: NEW → ASSIGNED
No longer blocks: 1481052
Depends on: 1481052
Attached patch 1511945-cloudfile-decom-1.diff (obsolete) — — Splinter Review

Work in progress. It passes all the tests but I'm not totally happy with it. There are all sorts of unneeded bits lying around now and I think some of the methods could be improved.

What I've done, in short:

  • Made a proper distinction between providers and accounts. This has been super confusing. A provider is now just an object with some fields and an "initAccount" method.
  • Changed the UI so the add account dialog has gone away. I haven't made anything different happen for an unconfigured account yet, but this will need to be fixed.
  • Removed as much XPCOM stuff as possible.
  • Updated all the in-tree implementations and tests to match.
Attachment #9036460 - Flags: feedback?(philipp)

I've just discovered that the Box provider is broken since I changed the Hightail provider and loaded them both in the same scope. So if you try to use it you're gonna have a bad time.

Attached patch 1511945-cloudfile-decom-2.diff (obsolete) — — Splinter Review

Further updates. I still haven't done anything with unconfigured accounts.

Attachment #9036460 - Attachment is obsolete: true
Attachment #9036460 - Flags: feedback?(philipp)
Attachment #9039464 - Flags: feedback?(philipp)
Attachment #9039464 - Attachment is obsolete: true
Attachment #9039464 - Flags: feedback?(philipp)
Attachment #9041369 - Flags: feedback?(philipp)

We never use new_account_url, and plan to stop using settings_url, so I want to mark them as deprecated. I've also added a bunch of missing descriptions.

Attachment #9041629 - Flags: review?(philipp)
Attachment #9041629 - Flags: approval-comm-esr60?
Attachment #9041629 - Flags: approval-comm-beta?

I haven't mentioned, because I've been massively sidetracked, that attachment 9041369 [details] [diff] [review] is complete apart from the Hightail management UI. There are further things I want to do (namely replace the callback mess with promises), but this is a good place to pause.

Attachment #9041629 - Flags: review?(philipp) → review+
Comment on attachment 9041369 [details] [diff] [review] 1511945-cloudfile-decom-3.diff Review of attachment 9041369 [details] [diff] [review]: ----------------------------------------------------------------- Generally looks like a good approach, I'm sure there is more we could clean up but this already goes a long way. Is there a visual change as well? Maybe you can add a screenshot. ::: mail/components/cloudfile/box/management.xhtml @@ +12,5 @@ > <head> > <script type="application/javascript" > src="chrome://messenger/content/protovis-r2.6-modded.js"/> > <script type="application/javascript" > + src="chrome://messenger/content/cloudfile/Box/management.js" If you can make any of these script type="module" I think that will help us further on when it is used more widely.
Attachment #9041369 - Flags: feedback?(philipp) → feedback+
Keywords: checkin-needed
Keywords: leave-open

What do you want me to land here? This thing? Doesn't look like it would change anything functionally.

Attachment 9041629 [details] [diff]. It might not look like much but it will tell extension developers that what they're spending their time developing is going to disappear in a few months.

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/df9638be9883
Mark some cloud file provider properties as deprecated in the schema. r=philipp DONTBUILD

Keywords: checkin-needed
Target Milestone: --- → Thunderbird 67.0
Comment on attachment 9041629 [details] [diff] [review] 1511945-cloudfile-schema-1.diff https://hg.mozilla.org/releases/comm-esr60/rev/be29b0c0d1d850af78a09aef2fa2190f8dd4acfd
Attachment #9041629 - Flags: approval-comm-esr60?
Attachment #9041629 - Flags: approval-comm-esr60+
Attachment #9041629 - Flags: approval-comm-beta?
Attachment #9041629 - Flags: approval-comm-beta+
Attached patch 1511945-cloudfile-decom-4.diff (obsolete) — — Splinter Review
Attachment #9043580 - Flags: review?(philipp)

(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #7)

 <script type="application/javascript"
         src="chrome://messenger/content/protovis-r2.6-modded.js"/>
 <script type="application/javascript"

  •        src="chrome://messenger/content/cloudfile/Box/management.js"

If you can make any of these script type="module" I think that will help us
further on when it is used more widely.

Neither are modules, although I don't really understand your point so maybe I'm missing something.

Comment on attachment 9043580 [details] [diff] [review] 1511945-cloudfile-decom-4.diff Review of attachment 9043580 [details] [diff] [review]: ----------------------------------------------------------------- Only updating the comment is really something for this bug. Code looks fine otherwise. You should get a ui-review on this from Richard. My comment on the UI, I can imagine the buttons will be a bit heavy especially if there are more than a few providers. Can we make this a dropdown similar that holds the providers to add? ::: mail/components/cloudfile/box/management.js @@ +4,5 @@ > > // mail/base/content/protovis-r2.6-modded.js > /* globals pv */ > > +var {cloudFileAccounts} = ChromeUtils.import("resource:///modules/cloudFileAccounts.js"); I really would prefer object-curly-spacing = always, but it seems this isn't common style for this directory. @@ +26,5 @@ > + loading.hidden = false; > + auth.hidden = true; > + > + account.createExistingAccount({ > + onStartRequest() { Next step: use promises instead of request listeners. ::: mail/components/cloudfile/hightail/nsHightail.js @@ +3,5 @@ > * You can obtain one at http://mozilla.org/MPL/2.0/. */ > > /* This file implements the nsIMsgCloudFileProvider interface. > * > * This component handles the Hightail implementation of the This comment needs updating. Maybe you can also add some words on why we are not exporting anything here. That aside, importing a file for side effects sounds a bit strange to me. Maybe consider exporting something and then registering in the caller?
Attachment #9043580 - Flags: review?(philipp) → review+

That aside, importing a file for side effects sounds a bit strange to me. Maybe consider exporting something and then registering in the caller?

It's hardly without precedent, and exporting something just to use it once is rather messy in this case.

Comment on attachment 9043580 [details] [diff] [review] 1511945-cloudfile-decom-4.diff Richard, what do you think? (Especially regarding comment 15.) Personally I don't think someone would have so many providers that the UI becomes unwieldy. We ship two, a user might add, I guess, one or two add-ons with providers. There's a screenshot attached that might help.
Attachment #9043580 - Flags: ui-review?(richard.marti)

This is the same patch with the CSS changes added for Mac and Windows.

Yes, I don't think a user will have too many different providers installed. But could the Add button be removed for the provider that is already added? Then only the not added providers are shown to add.

WeTransfer shows no icon in the list and on the button.

Attachment #9043580 - Attachment is obsolete: true
Attachment #9043580 - Flags: ui-review?(richard.marti)
Attachment #9044298 - Flags: ui-review+
Attachment #9044298 - Flags: review+

(In reply to Richard Marti (:Paenglab) from comment #18)

Created attachment 9044298 [details] [diff] [review]
1511945-cloudfile-decom-4.diff

This is the same patch with the CSS changes added for Mac and Windows.

Yes, I don't think a user will have too many different providers installed. But could the Add button be removed for the provider that is already added? Then only the not added providers are shown to add.

WeTransfer shows no icon in the list and on the button.

I don't know if it is reasonable, but I could see someone adding a "home" and "work" account for the same provider? I'm not sure you'd even be able to tell the difference in the UI anywhere though.

(In reply to Patrick Cloke [:clokep] from comment #19)

I don't know if it is reasonable, but I could see someone adding a "home" and "work" account for the same provider? I'm not sure you'd even be able to tell the difference in the UI anywhere though.

Good catch. The label is editable.

My concern is that we'd end up with button overflow if the user does add more than just a few providers, or their Thunderbird is resized fairly small. The UI should behave gracefully if the user does something we don't expect, as far as possible. Could we maybe use buttons as is if there are less than 4 providers, and turn it into a menu if there are more? (4 being arbitrary)

We will want to allow for more than one account per provider, so we might need to add a provider-specified differentiator to the list.

(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #21)

My concern is that we'd end up with button overflow if the user does add more than just a few providers, or their Thunderbird is resized fairly small. The UI should behave gracefully if the user does something we don't expect, as far as possible. Could we maybe use buttons as is if there are less than 4 providers, and turn it into a menu if there are more? (4 being arbitrary)

Yes, but I'm going to ship this first.

(In reply to Richard Marti (:Paenglab) from comment #18)

WeTransfer shows no icon in the list and on the button.

I see this now and I'm certain it's not caused by this bug.

When is the second part going to land? We're thinking about TB 66 beta 2.

OK, the second part has string changes, so I'll just take the first part to beta. Sorry about the noise.

I do not want the second part on beta anyway.

A few changes.

Pushed by geoff@darktrojan.net: https://hg.mozilla.org/comm-central/rev/f00a9035dd99 DeCOMtaminate FileLink and improve its preferences UI; r=Fallen

I forgot this had leave-open. Will do stage 2 in another bug.

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Keywords: leave-open
Resolution: --- → FIXED
Depends on: 1531595
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: