[REFACTORING] Add interface for code editors #4341

jpelay · 2023-06-14T18:50:15Z

Description

This is the first step towards replacing the Ace editor with CodeMirror, at least for languages that don't use the latin alphabet. This PR will build an interface that must be followed by whatever implementation we decide to use.

fixes #4339

How to test

Everything should work the same once everything is done

jpelay · 2023-06-26T17:54:45Z

This is done! And the tests pass. There's a part that I'd like to discuss a bit more before moving forward, and that is the debugger. I think that maybe we can do an interface for the debugger in another PR, but we can discuss that tomorrow!

jpelay · 2023-06-27T18:13:19Z

After discussing today in the meeting we are planning to move this refactoring forward, and in the future we want to add an interface for the debugger, but since we need to learn how to to that in CodeMirror is better leaving that for a further PR.

rix0rrr

I love what you're doing! Moving it all into one place makes the editor code much more manageable.

I happen to do TypeScript every day so I have a number of comments/tips 😉 but don't wait for me to merge. This is definitely better than we have today, and I don't want to stand in the way of progress.

As general comments, I think the following tips:

The smaller an API is, the better. So any property/method/whatever has to earn their keep and be there for a reason. If we can rewrite something to get rid of it, so much the better!
Let's try to keep a clear separation of responsibilities. If something depends on Ace, it should be in this class. If something does not, then it shouldn't.
Think of the life cycle of an object. What information can an object have at creation time? What stays constant during its lifetime? That information can get passed in via the constructor.

But again, good work!

rix0rrr · 2023-07-03T20:38:01Z

static/js/editor-markers.ts

+ * 
+ */
+
+export interface HedyEditorMarker {


Is this still used?

I forgot to delete this! The plan here was to make an interface of markes used in both editors. But I think that's not really doable, since both editor use quite a different way of handling markers and annotations.

rix0rrr · 2023-07-03T20:38:53Z

static/js/editor.ts

+   * Ininitialize an editor that appears in a modal
+   * @param $editor reference to the div that contains this editor
+   */
+  initializeModalEditor: ($editor: JQuery) => HedyEditor;


What's the difference between an editor that appears in a modal and an editor that appears elsewhere?

I wonder if it's the same as the distinction between the "main" editor and not?

That's an interesting question. I think so .The main editor has different options than the modal editor, like adding options for the ask modal, and being runnable. It also has the debugger attached.

rix0rrr · 2023-07-03T20:41:03Z

static/js/editor.ts

+   * @param isReadOnly to decide weather to remove the cursor
+   * @param isMainEditor should we show the line numbers
+   */
+  turnIntoEditor: (element: HTMLElement, isReadOnly: boolean, isMainEditor: boolean) => HedyEditor;


Would we ever set isMainEditor: true here? Because we also have initializeMainEditor, seems like you would call that instead?

If it so happens to be the case that initializeMainEditor calls turnIntoAceEditor -- then that's fair enough, but now that we have classes and everything, you could have a private isMainEditor: boolean field on the class, and then turnIntoEditor could read that field.

And if you are first leaving everything mostly the same before you start further cleanup, to make the refactor simpler -- even more fair enough! 😅

If it so happens to be the case that initializeMainEditor calls turnIntoAceEditor -- then that's fair enough, but now that we have classes and everything, you could have a private isMainEditor: boolean field on the class, and then turnIntoEditor could read that field.

Yeah exactly! initializeMainEditor calls turnIntoAceEditor I imagine that that field would be not in the class HedyAceCreator but rather in the class HedyAceEditor itself?

And if you are first leaving everything mostly the same before you start further cleanup, to make the refactor simpler -- even more fair enough! sweat_smile

Also yes 😅 this first refactor was about putting everything in a single place, and seeing what could go in the interface!

rix0rrr · 2023-07-03T20:41:56Z

static/js/editor.ts

+  /**
+ * Represents whether there's an open 'ask' prompt
+ */
+  askPromptOpen: boolean;


Is this really an editor feature? Or is it more about the chrome that contains the editor?

I thought a bit about whether to include this variable in the interface or not. This variable is used in the main editor to focus on the ask modal, and other things. I included here, because I think that aspects that control HTML of the page that contains the editor and also control behaviors of what one would think is the editor, should go in this interface.

rix0rrr · 2023-07-03T20:46:36Z

static/js/editor.ts

+   * Set the highlither rules for a particular level
+   * @param level      
+   */
+  setHighliterForLevel: (level: number) => void;


"highlight" 🙃

This declares a function-valued member, which of course works. But, you will also be able to assign the member if you declare it like this, because it's a read/write field.

If you want to declare a method, it looks like this:

interface HedyEditor { setHighlighterForLevel(level: number): void; }

Isn't this function more about setting a level than setting a highligher?

"highlight" upside_down_face

Nice catch!

This declares a function-valued member, which of course works. But, you will also be able to assign the member if you declare it like this, because it's a read/write field.

If you want to declare a method, it looks like this:

interface HedyEditor { setHighlighterForLevel(level: number): void; }

Thanks for the tip!

Isn't this function more about setting a level than setting a highlighter?

You mean it should be called, setLevel? I think so, but maybe setHighlighterForLevel is more explicit?

rix0rrr · 2023-07-03T20:57:34Z

static/js/ace-editor.ts

+    this._editor?.setOptions(options);
+  }
+
+  get editor(): AceAjax.Editor {


If this editor needs to be public (why?) then you can declare as a public member and you don't need the getter anymore.

public readonly editor: AceAjax.Editor;

The editor being public is a temporary measure as I further refactor the debugger and the markers!

rix0rrr · 2023-07-03T20:58:39Z

static/js/ace-editor.ts

+      stopDebug();
+    });
+
+    this._markers = new Markers(this._editor!);


