[Arch] Dialog.done() is confusing and should be removed

[jasonsanjose]

After the Dialog API refactoring in #3086, the Dialogs module methods returned a dialog instance instead of a Promise as they did prior to sprint 25. It appears that the Dialog class has a done() method for backwards compatibility with clients that still expect a promise.

The usage is confusing (see #4102 and #4087). Ideally we would deprecate done and remove it.

[jasonsanjose]

cc @TomMalbran in case there's other issues I'm missing

[TomMalbran]

It seems like is an useful API, but it can be confusing. What about eventually renaming it? Maybe just call it onClose() or something like that? Maybe have both API's and have done() return a API deprecation console warning as we started to do?

[peterflynn]

If we flat-out remove it, it'll beak every extension that opens any sort of dialog. Probably better to deprecate. But actually, I'd vote for keeping it -- the 90% use case seems to be calling done() and doing nothing else with the returned Dialog instance.

[pthiess]

Assigned at @gruehle - this is an architecture issue.

[gruehle]

After further discussion at the architecture meeting, we decided to leave the API as-is (at least for now).

Here are the reasons:

• The current API covers the 90% use case where you just need to be notified when a dialog is closed, and which button was pressed.
• Chaining to return something like a promise or the dialog instance is misleading, and could be dangerous. If you really need to get the promise, use dialog.promise. If you really need the dialog instance, it's easy enough to find.

We may revisit this as part of the extensions API overhaul proposed here.

[TomMalbran]

I agree that the API is good, but the naming might be confusing. You could expect that done is used when a dialog is closed using an Ok button, and have fail for the other cases. I think it will be better to rename it to onClose and even have onAccept and onCancel methods to mimic the done/fail/always of a promise in a better way.