🪲 Set highlighter language and level #6126

boryanagoncharenko · 2025-01-23T10:21:38Z

PR #6085 makes uses of data-level and data-kwlang but these are never set. As a result, the language of the highlighter always defaults to English regardless of the current language. This PR makes some minimal changes to address the issue.

Fixes #6112

How to test
As an anonymous user go to /hedy and change the language to Turkish (or any other language). In the editor paste yazdır test and check that the first word is highlighted as a command. Do the same check for the /tryit page and a logged in user.

mergify · 2025-01-23T11:00: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).

Oops

mergify · 2025-01-23T11:00:14Z

This pull request has been removed from the queue for the following reason: pull request dequeued.

Pull request #6126 has been dequeued. The pull request rule doesn't match anymore. The following conditions don't match anymore:

all of:
- #approved-reviews-by>=1
any of: [🔀 queue conditions]
- all of: [📌 queue conditions of queue default_queue]
  - #approved-reviews-by >= 1 [🛡 GitHub branch protection]

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

rix0rrr · 2025-01-23T11:45:42Z

static/js/cm-editor.ts

@@ -210,8 +210,8 @@ export class HedyCodeMirrorEditor implements HedyEditor {
            state: state
        });

-        const levelStr = $(element).closest('[data-level]').attr('data-level');
-        const lang = $(element).closest('[data-kwlang]').attr('data-kwlang') ?? 'en';
+        const levelStr = $('[data-level]').attr('data-level');


The old code tried to find the closest enclosing element that has a data-level attribute.

The new code finds any data-level attribute anywhere on the page, whether it has a relation to the editor or not.

Why did the old code not work?

There is no data-level or data-kwlang on any element. This is why it did not work. I added the attributes to the editor but then ofc closest does not work. We could either put the attributes on an enclosing element indeed which is a bit weird or keep them in the editor and fetch them directly from the editor $('#editor').attr('data-*'). What was your intention?

Putting the level and kwlang directly on the editor works for the main editor because we control the HTML for it, but doesn't work very well for example for all the syntax highlighted preview editors in running text, because those are <pre> tags that are generated from Markdown. So I wanted a place to put it where it's still in context, but not the editor itself, so it become "some enclosing <div>".

I'd like to keep the logic consistent between the read/write editor and the preview-only editors. That allows us to de-dupe this information wherever it makes sense (*).

I for sure though x.closest() would return x if it matched the selector, but perhaps it doesn't. The document seems to bear that out though, I recall looking this up:

https://api.jquery.com/closest/

(*) In general, I'd like to move to a future in which more data is stored in the HTML, via data-* attributes in strategic locations, than passed via JavaScript to initializeXxx() functions. The latter requires a very rigid initialization order that doesn't work very well with HTMX.

rix0rrr · 2025-01-23T11:46:10Z

templates/hedy-page/editor-and-output.html

@@ -15,7 +15,8 @@
      <!-- The actual editor -->
      <div id="editor" data-cy="editor" style="background: #272822; font-size:0.95em; color: white; direction: {{ g.dir }};" lang="en" blurred='false'
        {% if editor_readonly %}data-readonly="true"{% endif %}
-        class="w-full flex-1 text-lg rounded min-h-0 overflow-hidden fixed-editor-height"></div>
+        class="w-full flex-1 text-lg rounded min-h-0 overflow-hidden fixed-editor-height"
+           data-kwlang="{{ current_language().lang }}" data-level="{{ level }}"></div>


An enclosing scope should be good enough, and I think it should be on there already, no?

rix0rrr · 2025-01-23T11:46:34Z

templates/incl/editor-and-output.html

@@ -25,7 +25,8 @@
        <!-- The actual editor -->
        <div id="editor" style="background: #272822; font-size:0.95em; color: white; direction: {{ g.dir }};" lang="en" blurred='false'
          {% if editor_readonly %}data-readonly="true"{% endif %}
-          class="w-full flex-1 text-lg rounded min-h-0 overflow-hidden fixed-editor-height"></div>
+          class="w-full flex-1 text-lg rounded min-h-0 overflow-hidden fixed-editor-height"
+             data-kwlang="{{ current_language().lang }}" data-level="{{ level }}"></div>


