[BUG] fix (tab) indentation within CodeMirror #4832

hasan-sh · 2023-11-30T14:35:51Z

How to test?

go to code editor
try indenting with tab (tip: shift-tab removes indentation as well!)
and within blocks; e.g., now when you click Enter within a block (i.e., if or for), the correct indentation will be chosen based on the previous and next lines.

for i in range 1 to 5 # (CLICK ENTER)
    # CURSOR_STARTSHERE
    print i 

or 

for i in range 1 to 5
    print i #  (CLICK ENTER)
    # CURSOR_STARTS_HERE

- indent within blocks; according to previous and next lines.

ghost · 2023-11-30T14:36:57Z

👇 Click on the image for a new way to code review

Legend

jpelay

Hello Hasan, what a nice and clever solution! I have a couple of comments:

With respect to indent with tab, we decided that it was better to be as accessible as possible and not trapping users in the editor on the case that they don't use pointing devices.

With respect to the approach, this is clever! But I think we can do a bit better. We actually can automatically add indentation when the CodeMirror parser detects a certain type of command, i.e: if the user writes: repeat 3 times and hits enter, the next line will be indented, so, no need to even indent manually!

This would be a little trickier to implement, since it involves messing with the grammars and the tokenizer, but here is a simple example of how this can be done: https://lezer.codemirror.net/examples/indent/ and you can try this by going here: https://codemirror.net/try/ and changing the code to this:

import {basicSetup, EditorView} from "codemirror"
import {python} from "@codemirror/lang-python"

new EditorView({
  doc: "print('hello')\n",
  extensions: [basicSetup, python()],
  parent: document.body
})

Then you can mess around with the indentation.

Do think you could do that approach? If not, we can always ship your solution and make this improvement later on!!! This is already a lot better over what we have 😄

jpelay · 2023-11-30T15:35:41Z

static/js/cm-editor.ts

@@ -117,7 +121,17 @@ export class HedyCodeMirrorEditor implements HedyEditor {
                    ...defaultKeymap,
                    ...searchKeymap, // we need to replace this with our own search widget
                    ...historyKeymap,
+                    indentWithTab,


It was a explicit decision not to use this. Felienne and I discussed it when making the change, and we ultimetaly decided to leave the default behaviour due to accessibility concerns that the CodeMirror maintainer expresses here: https://codemirror.net/examples/tab/

See this #4830 (comment)

If we do this, then we need to mention somewhere that this escape hatch is possible! Perhaps on the tutorial?

I think users will know that naturally, but I do agree, that would be the perfect spot to include it.

I'd be inclined to agree if our users were computer literates, but since our age group is around 12, I'm not sure they'd know this. We can also add it in the Teacher Manual in a future PR and merge this, what do you think?

jpelay · 2023-11-30T15:43:08Z

static/js/cm-editor.ts

+                    const previousLine = context.lineAt(pos, -1);
+                    const nextLine = context.lineAt(pos + 1, -1);
+                    const nextIndentationSize =  nextLine.text.match(/^\s*/)![0].length;
+                    const prevIndentationSize =  previousLine.text.match(/^\s*/)![0].length;
+                    const indentBy = Math.max(prevIndentationSize, nextIndentationSize);
+                    return indentBy
+                }),


When there's no next line, this returns an error. Also, can you add this code to its own function?

Good one, fixed!

hasan-sh · 2023-12-04T10:30:15Z

Hello Hasan, what a nice and clever solution! I have a couple of comments:

With respect to indent with tab, we decided that it was better to be as accessible as possible and not trapping users in the editor on the case that they don't use pointing devices.

With respect to the approach, this is clever! But I think we can do a bit better. We actually can automatically add indentation when the CodeMirror parser detects a certain type of command, i.e: if the user writes: repeat 3 times and hits enter, the next line will be indented, so, no need to even indent manually!

This would be a little trickier to implement, since it involves messing with the grammars and the tokenizer, but here is a simple example of how this can be done: https://lezer.codemirror.net/examples/indent/ and you can try this by going here: https://codemirror.net/try/ and changing the code to this:
import {basicSetup, EditorView} from "codemirror"
import {python} from "@codemirror/lang-python"

new EditorView({
  doc: "print('hello')\n",
  extensions: [basicSetup, python()],
  parent: document.body
})
Then you can mess around with the indentation.

Do think you could do that approach? If not, we can always ship your solution and make this improvement later on!!! This is already a lot better over what we have 😄

I do believe this would be far better indeed and more accurate. The difference though is that Hedy isn't python, so we have to kinda translate all grammars we have and apply indentation accrodingly. Perhaps we ship this and discuss in our next meeting when/how we wanna do this?

Felienne · 2023-12-07T15:12:12Z

Thanks for the detailed discussion here and in the issue on accessibility!

I am not sure, reading the code, if this PR already adds the "press escape to exit the editor" hatch? If not, I think I would like to add it here so we are compliant with the rule of:

"if it requires more than unmodified arrow or tab keys or other standard exit methods" (assuming escape key is a standard exit methods, I could not find a definition in the document)