The functionality of the Markers will probably also need to move into this class in the future. Because that contains stuff like "set a marker on line 3", and that's going to be editor-specific as well.

But probably leave that for a future PR, no need to boil the ocean in this one!

The functionality of the Markers will probably also need to move into this class in the future. Because that contains stuff like "set a marker on line 3", and that's going to be editor-specific as well.

But probably leave that for a future PR, no need to boil the ocean in this one!

Yes! But that requires also a bit of digging about markers in CodeMirror, and see what they share and build an abstraction that suits them both

rix0rrr · 2023-07-03T21:00:38Z

static/js/ace-editor.ts

+    // If prompt is shown and user enters text in the editor, hide the prompt.
+    this._editor?.on('change', () => {
+      if (this.askPromptOpen) {
+        stopit();


Now the editor needs to know about the JS running, and it needs to know about HTML elements that are not itself.

This is a bit of mixing responsibilities that we probably shouldn't have.

I think the best thing to do is to probably make the HedyEditor emit a change event itself (either by being a full-blown EventEmitter, or by making some simple callback mechanism ourselves), and leaving this logic where it was.

We should have some EventEmitter code in a file somewhere. If not, we can always build something like this ourselves

type ChangeListener = (x: string) => void; interface HedyEditor { public readonly onChange: ChangeListener[]; } class AceHedyEditor { public readonly onChange = new Array<ChangeListener>(); constructor() { this.editor.on('change', () => { const contents = this.getContents(); for (const listener of this.onChange) { listener(contents); } }); } } // Consumer editor.onChange.push((contents) => { console.log('Editor changed to', contents); });

I love this idea!

rix0rrr · 2023-07-03T21:06:04Z

static/js/ace-editor.ts

+  getBreakpoints(): Breakpoints {
+    return this._editor?.session.getBreakpoints() as unknown as Breakpoints;
+  }


Same here, we probably need the functionality inside the class (because the Breakpoints class is also Ace specific)

rix0rrr · 2023-07-03T21:11:37Z

static/js/editor.ts

+export type Breakpoints = Record<number, string>;
+
+
+export interface HedyEditorCreator {


Ahh, I see what you're doing. Makes it easy to switch out all different entry points in one go later, right?

Clever!

Felienne

Let's merge despite the small issues. @jpelay, shall we make 1 issue for the remaining open points so we don't forget?

mergify · 2023-07-06T13:13:06Z

Thank you for contributing! Your pull request is now going on the merge train (choo choo! Do not click update from main anymore, and be sure to allow changes to be pushed to your fork).

jpelay · 2023-07-06T13:21:41Z

Hi @rix0rrr! Thank you for your comments! Felienne and I discussed and we decided to merge this and address your comments ina future PR. I will also try to answer the questions you have about the code.

mergify · 2023-07-06T16:46:43Z

Thank you for contributing! Your pull request is now going on the merge train (choo choo! Do not click update from main anymore, and be sure to allow changes to be pushed to your fork).

**Description** After we merged #4341 there was a error were the errors were not being shown properly. This was a front-end error that happened because I didn't change a coupe of calls to the markers interface. This PR fixes that, in a way that needs to be addressed furthermore when we keep on working on #4382, namely: the Hedy Interface shouldn't have Markers inside it. **Fixes #4393 ** **How to test** Make some errors and see if the skipping errors feature works properly and the errors are being shown. **Checklist** Done? Check if you have it all in place using this list:* - [ ] Contains one of the PR categories in the name - [ ] Describes changes in the format above - [ ] Links to an existing issue or discussion - [ ] Has a "How to test" section If you're unsure about any of these, don't hesitate to ask. We're here to help!

@rix0rrr

**Description** This PR tackles the different comments left by @rix0rrr in #4341. The changes requested are the following: - [x] [Delete or develop the Hedy Editor Markers interface](#4341 (comment)) Deleted for now. - [x] [Maybe remove `initializeModalEditor` if the difference between a main editor if the same as "main" editor and not ](#4341 (comment)) - [x] [Remove `askPromptOpen` ](#4341 (comment)) - [x] [Change function-valued members to methods](#4341 (comment)) - [x] [Change name of `getValue` method to something more descriptive and maybe turn it into a read-only property](#4341 (comment)) - [x] [Cahge `isReadOnly` from method to read-only property passed in the constructor](#4341 (comment)) (~not passed in the constructor yet~ done!, and can't put it to read only mode due to [this](https://github.com/hedyorg/hedy/blob/345dc3b26e66229483d863702b4d20a1212a489c/static/js/app.ts#L1629) and [this](https://github.com/hedyorg/hedy/blob/345dc3b26e66229483d863702b4d20a1212a489c/static/js/app.ts#L1658) ) - [ ] [Remove `trimTrailingSpace` method? Maybe do it automatically in `getValue`](#4341 (comment)) - [x] [Remove `configureMainEditor` as part of the generic interface](#4341 (comment)) - [x] [Remove `setEditorMode`](#4341 (comment)) - [x] [Give the class a constructor and guarantee that there's always an editor.](#4341 (comment)) - [x] [The editor doesn't need to be public](#4341 (comment)) - [ ] [The markers will probably need to be merged into the ace class.](#4341 (comment)) Left for a future PR - [x] [Make the HedyEditor emit a change event itself (either by being a full-blown EventEmitter, or by making some simple callback mechanism ourselves](#4341 (comment)) **Fixes #4382** **How to test** Everything should work as expected

**Description** One of the points in #4341 was to move the markers functionality to the editor interface. This PR does that, also modifying the debugger file in accordance to the new interface. This changes will allow me to keep working on #4479, as this is a previous step to that change. I basically just copy and pasted the `Markers` class into the `HedyAceEditor` class, modifying it when needed. I also added a couple more editor events, since that was needed in order for the debugger to work. **How to test** Everything should work as previously.

Switching the keyword language produced "converting circular JSON" error message. The reason was that we were trying to post the entire editor object as JSON to the server, instead of just the code from the editor (bug introduced in the following refactoring #4341).

Switching the keyword language produced "converting circular JSON" error message. The reason was that we were trying to post the entire editor object as JSON to the server, instead of just the code from the editor (bug introduced in the following refactoring #4341). **How to test** Log in, set your language to non-English and your preferred keyword language to non-English. Then use the language switcher on a code page.

add sketch of interface for editor

a50868f

jpelay added the wip Work in progress label Jun 14, 2023

jpelay marked this pull request as draft June 14, 2023 18:51

jpelay added 5 commits June 16, 2023 14:46

adding more functions to the interface

051550d

adding more functions to the interface

aa66137

following gang of four advice

331a7f0

keep refactoring some more

9213a3f

almost there

730b3ae

jpelay added 3 commits June 26, 2023 13:55

Merge branch 'main' of github.com:jpelay/hedy into editor-interface

ed3b2b4

Merge branch 'main' of github.com:hedyorg/hedy into editor-interface

a9e62b0

adding javascript generated files

f5f998d

jpelay marked this pull request as ready for review June 27, 2023 18:07

Merge branch 'main' into editor-interface

326a8f9

jpelay changed the title ~~[WIP, REFACTORING] Add interface for code editors~~ [REFACTORING] Add interface for code editors Jun 27, 2023

jpelay removed the wip Work in progress label Jun 27, 2023

jpelay requested review from rix0rrr and Felienne June 27, 2023 18:20

rix0rrr reviewed Jul 3, 2023

View reviewed changes

Felienne approved these changes Jul 6, 2023

View reviewed changes

jpelay added 2 commits July 6, 2023 12:30

Merge branch 'main' of github.com:hedyorg/hedy into editor-interface

803d2eb

merge source map listener to ace editor class

43b49fa

mergify bot merged commit 82e0b77 into hedyorg:main Jul 6, 2023

ToniSkulj mentioned this pull request Jul 23, 2023

[BUG] Skipping mistakes does not work anymore #4393

Closed

jpelay mentioned this pull request Aug 1, 2023

[FIX] Fix errors not being displayed properly #4403

Merged

4 tasks

jpelay mentioned this pull request Aug 28, 2023

[REFACTORING] Improve Hedy Editor interface #4435

Merged

13 tasks

jpelay mentioned this pull request Sep 18, 2023

[REFACTORING] Moves markers functionality to editor interface #4508

Merged

rix0rrr mentioned this pull request Oct 20, 2023

[FIX] Fix inline keyword language switcher #4654

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REFACTORING] Add interface for code editors #4341

[REFACTORING] Add interface for code editors #4341

jpelay commented Jun 14, 2023 •

edited

Loading

jpelay commented Jun 26, 2023

jpelay commented Jun 27, 2023

rix0rrr left a comment

rix0rrr Jul 3, 2023

jpelay Jul 6, 2023

rix0rrr Jul 3, 2023

jpelay Jul 6, 2023

rix0rrr Jul 3, 2023

jpelay Jul 6, 2023

jpelay Jul 6, 2023

rix0rrr Jul 3, 2023

jpelay Jul 6, 2023

rix0rrr Jul 3, 2023

jpelay Jul 6, 2023

rix0rrr Jul 3, 2023

jpelay Jul 6, 2023

rix0rrr Jul 3, 2023

jpelay Jul 6, 2023

rix0rrr Jul 3, 2023

rix0rrr Jul 3, 2023

jpelay Jul 6, 2023

rix0rrr Jul 3, 2023

jpelay Jul 6, 2023

rix0rrr Jul 3, 2023

Felienne left a comment

mergify bot commented Jul 6, 2023

jpelay commented Jul 6, 2023

mergify bot commented Jul 6, 2023

		export type Breakpoints = Record<number, string>;


		export interface HedyEditorCreator {

[REFACTORING] Add interface for code editors #4341

[REFACTORING] Add interface for code editors #4341

Conversation

jpelay commented Jun 14, 2023 • edited Loading

jpelay commented Jun 26, 2023

jpelay commented Jun 27, 2023

rix0rrr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Felienne left a comment

Choose a reason for hiding this comment

mergify bot commented Jul 6, 2023

jpelay commented Jul 6, 2023

mergify bot commented Jul 6, 2023

jpelay commented Jun 14, 2023 •

edited

Loading