Same here, if we can put this attribute on a <div> that contains all editors, it should work already?

rix0rrr · 2025-01-24T08:56:43Z

templates/layout.html

@@ -48,7 +48,8 @@
    <link rel="icon" type="image/png" href="{{static('/images/Hedy-logo-96x96-laptop.png')}}"/>
    <link rel="stylesheet" href="{{static('/fontawesome/css/all.min.css')}}"/>
  </head>
-  <body id="main_container" dir="{{ g.dir }}" class="{% if not raw %}bg-gray-100{% endif %}" hx-ext="disable-element,morph" data-logged-in="{{ 1 if username else 0 }}">
+  <body id="main_container" dir="{{ g.dir }}" class="{% if not raw %}bg-gray-100{% endif %}" hx-ext="disable-element,morph"
+        data-logged-in="{{ 1 if username else 0 }}" data-kwlang="{{ current_language().lang }}" data-level="{{ level }}">


Is the kwlang necessarily the same as the current display language?

And what about pages that don't have a level parameter? (Like, teacher's interface documentation? Or the explore page? Or the public adventures search page) It seems safer to scope this more strictly to the code page bits?

But also -- I'm not sure, I'm asking.

If the user is logged in and their profile language is set to Turkish but their keyword language is set to English, should we highlight Turkish keywords if they are used? I think we should, so the language there should not be the g.keyword_lang, it should be the session language.

I am not sure how to resolve this with given the constraints. I put the data attributes in the main container because this is literally the common div that encompasses all usages of editor and editor-like elements. However, as you pointed out it does not make sense to use a level there, which is what I meant by weird in my previous comment.

I placed it now on the editor_area which is your initial idea probably. However, this does not work as expected. I get intermittent failures to load the highlighting correctly which look like this:

Converted this to a draft, so that it does not get merged until this is solved.

mergify · 2025-01-24T11:39:33Z

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

…ll relevant elements

rix0rrr · 2025-01-24T12:05:43Z

I ended up putting them in level-page.html, which contains a <div> that is a shared common ancestor of the editor and the adventure tabs with preview text. It seems to work.

I did change the source back to g.keyword_lang though.

If the user is logged in and their profile language is set to Turkish but their keyword language is set to English, should we highlight Turkish keywords if they are used? I think we should, so the language there should not be the g.keyword_lang, it should be the session language.

Well... if not for that, then what is keyword_lang even for?

It may be that there is no useful distinction between keyword language and interface language; but if that's the case, I think I'd rather do that by setting g.keyword_lang = g.lang somewhere in the Python, rather than having a mix of language interpretations in the HTML templates.

There also seems to be a bunch of swizzling of HTML attributes in JavaScript, where we set $(element).attr('lang', someValue). I changed all these to data-lang as well... but preferably they all shouldn't be necessary anymore. This kind of set-x-to-y-before-calling-z code is bound to creep full of errors over time as it is being changed and no one has a full picture of what data goes where, when, anymore.

