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

[core-ai-bot]

Issue by jasonsanjose
Thursday Jun 06, 2013 at 18:29 GMT
Originally opened as adobe/brackets#4125

---

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.

[core-ai-bot]

Comment by jasonsanjose
Thursday Jun 06, 2013 at 18:30 GMT

---

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

[core-ai-bot]

Comment by TomMalbran
Thursday Jun 06, 2013 at 20:19 GMT

---

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?

[core-ai-bot]

Comment by peterflynn
Thursday Jun 06, 2013 at 20:28 GMT

---

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.

[core-ai-bot]

Comment by pthiess
Monday Jun 10, 2013 at 18:18 GMT

---

Assigned at@gruehle - this is an architecture issue.

[core-ai-bot]

Comment by gruehle
Friday Jul 26, 2013 at 16:55 GMT

---

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.

[core-ai-bot]

Comment by TomMalbran
Friday Jul 26, 2013 at 20:40 GMT

---

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.