Upgrade to CodeMirror 6 #2058

grolu · 2024-08-30T15:49:10Z

What this PR does / why we need it:
Upgrade CodeMirror 5 to CodeMirror 6

Which issue(s) this PR fixes:
Fixes #1892

Special notes for your reviewer:

Release note:

Upgraded the code editor from CodeMirror 5 to CodeMirror 6 to enhance performance, modernize the interface, and improve extensibility

- upgrade to CM6 - migrated editor line highlighter composable TODO - code completion - test migration

Removed obsolete CSS

gardener-robot · 2024-09-06T14:02:02Z

@grolu You need rebase this pull request with latest master branch. Please check.

- Restructured main scss theme dependant classes

Make editor always consume 100% height Cleaned-up some comments

…m:gardener/dashboard into enh/upgrade_cm_6

petersutter · 2024-09-19T12:25:13Z

frontend/src/composables/useEditorLineHighlighter.js

+      let [startLine, endLine = startLine] = values
+      const [startLineCurrent, endLineCurrent] = valuesCurrent
+
+      if (endLine === startLine && startLine === startLineCurrent && !endLineCurrent) {


I had similar code in my PR where I introduce the feature to clear the selected line if the current selected line is clicked again, however holger propsed to use ESC-key instead to clear the selection. Clearing with ESC-key currently does not work anymore

I know. The problem here is that I did not find a way to add a key listener to the gutter. It is possible to add a key listener to the editor itself and clear the highlight on escape. However, the editor needs to be focused, so you would need to click inside the editor to make this work.
So I checked how Github does the highlighting and it does not clear the selection on escape, it works exactly like I implemented it now.

frontend/src/composables/useEditorLineHighlighter.js

frontend/src/composables/useShootEditor/helper.js

petersutter · 2024-09-19T14:06:35Z

frontend/src/composables/useShootEditor/helper.js

+        label: string,
+        displayLabel: text,
+        detail: typeText,
+        type: 'keyword', // TODO check weather we want to show diffrent icons


is there a list of icons / types to choose from?

https://codemirror.net/docs/ref/#autocomplete.Completion.type
It is also possible to omit the type property so that no icon is rendered. AFAIK it is not possible to provide a custom render function for the completion box (like we did for CM5), only for the content that is shown next to it, the info (details) it is possible to provide a render function

let's leave it like it is for now (and remove the TODO), I do not have a better suggestion

petersutter · 2024-09-19T14:36:11Z

frontend/src/composables/useShootEditor/helper.js


-    return { lineTokens, lineString }
+    return token


not part of this PR, but the hasOnlyTypeOrTypeAndFormatProperties function needs an overhaul

const hasOnlyTypeOrTypeAndFormatProperties = value => { if (value.every(item => { return (Object.keys(item).length === 1 && item.type) || (Object.keys(item).length === 2 && item.type && item.format) })) { return true } return false }

How about refactoring it to either

const hasValidKeys = value => { return value.every(item => { const keys = Object.keys(item) const hasType = 'type' in item const hasFormat = 'format' in item return (keys.length === 1 && hasType) || (keys.length === 2 && hasType && hasFormat) }) }

or

const hasValidKeys = value => { const validKeyCombinations = [ ['type'], ['type', 'format'] ] return value.every(item => { const keys = Object.keys(item) return validKeyCombinations.some(combination => combination.length === keys.length && combination.every(key => keys.includes(key)) ) }) }

do you want to change it in this PR anyway?

frontend/src/composables/useShootEditor/index.js

# Conflicts: # .pnp.cjs # .yarn/cache/codemirror-npm-5.65.18-debbbac10b-806e00c708.zip # frontend/src/composables/useShootEditor/helper.js # frontend/src/composables/useShootEditor/index.js # yarn.lock

petersutter

minor things, otherwise /lgtm

petersutter · 2024-10-14T14:47:50Z

frontend/src/composables/useShootEditor/helper.js

@@ -83,9 +90,9 @@ export class EditorCompletions {
        end,
        string,
      }
-      if (token.start <= pos.ch && pos.ch <= token.end) {
+      if (token.start <= lineChar && lineChar <= token.end) {
        // Ensure that mouse pointer is on propety and not somewhere else on this line


Suggested change

// Ensure that mouse pointer is on propety and not somewhere else on this line

// Ensure that mouse pointer is on property and not somewhere else on this line

petersutter · 2024-10-14T14:48:59Z

frontend/src/composables/useShootEditor/helper.js

        // Ensure that mouse pointer is on propety and not somewhere else on this line
-        const completions = this._getYamlCompletions(token, pos, cm, true)
+        const completions = this._getYamlCompletions(token, { ch: lineChar, line }, cmView, true)


extract to variable for better readability

Suggested change

const completions = this._getYamlCompletions(token, { ch: lineChar, line }, cmView, true)

const cur = { ch: lineChar, line }

const completions = this._getYamlCompletions(token, cur, cmView, true)

What is cur? Is it current? Or is it cursor? Use easy understable names.

petersutter · 2024-10-14T14:51:19Z

frontend/src/composables/useShootEditor/helper.js


-    return { lineTokens, lineString }
+    return token


do you want to change it in this PR anyway?

holgerkoser · 2024-10-15T13:44:35Z

frontend/src/components/GShootEditor.vue

+              <div v-bind="slotProps.props">
+                <g-action-button
+                  size="x-small"
+                  :icon="renderWhitespaces ? 'mdi-grid' : 'mdi-grid-off'"


These icons are not intuitive to me. I would use these ones. We should use Hide and Show (not Render). And I think we can delete the tab icon in assets?

<v-tooltip location="top"> <template #activator="slotProps"> <div v-bind="slotProps.props"> <v-btn variant="text" size="x-small" flat icon color="black" @click="renderWhitespaces = !renderWhitespaces" > <span v-html="renderWhitespaces ? whitespaceEye : whitespaceEyeOff" style="width: 18px; height: 18px"/> </v-btn> </div> </template> <span>{{ renderWhitespaces ? 'Hide' : 'Render' }} whitespaces</span> </v-tooltip>

holgerkoser · 2024-10-15T15:31:09Z

frontend/src/composables/useEditorLineHighlighter.js

+      }
+      return highlights
+    },
+    provide: f => EditorView.decorations.from(f),


Suggested change

provide: f => EditorView.decorations.from(f),

provide (f) {

return EditorView.decorations.from(f)

},

holgerkoser · 2024-10-15T15:35:34Z

frontend/src/composables/useEditorWhitespace.js

+export function useEditorWhitespace () {
+  const matchDecorator = new MatchDecorator({
+    regexp: /[ \t]/g,
+    decoration: match => {


Suggested change

decoration: match => {

decoration (match) {

holgerkoser · 2024-10-15T15:36:03Z

frontend/src/composables/useEditorWhitespace.js

+        return Decoration.mark({
+          attributes: { class: 'g-cm-highlightedTab' },
+        })
+      } else if (match[0] === ' ') {


Suggested change

} else if (match[0] === ' ') {

}

if (match[0] === ' ') {

holgerkoser · 2024-10-15T15:38:57Z

frontend/src/composables/useShootEditor/helper.js

        // Ensure that mouse pointer is on propety and not somewhere else on this line
-        const completions = this._getYamlCompletions(token, pos, cm, true)
+        const completions = this._getYamlCompletions(token, { ch: lineChar, line }, cmView, true)


What is cur? Is it current? Or is it cursor? Use easy understable names.

holgerkoser · 2024-10-15T15:41:14Z

frontend/src/composables/useShootEditor/helper.js

    return completionArray
  }

  // get token at cursor position in editor with yaml content
-  _getYamlToken (cm, cur) {
+  _getYamlToken (cmView, cur) {


These methods starting with underscore should be private and the underscore removed.

holgerkoser · 2024-10-15T15:46:21Z

frontend/src/sass/main.scss

+.v-theme-- {
+  @mixin alternate-row-background($background-color) {
+    .alternate-row-background > div:nth-of-type(even) {
+      background-color: $background-color;
+    }
+  }
+
+  @mixin g-highlighted($background-color, $border-color) {
+    .g-highlighted {
+      position: relative;
+      background-color: $background-color;
+      /* Left border */
+      box-shadow: inset 1px 0 0 0 $border-color;
+
+      /* Top border */
+      &.g-highlighted--top::before {
+        content: '';
+        position: absolute;
+        top: 0;
+        left: 0;
+        right: 0;
+        height: 1px; /* Adjust border thickness as needed */
+        background-color: $border-color;
+      }
+
+      /* Bottom border */
+      &.g-highlighted--bottom::after {
+        content: '';
+        position: absolute;
+        bottom: 0;
+        left: 0;
+        right: 0;
+        height: 1px; /* Adjust border thickness as needed */
+        background-color: $border-color;
+      }
+    }
+  }
+
+  @mixin g-cm-highlighted-whitespace($whitespace-color) {
+    $whitespace-color-encoded: #{'%23' + str-slice(inspect($whitespace-color), 2)};
+
+    .g-cm-highlightedSpace {
+      background-image: url('data:image/svg+xml,<svg xmlns="http://www.w3.org/2000/svg" width="6" height="6"><circle cx="3" cy="3" r="1" fill="#{$whitespace-color-encoded}" /></svg>');
+      background-repeat: no-repeat;
+      background-position: center;
+      background-size: contain;
+    }
+
+    .g-cm-highlightedTab {
+      background-image: url('data:image/svg+xml,<svg xmlns="http://www.w3.org/2000/svg" width="10" height="6"><path d="M0 3 L10 3" stroke="#{$whitespace-color-encoded}" stroke-width="1" /></svg>');
+      background-repeat: no-repeat;
+      background-position: center;
+      background-size: contain;
+    }
+
+    .g-cm-newline-marker {
+      color: $whitespace-color;
+    }
+  }


Suggested change

.v-theme-- {

@mixin alternate-row-background($background-color) {

.alternate-row-background > div:nth-of-type(even) {

background-color: $background-color;

}

}

@mixin g-highlighted($background-color, $border-color) {

.g-highlighted {

position: relative;

background-color: $background-color;

/* Left border */

box-shadow: inset 1px 0 0 0 $border-color;

/* Top border */

&.g-highlighted--top::before {

content: '';

position: absolute;

top: 0;

left: 0;

right: 0;

height: 1px; /* Adjust border thickness as needed */

background-color: $border-color;

}

/* Bottom border */

&.g-highlighted--bottom::after {

content: '';

position: absolute;

bottom: 0;

left: 0;

right: 0;

height: 1px; /* Adjust border thickness as needed */

background-color: $border-color;

}

}

}

@mixin g-cm-highlighted-whitespace($whitespace-color) {

$whitespace-color-encoded: #{'%23' + str-slice(inspect($whitespace-color), 2)};

.g-cm-highlightedSpace {

background-image: url('data:image/svg+xml,<svg xmlns="http://www.w3.org/2000/svg" width="6" height="6"><circle cx="3" cy="3" r="1" fill="#{$whitespace-color-encoded}" /></svg>');

background-repeat: no-repeat;

background-position: center;

background-size: contain;

}

.g-cm-highlightedTab {

background-image: url('data:image/svg+xml,<svg xmlns="http://www.w3.org/2000/svg" width="10" height="6"><path d="M0 3 L10 3" stroke="#{$whitespace-color-encoded}" stroke-width="1" /></svg>');

background-repeat: no-repeat;

background-position: center;

background-size: contain;

}

.g-cm-newline-marker {

color: $whitespace-color;

}

}

@mixin alternate-row-background($background-color) {

.alternate-row-background>div:nth-of-type(even) {

background-color: $background-color;

}

}

@mixin g-highlighted($background-color, $border-color) {

.g-highlighted {

position: relative;

background-color: $background-color;

/* Left border */

box-shadow: inset 1px 0 0 0 $border-color;

/* Top border */

&.g-highlighted--top::before {

content: '';

position: absolute;

top: 0;

left: 0;

right: 0;

height: 1px;

/* Adjust border thickness as needed */

background-color: $border-color;

}

/* Bottom border */

&.g-highlighted--bottom::after {

content: '';

position: absolute;

bottom: 0;

left: 0;

right: 0;

height: 1px;

/* Adjust border thickness as needed */

background-color: $border-color;

}

}

}

@mixin g-cm-highlighted-whitespace($whitespace-color) {

$whitespace-color-encoded: #{'%23' + str-slice(inspect($whitespace-color), 2)};

.g-cm-highlightedSpace {

background-image: url('data:image/svg+xml,<svg xmlns="http://www.w3.org/2000/svg" width="6" height="6"><circle cx="3" cy="3" r="1" fill="#{$whitespace-color-encoded}" /></svg>');

background-repeat: no-repeat;

background-position: center;

background-size: contain;

}

.g-cm-highlightedTab {

background-image: url('data:image/svg+xml,<svg xmlns="http://www.w3.org/2000/svg" width="10" height="6"><path d="M0 3 L10 3" stroke="#{$whitespace-color-encoded}" stroke-width="1" /></svg>');

background-repeat: no-repeat;

background-position: center;

background-size: contain;

}

.g-cm-newline-marker {

color: $whitespace-color;

}

}

.v-theme-- {

holgerkoser · 2024-10-15T15:48:15Z

frontend/src/views/GNewShootEditor.vue

+  }
+  return next()
+})
+onBeforeRouteUpdate(async (to, from, next) => {


What is the reason for this change. I think this has nothing to do with cm6. Isnt't this part of an ohter PR?

grolu added 2 commits August 28, 2024 16:19

First steps

3c28ca7

- upgrade to CM6 - migrated editor line highlighter composable TODO - code completion - test migration

Fixed code completion and tooltips

232c1c3

Removed obsolete CSS

gardener-robot added needs/review Needs review size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py) needs/second-opinion Needs second review by someone else labels Aug 30, 2024

gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 30, 2024

gardener-robot-ci-2 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Aug 30, 2024

Fixed Test Suite and Completion Errors [WIP]

9a16e96

gardener-robot added the needs/rebase Needs git rebase label Sep 6, 2024

gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Sep 6, 2024

Finished adapting completion logic and tests

f067c4e

gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Sep 9, 2024

Merge branch 'master' into enh/upgrade_cm_6

aa0192d

gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Sep 10, 2024

grolu force-pushed the enh/upgrade_cm_6 branch from 3d3f503 to aa0192d Compare September 10, 2024 14:11

gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Sep 10, 2024

- Added whitespace highlighting

fed8458

- Restructured main scss theme dependant classes

gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Sep 16, 2024

grolu added 2 commits September 16, 2024 18:25

Fixed scrolling on enter

5c8c6ca

Make editor always consume 100% height Cleaned-up some comments

Merge branch 'master' into enh/upgrade_cm_6

48d3839

gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Sep 16, 2024

grolu marked this pull request as ready for review September 16, 2024 16:26

grolu requested review from holgerkoser and petersutter as code owners September 16, 2024 16:26

grolu changed the title ~~[DRAFT] Enh/upgrade cm 6~~ Upgrade to CodeMirror 6 Sep 16, 2024

gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Sep 16, 2024

grolu added 2 commits September 16, 2024 18:38

Fixed editorEnter tests

494f21d

Merge branches 'enh/upgrade_cm_6' and 'enh/upgrade_cm_6' of github.co…

aac852c

…m:gardener/dashboard into enh/upgrade_cm_6

gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Sep 16, 2024

gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Sep 16, 2024

petersutter reviewed Sep 19, 2024

View reviewed changes

Merge branch 'master' into enh/upgrade_cm_6

d47a008

# Conflicts: # .pnp.cjs # .yarn/cache/codemirror-npm-5.65.18-debbbac10b-806e00c708.zip # frontend/src/composables/useShootEditor/helper.js # frontend/src/composables/useShootEditor/index.js # yarn.lock

gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Sep 24, 2024

gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Sep 24, 2024

grolu added 2 commits September 24, 2024 14:28

PR Feedback

ea56d85

Merge branch 'master' into enh/upgrade_cm_6

c92897e

gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Sep 24, 2024

gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Sep 24, 2024

grolu added the area/ipcei IPCEI (Important Project of Common European Interest) label Oct 14, 2024

grolu mentioned this pull request Oct 14, 2024

[IPCEI] UI Improvements #1903

Open

48 tasks

petersutter approved these changes Oct 14, 2024

View reviewed changes

holgerkoser reviewed Oct 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to CodeMirror 6 #2058

Upgrade to CodeMirror 6 #2058

grolu commented Aug 30, 2024 •

edited

Loading

gardener-robot commented Sep 6, 2024

petersutter Sep 19, 2024

grolu Sep 24, 2024

petersutter Sep 19, 2024

grolu Sep 24, 2024 •

edited

Loading

petersutter Oct 14, 2024 •

edited

Loading

petersutter Sep 19, 2024

petersutter Oct 14, 2024

petersutter left a comment

petersutter Oct 14, 2024

petersutter Oct 14, 2024

holgerkoser Oct 15, 2024

petersutter Oct 14, 2024

holgerkoser Oct 15, 2024

holgerkoser Oct 15, 2024

holgerkoser Oct 15, 2024

holgerkoser Oct 15, 2024

holgerkoser Oct 15, 2024

holgerkoser Oct 15, 2024

holgerkoser Oct 15, 2024

holgerkoser Oct 15, 2024

	// Ensure that mouse pointer is on propety and not somewhere else on this line
	// Ensure that mouse pointer is on property and not somewhere else on this line

	const completions = this._getYamlCompletions(token, { ch: lineChar, line }, cmView, true)
	const cur = { ch: lineChar, line }
	const completions = this._getYamlCompletions(token, cur, cmView, true)

-    provide: f => EditorView.decorations.from(f),
+    provide (f) {
+      return EditorView.decorations.from(f)
+    },

Upgrade to CodeMirror 6 #2058

Are you sure you want to change the base?

Upgrade to CodeMirror 6 #2058

Conversation

grolu commented Aug 30, 2024 • edited Loading

gardener-robot commented Sep 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grolu Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

petersutter Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petersutter 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

grolu commented Aug 30, 2024 •

edited

Loading

grolu Sep 24, 2024 •

edited

Loading

petersutter Oct 14, 2024 •

edited

Loading