(I don't want to put in the effort to validate which bits can be removed though...)

rix0rrr · 2025-01-24T12:08:48Z

Current status:

mergify · 2025-01-24T12:27:21Z

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

boryanagoncharenko · 2025-01-27T09:05:44Z

I am afraid using the level-page.html does not cover all scenarios. The following screenshots are done with Teacher1 when the language and the keyword language are both set to Bulgarian:

Viewing a public program:

Previewing a custom adventure:

I think the common denominator is the layout.html which is too wide of a context.

rix0rrr · 2025-01-27T09:07:45Z

Do we have a list of places where common UI elements are used? I feel like this "not catching of all cases" comes up all the time... ☹️

boryanagoncharenko · 2025-01-27T11:53:56Z

Do we have a list of places where common UI elements are used? I feel like this "not catching of all cases" comes up all the time... ☹️

I know :(

I traced that the editor-and-output.html is used in the following places:

the code-page which uses layout.html > level-page.html. Btw the code-page can be tested via /hedy, /tryit, /slides/{level_id}, /hour-of-code. The rest of the usages will be removed soon.
the customize-adventure page which uses layout.html > auth.html
the view-program page which uses directly layout.html
the public-adventures page (specifically the htmx-preview-adventure.html) which uses layout.html > auth.html

But I noticed we also have an embedded-editor.html which is returned by embedded/{level_id}, which I forgot to check at all.

Btw I think the keyword language is meant to define in which language to display the keywords in the program at hand. The highlighting, however, should work for the current language and English. I base this assumption on the way the grammars of the language (the lark files, not the highlighting grammars) are created.

rix0rrr · 2025-01-27T12:01:09Z

Thanks, that list is extremely helpful!

Btw I think the keyword language is meant to define in which language to display the keywords in the program at hand. The highlighting, however, should work for the current language and English.

I think highlighting will automatically work for <language> + English anyway... ?

But that brings up a good point: let's say keyword language is set to German and display language is set to Dutch (for whatever reason). Keywords should be rendered in German if we do that, right? Shouldn't we then highlight "German + English" (i.e., the keyword language) rather than "Dutch + English" (the UI language) ?

boryanagoncharenko · 2025-01-27T12:06:34Z

But that brings up a good point: let's say keyword language is set to German and display language is set to Dutch (for whatever reason). Keywords should be rendered in German if we do that, right? Shouldn't we then highlight "German + English" (i.e., the keyword language) rather than "Dutch + English" (the UI language) ?

In your profile, if you set a language other than English, you can only choose your keyword language to be either that same language or English. So, if my language is Turkish, I can pick to have as a keyword language either English or Turkish. I think this was done to simplify the problem of grammar generation.

boryanagoncharenko · 2025-01-27T12:09:26Z

However, I have to point out that we did explore the idea of having multiple languages at the same time, e.g. Dutch, Frisian and English, or Dutch, French and English. This would work for countries with multiple official languages. It sounds like a very nice feature but it got de-prioritized.

rix0rrr · 2025-01-27T12:25:25Z

So you are saying that the highlighting grammar based on the display language is going to be a union of all possible keyword languages for that display language?

That's good to know... but for clarity's sake I think I would still prefer a different keyword then. How about:

g.render_keyword_lang
g.highlight_keyword_lang

? Or something to that effect?

(Because effectively we are talking about them being different)

rix0rrr · 2025-01-28T20:28:36Z

I added the attributes in other missing places. It's not elegant, but it'll do... ? We can start paying more attention in the future.

This reverts commit 4801cbb.

…ing_6112

…to broken_highlighting_6112

This reverts commit 8092637.

This reverts commit 953421c.

…ing_6112

mergify · 2025-01-30T10:13:07Z

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

boryanagoncharenko requested a review from rix0rrr January 23, 2025 10:21

boryanagoncharenko changed the title ~~🪲 Set highliter language and level~~ 🪲 Set highlighter language and level Jan 23, 2025

rix0rrr previously approved these changes Jan 23, 2025

View reviewed changes

rix0rrr requested changes Jan 23, 2025

View reviewed changes

boryanagoncharenko and others added 3 commits January 24, 2025 10:53

Set highliter language and level #6112

2d792de

🤖 Automatically update generated files

b308bd8

Address feedback #6112

2eee2db

boryanagoncharenko force-pushed the broken_highlighting_6112 branch from 6a32e44 to 2eee2db Compare January 24, 2025 08:54

🤖 Automatically update generated files

02eb81e

rix0rrr reviewed Jan 24, 2025

View reviewed changes

address feedback

519ecbd

rix0rrr approved these changes Jan 24, 2025

View reviewed changes

boryanagoncharenko marked this pull request as draft January 24, 2025 11:22

Putting the kwlang and level tags on the closest shared ancestor of a…

2206427

…ll relevant elements

Add data-level/data-kwlang in more places where it is necessary

3053e8e

rix0rrr and others added 9 commits January 28, 2025 20:29

🤖 Automatically update generated files

953421c

Fix a typo

4801cbb

🤖 Automatically update generated files

8092637

Revert "Fix a typo"

b98ec33

This reverts commit 4801cbb.

Merge branch 'main' of github.com:Felienne/hedy into broken_highlight…

83feaac

…ing_6112

Use correct comment delimiter

8a3dbf9

🤖 Automatically update generated files

3518c4a

Merge branch 'broken_highlighting_6112' of github.com:hedyorg/hedy in…

b796cd6

…to broken_highlighting_6112

Merge branch 'broken_highlighting_6112' of github.com:hedyorg/hedy in…

b0e8ad3

…to broken_highlighting_6112

boryanagoncharenko marked this pull request as ready for review January 29, 2025 13:14

boryanagoncharenko and others added 6 commits January 29, 2025 15:53

Revert "🤖 Automatically update generated files"

155f095

This reverts commit 8092637.

Revert "🤖 Automatically update generated files"

58c4b38

This reverts commit 953421c.

Remove a console.log

5a5ce21

🤖 Automatically update generated files

074ba3b

Merge branch 'main' of github.com:Felienne/hedy into broken_highlight…

dc1f663

…ing_6112

Default adventure's level to empty string instead of 'None'

19109ad

mergify bot merged commit 063e62a into main Jan 30, 2025
11 checks passed

mergify bot deleted the broken_highlighting_6112 branch January 30, 2025 10:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🪲 Set highlighter language and level #6126

🪲 Set highlighter language and level #6126

boryanagoncharenko commented Jan 23, 2025

mergify bot commented Jan 23, 2025

mergify bot commented Jan 23, 2025

rix0rrr Jan 23, 2025

boryanagoncharenko Jan 23, 2025

rix0rrr Jan 23, 2025 •

edited

Loading

rix0rrr Jan 23, 2025

rix0rrr Jan 23, 2025

rix0rrr Jan 24, 2025

boryanagoncharenko Jan 24, 2025

mergify bot commented Jan 24, 2025

rix0rrr commented Jan 24, 2025 •

edited

Loading

rix0rrr commented Jan 24, 2025

mergify bot commented Jan 24, 2025

boryanagoncharenko commented Jan 27, 2025

rix0rrr commented Jan 27, 2025

boryanagoncharenko commented Jan 27, 2025

rix0rrr commented Jan 27, 2025 •

edited

Loading

boryanagoncharenko commented Jan 27, 2025

boryanagoncharenko commented Jan 27, 2025

rix0rrr commented Jan 27, 2025 •

edited

Loading

rix0rrr commented Jan 28, 2025

mergify bot commented Jan 30, 2025

🪲 Set highlighter language and level #6126

🪲 Set highlighter language and level #6126

Conversation

boryanagoncharenko commented Jan 23, 2025

mergify bot commented Jan 23, 2025

mergify bot commented Jan 23, 2025

rix0rrr Jan 23, 2025

Choose a reason for hiding this comment

boryanagoncharenko Jan 23, 2025

Choose a reason for hiding this comment

rix0rrr Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

rix0rrr Jan 23, 2025

Choose a reason for hiding this comment

rix0rrr Jan 23, 2025

Choose a reason for hiding this comment

rix0rrr Jan 24, 2025

Choose a reason for hiding this comment

boryanagoncharenko Jan 24, 2025

Choose a reason for hiding this comment

mergify bot commented Jan 24, 2025

rix0rrr commented Jan 24, 2025 • edited Loading

rix0rrr commented Jan 24, 2025

mergify bot commented Jan 24, 2025

boryanagoncharenko commented Jan 27, 2025

rix0rrr commented Jan 27, 2025

boryanagoncharenko commented Jan 27, 2025

rix0rrr commented Jan 27, 2025 • edited Loading

boryanagoncharenko commented Jan 27, 2025

boryanagoncharenko commented Jan 27, 2025

rix0rrr commented Jan 27, 2025 • edited Loading

rix0rrr commented Jan 28, 2025

mergify bot commented Jan 30, 2025

rix0rrr Jan 23, 2025 •

edited

Loading

rix0rrr commented Jan 24, 2025 •

edited

Loading

rix0rrr commented Jan 27, 2025 •

edited

Loading

rix0rrr commented Jan 27, 2025 •

edited

Loading