Add Script Editor language-specific subclasses

[karlaspuldaro]

Fixes #1517
This PR introduces new PythonEditor and REditor sublasses, making the ScriptEditor class language agnostic.
The existing functionality is maintained.

How was this pull request tested?

Ran existing Script Editor integration tests.

Developer's Certificate of Origin 1.1

   By making a contribution to this project, I certify that:

   (a) The contribution was created in whole or in part by me and I
       have the right to submit it under the Apache License 2.0; or

   (b) The contribution is based upon previous work that, to the best
       of my knowledge, is covered under an appropriate open source
       license and I have the right under that license to submit that
       work with modifications, whether created in whole or in part
       by me, under the same open source license (unless I am
       permitted to submit under a different license), as indicated
       in the file; or

   (c) The contribution was provided directly to me by some other
       person who certified (a), (b) or (c) and I have not modified
       it.

   (d) I understand and agree that this project and the contribution
       are public and that a record of the contribution (including all
       personal information I submit with it, including my sign-off) is
       maintained indefinitely and may be redistributed consistent with
       this project or the open source license(s) involved.

[elyra-bot]

Thanks for making a pull request to Elyra!

To try out this branch on binder, follow this link:

[kevin-bates]

I suspect both constructors (PythonEditor and REditor) could be removed simply by treating ScriptEditor as an abstract base class that defines two abstract methods: getLanguage() and getIcon() that PythonEditor and REditor implement. Then, wherever there's this.editorLanguage in ScriptEditor, it is replaced with this.getLanguage() and same goes for this.title.icon -> this.getIcon(). The intialize Kernelspecs just moves to the ScriptEditor constructor as well.

You probably don't need the fields this.editorLanguage or this.title.icon, although, if they're a hassle to remove, the ScriptEditor constructor could just be like this...

    this.editorLanguage = this.getLanguage();

    // Add icon to main tab
    this.title.icon = this.getIcon();

    this.context.ready.then(() => {
      this.initializeKernelSpecs();

Since the subclasses know what they are, the fields could probably be made private.

I also suspect the factory stuff could be simplified just based on java/python experiences. Typically, the factory for something like this would be a class method on ScriptEditor like ScriptEditor.createInstance() where createInstance() takes an "identifying characteristic" corresponding to the class to create. Language would probably be a good "id".

Take this with a grain of salt since I don't know how much applies in the JS world.

[lresende]

See comments above about rIcon

[lresende]

why are we passing labIcon or string? we do have an rIcon defined in ui-components and thus I was expecting to use that in favor of consistency.

[karlaspuldaro]

I believe our rIcon is created differently, that's why we define it with a string, I didn't change the original way we refer to it.
When I import it from ui-components library the same way we add python icons, the r icon isn't loaded properly in the launcher, file browser or editor tab:

@ajbozarth might be able to elaborate on why/how that works?

[ajbozarth]

I can't remember why this fix works, but there's an issue with LabIcons where depending on the DOM within a svg added multiple copies of it causes problems. The R icon is the icon we found that has this issue.

[ajbozarth]

A couple clarification questions for @karlaspuldaro otherwise LGTM

[ajbozarth]

I can't remember why this fix works, but there's an issue with LabIcons where depending on the DOM within a svg added multiple copies of it causes problems. The R icon is the icon we found that has this issue.

[ajbozarth]

two comments on this (both non-blocking):

1. Why the name change from ScriptEditorFactory to ScriptEditorWidgetFactory?

2. Why move this into a separate file?

[karlaspuldaro]

1. The name has changed to make it more clear that it is a widget factory class.
2. It has moved to its own file because the editor is independent of its factory (imo this should have been done before). Makes it easier to maintain (the ScriptEditor class will become more complex with the debugger), follows common conventions and doesn't expose to ScriptEditor things it doesn't need to know about from certain libraries.