[REFACTORING] Improve Hedy Editor interface #4435

jpelay · 2023-08-28T18:37:52Z

Description

This PR tackles the different comments left by @rix0rrr in #4341. The changes requested are the following:

Fixes #4382

How to test

Everything should work as expected

jpelay · 2023-09-04T01:07:24Z

I changed the way in which we create the editors, and now all of the internal logic that has to do with Ace is handled in the constructor. I kept the factory, but now there are only 2 methods there: initializeWritableEditor and initializeReadOnlyEditor that in turn call the constructor of the HedyAceEditor class. I also added an Enum so we can properly set up the editor when we call the constructor. The next step now is to remove the make the HedyEditor emit events itself!

jpelay · 2023-09-06T16:05:21Z

static/js/editor.ts

-  setEditorMode: (isReadMode: boolean) => void;
+  getBreakpoints(): Breakpoints;
+  // TODO: improve this type definition
+  on(key: any, handler: any): void;


Guys I don't know how I can put the types that I used in the definition of this function here, can someone help?

Let's start with the declaration of HedyAceEditor.on():

public on(key: Parameters<typeof this.editorEvent.on>[0], handler: Parameters<typeof this.editorEvent.on>[1]) {

Let's look at this fragment:

Parameters<typeof this.editorEvent.on>[0]

This says:

Take the type of this.editorEvent.on (typeof turns a value into a type).

Assuming that's a function type, get the types of the parameter types of that function type

Then take the first element of that array

The reason you can't copy/paste this is because it contains typeof this.editorEvent.on; since this is an interface definition and not a class, we can't say "the field editorEvent on myself", because you don't even know that you have such a field.

But, we we know what type this.editorEvent would have if it existed, so we can substitute that!

Because on HedyAceEditor this is its declaration:

private editorEvent = new EventEmitter<EditorEvent>({change: true});

So the type of this.editorEvent is EventEmitter<EditorEvent>. Indexing into a type looks slightly different from indexing into an object (for reasons that I also don't quite understand), so we need to replace typeof this.editorEvent.on => EventEmitter<EditorEvent>['on'].

Putting it all together, the first element would look like:

Parameters<EventEmitter<EditorEvent>['on']>[0]

Now that's obviously an ugly mouthful, so let's introduce a type alias to make that palatable:

type EditorEventEmitter = EventEmitter<EditorEvent>; type OnEditorEventParameters = Parameters<EditorEventEmitter['on']>; on(key: OnEditorEventParameters[0], handler: OnEditorEventParameters[1]): void;

Thank you so muuuuch!!!! I really tried to look this up, but none of my attempts quite worked. Thank you for the detailed explanation!

jpelay · 2023-09-06T16:09:12Z

This PR is ready for review!

rix0rrr

Nice! As always, I have some tips and questions to make you think, but I'll leave it completely up to you what to do with those!

Looking good though!

rix0rrr · 2023-09-06T17:36:08Z

static/js/app.ts

+    theGlobalEditor = editorCreator.initializeWritableEditor($editor, EditorType.MAIN, dir);
+    attachMainEditorEvents();
+    error.setEditor(theGlobalEditor);
+    window.Range = ace.require('ace/range').Range // get reference to ace/range


Oof 😥. I see why this might be done, but sticking it window.Range feels a little dirty to me.

Can we not make a global:

let Range: /* needs a type here, not sure what that needs to be, maybe Ace.Range? */ Ace.Range; // and then Range = ace.require('ace/range').Range;

?

At least it would be keeping out of the global browser object.

The weird part is that this is not being referenced anywhere, so I thought this might be a property of the window object. If that's not the case maybe we can remove this.

window is the object that represents the global scope.

So if you do:

window.banana = 'banana';

At any point later you can do

console.log(banana);

And it will work

I understand. I removed the reference to windows.Range since it wasn't really used anywhere

rix0rrr · 2023-09-06T17:36:58Z

static/js/app.ts

@@ -239,6 +244,79 @@ export function initializeCodePage(options: InitializeCodePageOptions) {
  $('#program_name').on('blur', () => saveIfNecessary());
 }

+function attachMainEditorEvents() {


You might pass the editor in as an argument, and get rid of at least one use of the global variable?

rix0rrr · 2023-09-06T17:37:55Z

static/js/app.ts

+    }
+  });
+
+  // Removed until we can fix the skip lines feature