Also, let's indeed add this to the manual (for teachers) and docs (for developers). Can you pick that up @hasan-sh?

Felienne · 2023-12-07T15:37:36Z

and within blocks; e.g., now when you click Enter within a block (i.e., if or for), the correct indentation will be chosen based on the previous and next lines.

Well, you say correct, but you mean the indentation of the line before, I think. Let's discuss this in the next meeting, I think there might be some learning value in doing it explicitly (esp in earlier levels) but we can totally merge this as this is what we had in Ace too!

hasan-sh · 2023-12-08T14:32:10Z

Also, let's indeed add this to the manual (for teachers) and docs (for developers). Can you pick that up @hasan-sh?

Sure, will do in this PR.

and within blocks; e.g., now when you click Enter within a block (i.e., if or for), the correct indentation will be chosen based on the previous and next lines.

Well, you say correct, but you mean the indentation of the line before, I think. Let's discuss this in the next meeting, I think there might be some learning value in doing it explicitly (esp in earlier levels) but we can totally merge this as this is what we had in Ace too!

Yeah the current line or the next line indeed. So, this is really not considering the commands, but only the indentation of the current and next line w.r.t. where the cursor is.

Felienne · 2023-12-08T14:54:43Z

Sure, will do in this PR.

Thanks! Ping me if that is ready so I can merge!

So, this is really not considering the commands, but only the indentation of the current and next line w.r.t. where the cursor is.

No problem, that is what we have now too. We can discuss in the meeting how/if we want to make it smarter

jpelay · 2023-12-12T14:29:40Z

I am not sure, reading the code, if this PR already adds the "press escape to exit the editor" hatch?

Yes it does! Hasan added it by applying an extension from CodeMirror that adds this as well.

Felienne · 2023-12-12T14:42:26Z

I am not sure, reading the code, if this PR already adds the "press escape to exit the editor" hatch?

Yes it does! Hasan added it by applying an extension from CodeMirror that adds this as well.

In that case we can merge this right away!

Ready!

mergify · 2023-12-12T14:43:15Z

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

mergify · 2023-12-12T15:04:13Z

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

mergify · 2023-12-13T11:51:12Z

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

hasan-sh added 2 commits November 30, 2023 15:29

- tab indent by 4

4cc809b

- indent within blocks; according to previous and next lines.

unused variables

2dbe626

hasan-sh requested a review from jpelay November 30, 2023 14:50

jpelay previously requested changes Nov 30, 2023

View reviewed changes

code enhancement

1ca714e

hasan-sh mentioned this pull request Dec 4, 2023

[UI] Hedy rules in CodeMirror #4845

Closed

Felienne assigned hasan-sh Dec 11, 2023

hasan-sh mentioned this pull request Dec 12, 2023

[CONTENT] Tab indentation and esc. key to unfocus from editor #4893

Open

Felienne approved these changes Dec 12, 2023

View reviewed changes

Merge branch 'main' into fix-indentation

f6715d4

Felienne and others added 3 commits December 13, 2023 07:15

Merge branch 'main' into fix-indentation

dbc128c

Merge branch 'main' into fix-indentation

ef8a872

Fix tests

5a71a89

mergify bot merged commit 4e01118 into main Dec 13, 2023
11 checks passed

mergify bot deleted the fix-indentation branch December 13, 2023 11:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BUG] fix (tab) indentation within CodeMirror #4832

[BUG] fix (tab) indentation within CodeMirror #4832

hasan-sh commented Nov 30, 2023

ghost commented Nov 30, 2023 •

edited by ghost

Loading

Legend

jpelay left a comment •

edited

Loading

jpelay Nov 30, 2023

hasan-sh Dec 4, 2023

jpelay Dec 4, 2023

hasan-sh Dec 4, 2023

jpelay Dec 4, 2023

hasan-sh Dec 5, 2023

jpelay Nov 30, 2023

hasan-sh Dec 4, 2023

hasan-sh commented Dec 4, 2023

Felienne commented Dec 7, 2023

Felienne commented Dec 7, 2023

hasan-sh commented Dec 8, 2023

Felienne commented Dec 8, 2023

jpelay commented Dec 12, 2023

Felienne commented Dec 12, 2023

mergify bot commented Dec 12, 2023

mergify bot commented Dec 12, 2023

mergify bot commented Dec 13, 2023

[BUG] fix (tab) indentation within CodeMirror #4832

[BUG] fix (tab) indentation within CodeMirror #4832

Conversation

hasan-sh commented Nov 30, 2023

ghost commented Nov 30, 2023 • edited by ghost Loading

Legend

jpelay left a comment • edited Loading

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

hasan-sh commented Dec 4, 2023

Felienne commented Dec 7, 2023

Felienne commented Dec 7, 2023

hasan-sh commented Dec 8, 2023

Felienne commented Dec 8, 2023

jpelay commented Dec 12, 2023

Felienne commented Dec 12, 2023

mergify bot commented Dec 12, 2023

mergify bot commented Dec 12, 2023

mergify bot commented Dec 13, 2023

ghost commented Nov 30, 2023 •

edited by ghost

Loading

jpelay left a comment •

edited

Loading