neanes / neanes

Neanes is a free and open source scorewriter for notating Byzantine chant in Byzantine notation.
https://neanes.github.io/neanes/
GNU General Public License v3.0
35 stars 9 forks source link

Upgrade Vue from 2.7.14 to 3.3.4 #142

Closed basil closed 11 months ago

basil commented 11 months ago

Fixes #117

At the moment I am aware of two issues with this branch. First, there is a need to adapt to the Vue 3 mount API changes as described in https://github.com/danielgarthur/neanes/issues/117#issuecomment-1610610696.

The second issue is that when running with

diff --git a/package.json b/package.json
index 0e097d2..54408bc 100644
--- a/package.json
+++ b/package.json
@@ -28,6 +28,7 @@
   "dependencies": {
     "@ckpack/vue-color": "^1.5.0",
     "@types/mime-types": "^2.1.1",
+    "@vue/compat": "^3.3.4",
     "html-to-image": "^1.11.11",
     "image-size": "^1.0.2",
     "jszip": "^3.10.1",
diff --git a/src/shims-vue-compat.d.ts b/src/shims-vue-compat.d.ts
new file mode 100644
index 0000000..b8acb3b
--- /dev/null
+++ b/src/shims-vue-compat.d.ts
@@ -0,0 +1,8 @@
+declare module 'vue' {
+  import { CompatVue } from '@vue/runtime-dom';
+  const Vue: CompatVue;
+  export default Vue;
+  export * from '@vue/runtime-dom';
+  const { configureCompat } = Vue;
+  export { configureCompat };
+}
diff --git a/vue.config.js b/vue.config.js
index 1f710be..6b50b82 100644
--- a/vue.config.js
+++ b/vue.config.js
@@ -4,6 +4,22 @@ module.exports = {
   configureWebpack: {
     devtool: 'source-map',
   },
+  chainWebpack: (config) => {
+    config.resolve.alias.set('vue', '@vue/compat');
+    config.module
+      .rule('vue')
+      .use('vue-loader')
+      .tap((options) => {
+        return {
+          ...options,
+          compilerOptions: {
+            compatConfig: {
+              MODE: 3,
+            },
+          },
+        };
+      });
+  },
   pluginOptions: {
     electronBuilder: {
       preload: 'src/preload.js',

I get

vue.runtime.esm-bundler.js:1458 [Vue warn]: (deprecation ATTR_ENUMERATED_COERCION) Enumerated attribute "draggable" with v-bind value `false` will render the value as-is instead of coercing the value to "true" in Vue 3. Always use explicit "true" or "false" values for enumerated attributes. If the usage is intended, you can disable the compat behavior and suppress this warning with:
  configureCompat({ ATTR_ENUMERATED_COERCION: false })
  Details: https://v3-migration.vuejs.org/breaking-changes/attribute-coercion.html 
  at <ToolbarMain entryMode="Auto" zoom=1 zoomToFit=false  ... > 
  at <Editor ipcService= IpcService platformService= PlatformService showFileMenuBar=false > 
  at <Home onVnodeUnmounted=fn<onVnodeUnmounted> ref=Ref< undefined > > 
  at <RouterView> 
  at <App>
warn @ vue.runtime.esm-bundler.js:1458

and

vue.runtime.esm-bundler.js:1458 [Vue warn]: (deprecation ATTR_ENUMERATED_COERCION) Enumerated attribute "contenteditable" with v-bind value `plaintext-only` will render the value as-is instead of coercing the value to "true" in Vue 3. Always use explicit "true" or "false" values for enumerated attributes. If the usage is intended, you can disable the compat behavior and suppress this warning with:
  configureCompat({ ATTR_ENUMERATED_COERCION: false })
  Details: https://v3-migration.vuejs.org/breaking-changes/attribute-coercion.html 
  at <ContentEditable ref="text" class="text-box" style= Object  ... > 
  at <TextBox ref_for=true ref="element-0" element= TextBoxElement  ... > 
  at <Editor ipcService= IpcService platformService= PlatformService showFileMenuBar=false > 
  at <Home onVnodeUnmounted=fn<onVnodeUnmounted> ref=Ref< Proxy > > 
  at <RouterView> 
  at <App>

I read the page on attribute coercion behavior but could not make heads or tails of it, even after working through the Vue tutorial.

I can get the warnings to go away with

diff --git a/src/components/ButtonWithMenu.vue b/src/components/ButtonWithMenu.vue
index c95dffa..887f118 100644
--- a/src/components/ButtonWithMenu.vue
+++ b/src/components/ButtonWithMenu.vue
@@ -5,7 +5,7 @@
     @mouseleave="selectedOption = null"
   >
     <button class="neume-button" :disabled="disabled">
-      <img draggable="false" :src="mainIcon" />
+      <img :src="mainIcon" />
     </button>
     <div class="menu" v-if="showMenu">
       <div
@@ -14,7 +14,7 @@
         class="menu-item"
         @mouseenter="selectedOption = option.neume"
       >
-        <img draggable="false" :src="option.icon" />
+        <img :src="option.icon" />
       </div>
     </div>
   </div>
diff --git a/src/components/ContentEditable.vue b/src/components/ContentEditable.vue
index 8b34a58..14fd137 100644
--- a/src/components/ContentEditable.vue
+++ b/src/components/ContentEditable.vue
@@ -22,11 +22,7 @@ export default class ContentEditable extends Vue {
   @Prop({ default: 'break-spaces' }) whiteSpace!: string;

   get contentEditable() {
-    return this.editable
-      ? this.plaintextOnly
-        ? 'plaintext-only'
-        : 'true'
-      : 'false';
+    return this.editable ? (this.plaintextOnly ? undefined : true) : false;
   }

   get htmlElement() {
diff --git a/src/components/ToolbarMain.vue b/src/components/ToolbarMain.vue
index 10a8a71..88c3da2 100644
--- a/src/components/ToolbarMain.vue
+++ b/src/components/ToolbarMain.vue
@@ -36,57 +36,57 @@
       @mouseleave="selectedTempoNeume = null"
     >
       <button class="neume-button">
-        <img draggable="false" src="@/assets/icons/agogi-poli-argi.svg" />
+        <img src="@/assets/icons/agogi-poli-argi.svg" />
       </button>
       <div class="tempo-menu" v-if="showTempoMenu">
         <div
           class="tempo-menu-item"
           @mouseenter="selectedTempoNeume = TempoSign.VerySlow"
         >
-          <img draggable="false" src="@/assets/icons/agogi-poli-argi.svg" />
+          <img src="@/assets/icons/agogi-poli-argi.svg" />
         </div>
         <div
           class="tempo-menu-item"
           @mouseenter="selectedTempoNeume = TempoSign.Slower"
         >
-          <img draggable="false" src="@/assets/icons/agogi-argoteri.svg" />
+          <img src="@/assets/icons/agogi-argoteri.svg" />
         </div>
         <div
           class="tempo-menu-item"
           @mouseenter="selectedTempoNeume = TempoSign.Slow"
         >
-          <img draggable="false" src="@/assets/icons/agogi-argi.svg" />
+          <img src="@/assets/icons/agogi-argi.svg" />
         </div>
         <div
           class="tempo-menu-item"
           @mouseenter="selectedTempoNeume = TempoSign.Moderate"
         >
-          <img draggable="false" src="@/assets/icons/agogi-metria.svg" />
+          <img src="@/assets/icons/agogi-metria.svg" />
         </div>
         <div
           class="tempo-menu-item"
           @mouseenter="selectedTempoNeume = TempoSign.Medium"
         >
-          <img draggable="false" src="@/assets/icons/agogi-mesi.svg" />
+          <img src="@/assets/icons/agogi-mesi.svg" />
         </div>
         <div
           class="tempo-menu-item"
           @mouseenter="selectedTempoNeume = TempoSign.Quick"
         >
-          <img draggable="false" src="@/assets/icons/agogi-gorgi.svg" />
+          <img src="@/assets/icons/agogi-gorgi.svg" />
         </div>

         <div
           class="tempo-menu-item"
           @mouseenter="selectedTempoNeume = TempoSign.Quicker"
         >
-          <img draggable="false" src="@/assets/icons/agogi-gorgoteri.svg" />
+          <img src="@/assets/icons/agogi-gorgoteri.svg" />
         </div>
         <div
           class="tempo-menu-item"
           @mouseenter="selectedTempoNeume = TempoSign.VeryQuick"
         >
-          <img draggable="false" src="@/assets/icons/agogi-poli-gorgi.svg" />
+          <img src="@/assets/icons/agogi-poli-gorgi.svg" />
         </div>
       </div>
     </div>
diff --git a/src/components/ToolbarNeume.vue b/src/components/ToolbarNeume.vue
index 2b30b4d..25dd237 100644
--- a/src/components/ToolbarNeume.vue
+++ b/src/components/ToolbarNeume.vue
@@ -168,7 +168,7 @@
         class="neume-button"
         @click="$emit('update:noteIndicator', !element.noteIndicator)"
       >
-        <img draggable="false" src="@/assets/icons/note-ni.svg" />
+        <img src="@/assets/icons/note-ni.svg" />
       </button>
       <ButtonWithMenu
         :options="isonMenuOptions"

but I am not sure if that is the correct fix.

danielgarthur commented 11 months ago

I think the draggable and contenteditable errors are false positives. Looking at the rendered HTML, the values are correctly set to the same values that they are in the Vue 2 app. I.e. draggable is false and content-editable can take on multiple values.

From what I can tell, it is just a warning because the behavior changed from Vue 2 to 3. I understand why they are warning us, since this could potentially break things. But in these specific cases, nothing will break.

basil commented 11 months ago

Great! At the moment I am not aware of any issues on this branch.

danielgarthur commented 11 months ago

I'm seeing some slower speeds with larger files (the test file I'm using is 8 pages). I'm not sure what the root cause is. I'll continue investigating.

For example, I can open a large file in the Vue 2 version and hold down J to enter a bunch of ison neumes without slow-down. But in Vue 3 that grinds everything to a halt. Quickly typing in lyrics is also laggy.

danielgarthur commented 11 months ago

What I am seeing is that in Vue 2, whenever I add a neume to the end of a document, only the single neume is rendered. The rest of the already-rendered neumes are left as it.

But in Vue 3, it always re-renders everything. There must be some subtle behavioral change that is causing this. Somehow I need to communicate to Vue 3 that most of the elements have remained the same.

Below are screenshots from Vue.js Devtools Performance Timeline.

Vue 2

Only the one neume is updated.

vue2-1 vue2-2

Vue 3

Each little bar is a re-rendered component.

vue3-1
basil commented 11 months ago

Yikes. I did not notice that, but I am running the application on an 8-core Intel i7-9700K at 4.9 GHz.

From my extremely naïve perspective, two potential theories come to mind:

Since I am extremely inexperienced with this framework in general and with this code base in particular, please take these theories with a grain of salt and do not waste too much time on them if they do not make sense.

basil commented 11 months ago

By putting a breakpoint on startMeasure in runtime-core.esm-bundler.js I was able to get a stack trace right before the problematic rendering activity. The thing being rendered when we are about to go off the rails is a ContentEditable that is attached to a TextBox. I tried deleting anything related to ContentEditable from TextBox and the problem goes away:

diff --git a/src/components/Editor.vue b/src/components/Editor.vue
index bbb26e6..21801d2 100644
--- a/src/components/Editor.vue
+++ b/src/components/Editor.vue
@@ -2683,9 +2683,6 @@ export default class Editor extends Vue {
   onKeydownTextBox(event: KeyboardEvent) {
     let handled = false;

-    const index = this.elements.indexOf(this.selectedElement!);
-    const htmlElement = (this.$refs[`element-${index}`] as TextBox[])[0];
-
     switch (event.code) {
       case 'Tab':
         this.moveRightThrottled();
@@ -2698,12 +2695,6 @@ export default class Editor extends Vue {
         }
         break;
       case 'ArrowRight':
-        if (
-          getCursorPosition() === htmlElement.textElement.getInnerText().length
-        ) {
-          this.moveRightThrottled();
-          handled = true;
-        }
         break;
     }

@@ -3003,73 +2994,10 @@ export default class Editor extends Vue {
   ];

   moveLeft() {
-    let index = -1;
-
-    if (this.selectedElement) {
-      index = this.elements.indexOf(this.selectedElement);
-    } else if (this.selectionRange) {
-      index = this.selectionRange.end;
-    }
-
-    if (
-      index - 1 >= 0 &&
-      this.navigableElements.includes(this.elements[index - 1].elementType)
-    ) {
-      // If the currently selected element is a drop cap or text box, blur it first
-      if (this.selectedElement?.elementType === ElementType.DropCap) {
-        (this.$refs[`element-${index}`] as DropCap[])[0].blur();
-      } else if (this.selectedElement?.elementType === ElementType.TextBox) {
-        (this.$refs[`element-${index}`] as TextBox[])[0].blur();
-      }
-
-      this.selectedElement = this.elements[index - 1];
-
-      // If the newly selected element is a drop cap or text box, focus it
-      if (this.selectedElement.elementType === ElementType.DropCap) {
-        (this.$refs[`element-${index - 1}`] as DropCap[])[0].focus();
-      } else if (this.selectedElement.elementType === ElementType.TextBox) {
-        (this.$refs[`element-${index - 1}`] as TextBox[])[0].focus();
-      }
-
-      return true;
-    }
-
     return false;
   }

   moveRight() {
-    let index = -1;
-
-    if (this.selectedElement) {
-      index = this.elements.indexOf(this.selectedElement);
-    } else if (this.selectionRange) {
-      index = this.selectionRange.end;
-    }
-
-    if (
-      index >= 0 &&
-      index + 1 < this.elements.length &&
-      this.navigableElements.includes(this.elements[index + 1].elementType)
-    ) {
-      // If the currently selected element is a drop cap, blur it first
-      if (this.selectedElement?.elementType === ElementType.DropCap) {
-        (this.$refs[`element-${index}`] as DropCap[])[0].blur();
-      } else if (this.selectedElement?.elementType === ElementType.TextBox) {
-        (this.$refs[`element-${index}`] as TextBox[])[0].blur();
-      }
-
-      this.selectedElement = this.elements[index + 1];
-
-      // If the newly selected element is a drop cap, focus it
-      if (this.selectedElement.elementType === ElementType.DropCap) {
-        (this.$refs[`element-${index + 1}`] as DropCap[])[0].focus();
-      } else if (this.selectedElement.elementType === ElementType.TextBox) {
-        (this.$refs[`element-${index + 1}`] as TextBox[])[0].focus();
-      }
-
-      return true;
-    }
-
     return false;
   }

diff --git a/src/components/TextBox.vue b/src/components/TextBox.vue
index 71f045c..7aaeeb4 100644
--- a/src/components/TextBox.vue
+++ b/src/components/TextBox.vue
@@ -1,27 +1,17 @@
 <template>
   <div class="text-box-container" :style="containerStyle">
-    <ContentEditable
-      ref="text"
-      class="text-box"
-      :class="textBoxClass"
-      :style="textBoxStyle"
-      :content="content"
-      :editable="editMode"
-      @blur="updateContent($event)"
-    ></ContentEditable>
   </div>
 </template>

 <script lang="ts">
 import { Component, Prop, Vue } from 'vue-facing-decorator';
 import { TextBoxElement } from '@/models/Element';
-import ContentEditable from '@/components/ContentEditable.vue';
 import { withZoom } from '@/utils/withZoom';
-import { replaceTokens, TokenMetadata } from '@/utils/replaceTokens';
+import { TokenMetadata } from '@/utils/replaceTokens';
 import { getFontFamilyWithFallback } from '@/utils/getFontFamilyWithFallback';

 @Component({
-  components: { ContentEditable },
+  components: {},
   emits: ['update:content'],
 })
 export default class TextBox extends Vue {
@@ -29,18 +19,8 @@ export default class TextBox extends Vue {
   @Prop({ default: true }) editMode!: boolean;
   @Prop() metadata!: TokenMetadata;

-  get textElement() {
-    return this.$refs.text as ContentEditable;
-  }
-
   get content() {
-    return this.editMode
-      ? this.element.content
-      : replaceTokens(
-          this.element.content,
-          this.metadata,
-          this.element.alignment,
-        );
+    return this.element.content;
   }

   get width() {
@@ -87,14 +67,6 @@ export default class TextBox extends Vue {

     this.$emit('update:content', content);
   }
-
-  blur() {
-    this.textElement.blur();
-  }
-
-  focus() {
-    this.textElement.focus(true);
-  }
 }
 </script>

So I feel confident that I am on the right track and that something is different in Vue 3 with regard to this.$refs.text on TextBox.

basil commented 11 months ago

Never mind, the above comment was wrong because my Vue.js Devtools Performance Timeline hadn't actually refreshed after I had done the test. I fixed that by switching to the Routes tab and switching back to Timeline. Clearly the same problem is still there. Back to the drawing board…

danielgarthur commented 11 months ago

A large part of the issue is this: https://github.com/vuejs/core/issues/3271. I was able to move the click handler down into the SyllableNeumeBox component and have the component catch the click and send a new select event up that the Editor catches. The syllables now no longer mass re-render.

I'll continue applying this method to other v-for uses and investigate further.

danielgarthur commented 11 months ago

I've found another piece of the puzzle, perhaps the last one. Vue 3 makes the Editor's score property reactive and so processPages triggers a bunch of reactivity-related overhead. Fortunately, there is a method for that!

Updating the save method to use toRaw, combined with the event changes in my previous comment seem to have fixed the slow-down issues.

const pages = LayoutService.processPages(toRaw(this.score));

It may be a few days before I get all the changes committed but I think I have all the pieces that I need.