Ohh, this is the "debugger", isn't it?

What's the issue with it?

No, this is the skip line feature. It had a weird problem where if 2 different programs in different sessions where being executed at the same time and both had errors, they had their errors mixed up. We already have a fix for this in #4414, but we decided to wait a few weeks and keep testing before shipping it.

rix0rrr · 2023-09-06T17:40:33Z

static/js/editor.ts

+  COMMON_MISTAKES,
+  CHEATSHEET,
+  PARSONS,
+  EXAMPLE


Is it important to make a distinction between these? Are they not all "just some syntax highlighted block" ?

All of them require different options! And I didn't want to expose a setOptions method, so I decided to set that up in the constructors. Since there are different kinds of these blocks I created this enum to configure all of them in the constructor. You can see that in the constructor.

rix0rrr · 2023-09-06T17:41:56Z

static/js/message-translations.ts

-    "adventures_restored": "Die Standardabenteuer wurden wiederhergestellt.",
+    "adventures_restored": "The default adventures have been restored!",


rix0rrr · 2023-09-06T17:58:29Z

static/js/ace-editor.ts

+      } else { // It's an example editor
+         // Fits to content size
+        this._editor.setOptions({ maxLines: Infinity });     
+        if(editorType === EditorType.CHEATSHEET) {


Oof, do these really all need to be different?

I think so, because some of them serve different purpose and have different looks, I believe

rix0rrr · 2023-09-06T17:59:07Z

static/js/ace-editor.ts

+        }
+      }
+    } else {
+      if (editorType === EditorType.MAIN) {


I just saw this same if a couple of lines above. Can they be collapsed? Is one of them never executed?

I just deleted one of those if!

rix0rrr · 2023-09-06T18:01:02Z

static/js/ace-editor.ts

+export class HedyAceEditor implements HedyEditor {
+  private _editor: AceAjax.Editor;
+  private _markers?: Markers
+  isReadOnly: boolean;


You probably want to make this

Suggested change

isReadOnly: boolean;

private isReadOnly: boolean;

Because right now in other parts of the code you're accidentally directly accessing this public field, instead of going through the getter/setter as I think you probably intended.

rix0rrr · 2023-09-06T18:01:39Z

static/js/ace-editor.ts

+  /**     
+   * @returns if the editor is set to read-only mode
+   */
+  public get getIsReadOnly(): boolean {


Probably

public get isReadOnly(): boolean { return this._isReadOnly; }

?

Or honestly, can you read directly from the editor you wrapped?

You are so right. I'm using the editor to handle that now.

rix0rrr · 2023-09-06T18:03:35Z

static/js/ace-editor.ts

+    // TODO: FIX THIS
+    initializeDebugger({
+      editor: this._editor,
+      markers: this.markers,
+      level: theLevel,
+      language: theLanguage,


Yeah, it's interesting to think about whether the debugger should be inside the editor, or outside it.

You could say that the debugger is external to the editor, just using some features of the editor like "putting markers in the gutter" and "striking through certain lines".

But no need to change that now. You can always do that later, once you start adding a new editor implementation and you notice that there's a lot of repeated code between them 😉

Yes, exactly, this is a weird one. Because yeah, I can say that the debugger is part of an editor so it should be part of this interface. On the other hand, it'll probably have to deal with HTML and DOM objects, and we're trying to avoid to include those things in this interface.

However, I think that will become clearer once I start working on including code mirror and replacing the current debugger.

…space function

jpelay · 2023-09-07T21:51:07Z

I found that we can't remove the trimWhiteSpaceFunction since if we trim the contents of the editor every time you get the contents, you can't add spaces to the end of the line when editing.

Felienne · 2023-09-13T13:24:20Z

OK, let's test this extensively on alpha before deploying :) @jpelay can you let me know when you are sure we can deploy? Maybe also test the new dashboard a bit (less core, so less important)

mergify · 2023-09-13T13:24:28Z

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-09-13T13:34:13Z

OK, let's test this extensively on alpha before deploying :) @jpelay can you let me know when you are sure we can deploy? Maybe also test the new dashboard a bit (less core, so less important)

Yes, I will! And since the tests that we made for the editor that we remove earlier today work locally, I'll use them to test these changes in my computer. I'll let you know when I think everything's been tested extensively!

mergify · 2023-09-13T14:13:29Z

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).

change properties to functions

fe22b9d

jpelay marked this pull request as draft August 28, 2023 18:38

jpelay added 7 commits August 29, 2023 11:36

changing getvalue method to property

722204e

adding isReadOnly property

5003613

remove trim trailing white space from interface

140e261

correct spelling

ff4bf73

Update app.ts

7b09385

add constructor to class

e8c6e75

adding editor types

8685728

jpelay added 3 commits September 4, 2023 14:10

indentation

646be8e

remove property and method from generic interface

b788e6e

make hedy editor an event emitter

88ced94

jpelay commented Sep 6, 2023

View reviewed changes

Merge branch 'main' into improve-interface

e87eb15

jpelay marked this pull request as ready for review September 6, 2023 16:08

jpelay requested a review from rix0rrr September 6, 2023 16:14

Delete editor-markers.ts

a4d6c85

rix0rrr reviewed Sep 6, 2023

View reviewed changes

jpelay added 7 commits September 6, 2023 15:39

addint types definiton to on function of editor interface

a20235d

updating comment

683f54c

passing editor as parameter in the attachMainEditor function

9eca1af

change function name

2088465

remove unncesary if

ef23c71

uses editor to handle read mode

515f2d7

undo changes to messages

be5b01d

jpelay mentioned this pull request Sep 7, 2023

[UI] Add CodeMirror to Hedy #4464

Closed

4 tasks

deleted reference to window.Range and reinstaurated trimTrailingwhite…

1d38739

…space function

jpelay added 4 commits September 8, 2023 17:57

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

10e0da5

fix ask variable

a9e379f

javascript files

5f86852

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

0165ffb

Felienne approved these changes Sep 13, 2023

View reviewed changes

Merge branch 'main' into improve-interface

58719e6

mergify bot merged commit 0a54e7c into hedyorg:main Sep 13, 2023
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REFACTORING] Improve Hedy Editor interface #4435

[REFACTORING] Improve Hedy Editor interface #4435

jpelay commented Aug 28, 2023 •

edited

Loading

jpelay commented Sep 4, 2023

jpelay Sep 6, 2023

rix0rrr Sep 6, 2023

jpelay Sep 6, 2023

jpelay commented Sep 6, 2023

rix0rrr left a comment

rix0rrr Sep 6, 2023

jpelay Sep 6, 2023

rix0rrr Sep 7, 2023

jpelay Sep 7, 2023

rix0rrr Sep 6, 2023

rix0rrr Sep 6, 2023

jpelay Sep 6, 2023

rix0rrr Sep 6, 2023

jpelay Sep 6, 2023

rix0rrr Sep 6, 2023

rix0rrr Sep 6, 2023

jpelay Sep 6, 2023

rix0rrr Sep 6, 2023

jpelay Sep 6, 2023

rix0rrr Sep 6, 2023

rix0rrr Sep 6, 2023

rix0rrr Sep 6, 2023

jpelay Sep 6, 2023

rix0rrr Sep 6, 2023

jpelay Sep 6, 2023

jpelay commented Sep 7, 2023

Felienne commented Sep 13, 2023

mergify bot commented Sep 13, 2023

jpelay commented Sep 13, 2023

mergify bot commented Sep 13, 2023

		"adventures_restored": "Die Standardabenteuer wurden wiederhergestellt.",
		"adventures_restored": "The default adventures have been restored!",

[REFACTORING] Improve Hedy Editor interface #4435

[REFACTORING] Improve Hedy Editor interface #4435

Conversation

jpelay commented Aug 28, 2023 • edited Loading

jpelay commented Sep 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpelay commented Sep 6, 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

jpelay commented Sep 7, 2023

Felienne commented Sep 13, 2023

mergify bot commented Sep 13, 2023

jpelay commented Sep 13, 2023

mergify bot commented Sep 13, 2023

jpelay commented Aug 28, 2023 •

edited

Loading