[ARCH] Improving the Dialog API

[TomMalbran]

This pull request addresses the following issues in the Dialogs API:

• 258: Now the templates that don't use a template are templatized. This is done by using a generic template for all, a JSON object to define the variables for the default dialogs and a title and message parameters that replace the template variables if defined. The template is rendered and then the dialog is generated. This also adds a new function to create a new Dialog by passing an object instead of using the pre-defined ones.

• 3077: All the show dialog functions now return a new Dialog instance, which has the promise the dialog element and a close method. These can then be easily expanded to add more methods.

  Since the buttons are links and there is only one button at the left side, by removing the float: right and aligning them to the right I was able to be displayed in the order they appear in the template.

• 1089: Since now the buttons are displayed in the order they appear in the template, by going back to the old styles I was able to reverse the order of the Save/Cancel buttons just on windows.

I can split the request if needed, but since all are related to the dialogs I thought it was better to address them all together.

[TomMalbran]

I was checking the dialogs that use templates, and the only real difference is the content inside .modal-body. So I was thinking that in default-dialogs.json a new content property could be added, and when creating the template, we check first if content is defined and will use it, but if not it will use the message inside a p element like now. So then the buttons and title information from the dialogs using templates could be added to default-dialogs.json and showModalDialog would receive an extra content parameter, or even better an object containing any of title, message or content. Finally showModalDialogUsingTemplate, could be make private, and even change the name to createModalDialog, since the preferred way to create dialogs for brackets is showModalDialog and for extensions is showModalDialogFromJSON (the name can be different if needed). This way, every template will have the same design for the title, and buttons.

[njx]

Hi @TomMalbran -- we finally got around to discussing this one. Overall, the approach looks great, but we had a couple of suggestions:

• One thought was to preserve the existing API by having the returned object actually extend the Promise object instead of wrapping it. I normally don't like this sort of thing, but in this case it seems like keeping backwards compatibility might be worth it for now, and there's a good argument to be made that most dialog users will really just want the promise anyway and won't need the other methods. Later on, we'll have a better opportunity to make breaking API changes when we do a major extension API revamp.
• In general, people felt like breaking out some of the dialog declarations into a JSON file was overkill--there didn't seem to be much value in that part over and above just supplying the arguments directly in calls to the dialog functions. So it seemed like we could just merge those back into the code.

[TomMalbran]

Thanks for checking this.

1. You are right. I had to add a lot of getPromise() to just make it work in Brackets, so I guess it can be similar in the extensions. Extending the promise sounds good. An alternative would be a parameter that when true it would return the Dialog object and on false the promise, or an additional function to get it after the dialog was created.
2. I don't think it is an overkill. I did it like that mainly to save the buttons that don't change from dialog to dialog. Where would those fit best in the code? I don't want to need to pass them when making the dialogs, or back as multiple templates, although multiple templates isn't that bad. So maybe as an object inside Dialogs.js?

[njx]

For (2), I think the reason it doesn't seem that useful to have it factored out into the JSON file is that most of these dialogs are only used once anyway, so you might as well just have the caller pass in the buttons. (The "title" and "message" fields are all blank in the JSON anyway, and they always get replaced with custom titles/messages so there isn't really any point in having those factored out either.) The only dialog in that list that gets reused often is the error dialog, and in that case there's just an "OK" button anyway.

[njx]

Thanks for the merge. Any thoughts on getting rid of the JSON file per the last comment above?

[TomMalbran]

Yes, I will make it another parameter of showModalDialog, maybe use the error buttons as the default ones, so we don't need to pass those so many times, and remove showModalDialogUsingJSON. I just didn't had much time this past week. I'll try to update it this weekend or next week if possible.

About extending the promise object. Right now we are only using the done method, since it can't never actually fail. So it might be easier to have a done method on the returned Dialog object, that invokes the done method of the promise? If is required to use any other method/property of the promise object you could just retrieve it like now and use it.

[TomMalbran]

I've made the following changes on this update:

• Since the promise is used t know when the dialog is closed, the promise is never rejected, so the methods fail and always aren't used, so I just added a done method to the Dialog object that calls the promise done method. If other stuff is required from the promise it can be easily obtained from the getter. I changed some always methods used to done.
• Show Modal Dialog now has a buttons parameters that receives an array of buttons as where in the JSON file. Since the error dialogs is used a lot, the ok button is the default button, which makes it work as an alert when no buttons are passed. This reduce a lot the code to be added to pass to the function.
• I then removed the JSON file and the Show Modal Dialog from JSON, since Show Modal Dialog covers it now.
• I also changed the use of links for the buttons from proper buttons elements.

[TomMalbran]

This 2 modules aren't required anymore so I removed them.

[gruehle]

Hey @TomMalbran - thanks for the updates. The changes all look good. I just had a couple minor comments.

[TomMalbran]

@gruehle Thanks for the review. I addressed all your comments and added a few more fixes that I found going over the code multiple times.

[TomMalbran]

I noticed that I haven't addressed what was mentioned in #3522. Even if the changes where not used, some of the things mentioned could still be used. The Dialog class could have the handle key function as one of the method and the Dialog would store the autoDismiss property, we we can then save an array of Dialogs. The dialog could also have the add and remove listeners methods.

[njx]

@TomMalbran @gruehle I think as long as the nested dialog key handling code that we ended up going with (using the keydownListeners array) is still working, we don't really need to do anything more here. (We ended up merging a simpler solution than #3522 that didn't require maintaining a stack of dialogs--it just keeps a stack of the actual listener closures, which works just as well.)

To make sure it's still working, just bring up Extension Manager and click on the Install from URL button, then make sure you can type into the install field, and that the first ESC closes the install dialog and the second ESC closes the extension manager.

[gruehle]

I tested with nested dialogs and everything worked fine.

@TomMalbran - there is a merge conflict in FileSyncManager.js. Could you clean that up? Everything else is ready to go.

[TomMalbran]

@gruehle Merged with master and solved the conflict issue. If I didn't missed any change, which I believe I didn't, this might be ready.

[gruehle]

Looks good to me. Thanks @TomMalbran!

[iwehrman]

It looks like this change breaks modal dialogs in the EWF extension. Would someone mind emailing the brackets-dev Google group with a little summary of the changes and perhaps some advice on how clients can update their code? Thanks!

[gruehle]

@TomMalbran - could you add the API changes to the Sprint 26 release notes?

[TomMalbran]

@iwehrman The problem is that showModalDialog now creates a dialog from the passed parameters instead of creating it from an inserted dom. You should change the code to use showModalDialogUsingTemplate, and pass the rendered template as a parameter. The template could be rendered once and saved in a variable.

@gruehle I'll try to add it, but I might need some help, so if you can, please let me know or change what would be required after.

[iwehrman]

@TomMalbran Thanks for the info! Hey, I'm just gonna throw this out there: it looks like you did a bang-up job patching up the modals in Brackets on this pull; if you want to help out with another project, it would be awesome if you could do the same for the Adobe Edge Web Fonts Brackets extension. If you don't have time, no biggie; it looks like the changes will be straightforward. But, if you do, mention issue adobe/brackets-edge-web-fonts#133 and I'll merge it in straightaway!

[TomMalbran]

@iwehrman I just submitted a pull request to the Adobe Edge Web Fonts Brackets extension with a solution.

@gruehle I added some notes about the Dialog API changes, let me know if I missed something.