Issue #411 #501

inthar-raven · 2023-11-27T21:24:47Z

Added radio buttons and the reaction. Using zero-indexing is the default and omits the column of all unisons.

frostburn · 2023-11-28T03:29:37Z

src/views/AnalysisView.vue

@@ -87,7 +89,8 @@ const matrix = computed(() => {
    props.scale.head(MAX_SCALE_SIZE).mergeOptions({
      centsFractionDigits: 1,
      decimalFractionDigits: 3,
-    })
+    }), 
+    1 - (matrixStartIndex.value)


You shouldn't call a property something and then pass it as the inverse of that to another function.

What I have should work fine (for issue #411), though. I am using just "matrixStartIndex.value" in other parts.

It's admittedly a really janky way to not display the zeroth column but just for the zero-indexed option. I should fix that.

frostburn · 2023-11-28T03:32:16Z

src/views/AnalysisView.vue

@@ -99,10 +102,10 @@ const matrix = computed(() => {
      <table>
        <tr>
          <th></th>
-          <th v-for="i of Math.min(scale.size, MAX_SCALE_SIZE)" :key="i">
+          <th v-for="i of Math.min(scale.size-1+matrixStartIndex, MAX_SCALE_SIZE-1)" :key="i">


MAX_SCALE_SIZE is a sanity limit that makes sure the browser doesn't go 💥 . You could rename it to something like MAX_NUM_COLUMNS or something. That -1 should be there on a constant.

frostburn · 2023-11-28T03:35:13Z

src/analysis.ts

  const result = [];
-  const degrees = [...Array(scale.size + 1).keys()];
+  const degrees = [...Array(scale.size + 1 - startIndex).keys()];


I know this is my code, but this is no longer an array of degrees.
Just do an extra for loop below and collect stuff into a const row = [] array.

frostburn · 2023-11-28T03:35:40Z

src/views/AnalysisView.vue

@@ -19,6 +19,8 @@ const props = defineProps<{
 }>();

 const cellFormat = ref<"best" | "cents" | "decimal">("best");
+const indexing = ref<"zero" | "one">("zero");
+const matrixStartIndex = computed(() => indexing.value === "one" ? 1 : 0); 


To be specific: Swap here, not down there.

Computed properties should be collected together after mutable state.

frostburn · 2023-11-28T03:39:29Z

The choice should be persisted

watch(indexing, (newValue) =>
  window.localStorage.setItem("intervalMatrixIndexing", newValue)
);

and the corresponding .getItem(...) ?? "zero" when initializing the ref.

frostburn · 2023-11-29T03:34:52Z

src/views/AnalysisView.vue

@@ -19,6 +19,8 @@ const props = defineProps<{
 }>();

 const cellFormat = ref<"best" | "cents" | "decimal">("best");
+const intervalMatrixIndexing = ref<"zero" | "one">("zero");


If you want to persist the value you have to use a component prop and drill it back to the app using emits.
Now this line just creates a new local property.

frostburn · 2023-11-29T04:18:37Z

src/App.vue

@@ -288,6 +289,7 @@ watch(
    pingPongFeedback,
    pingPongSeparation,
    pingPongGain,
+    intervalMatrixIndexing


You specifically don't want to share your personal preferences with the world.

frostburn · 2023-11-29T04:20:49Z

The app doesn't load at all for me now.

frostburn · 2023-11-29T04:21:43Z

src/analysis.ts

@@ -103,12 +103,12 @@ export function utonalFundamental(frequencies: number[], maxDivisor = 23) {
 }

 // Interval matrix a.k.a the modes of a scale
-export function intervalMatrix(scale: Scale) {
+export function intervalMatrix(scale: Scale, startIndex: number) {


This is now an unrelated change that doesn't belong in the scope of this PR.

frostburn · 2023-11-29T04:39:20Z

Now it works on my end. What's the intended behaviour?
I'm seeing that toggling the radio button simply adds one to the header row. If that's all that this is supposed to do, then clean the code to only do that. No need for fancy computes or anything.

frostburn · 2023-11-29T04:41:16Z

src/App.vue

@@ -120,6 +120,7 @@ const centsFractionDigits = ref(3);
 const decimalFractionDigits = ref(5);
 const showVirtualQwerty = ref(false);
 const midiOctaveOffset = ref(-1);
+const intervalMatrixIndexing = ref<"zero" | "one">("zero");


This could be replaced with a number called intervalMatrixIndexOffset that gets added to the headers if that's the intended effect.

In my previous iterations of attempting that, it seems the radio buttons don't want me to make their associated type a number instead of a radio:

Internal server error: Unnecessary value binding used alongside v-model. It will interfere with v-model's behavior.
Plugin: vite:vue
File: /home/inthar/Downloads/scale-workshop-main/src/views/AnalysisView.vue
160| <input
161| type="number"
162| id="indexing-zero"
| ^
163| value="0"
164| v-model="intervalMatrixIndexing"
at Object.createCompilerError (/home/inthar/Downloads/scale-workshop-main/node_modules/@vue/compiler-core/dist/compiler-core.cjs.js:19:19)
at createDOMCompilerError (/home/inthar/Downloads/scale-workshop-main/node_modules/@vue/compiler-dom/dist/compiler-dom.cjs.js:2483:25)
at checkDuplicatedValue (/home/inthar/Downloads/scale-workshop-main/node_modules/@vue/compiler-dom/dist/compiler-dom.cjs.js:2547:29)
at transformModel (/home/inthar/Downloads/scale-workshop-main/node_modules/@vue/compiler-dom/dist/compiler-dom.cjs.js:2579:29)
at buildProps (/home/inthar/Downloads/scale-workshop-main/node_modules/@vue/compiler-core/dist/compiler-core.cjs.js:4777:48)
at Array.postTransformElement (/home/inthar/Downloads/scale-workshop-main/node_modules/@vue/compiler-core/dist/compiler-core.cjs.js:4374:38)
at traverseNode (/home/inthar/Downloads/scale-workshop-main/node_modules/@vue/compiler-core/dist/compiler-core.cjs.js:2225:19)
at traverseChildren (/home/inthar/Downloads/scale-workshop-main/node_modules/@vue/compiler-core/dist/compiler-core.cjs.js:2167:9)
at traverseNode (/home/inthar/Downloads/scale-workshop-main/node_modules/@vue/compiler-core/dist/compiler-core.cjs.js:2218:13)
at traverseChildren (/home/inthar/Downloads/scale-workshop-main/node_modules/@vue/compiler-core/dist/compiler-core.cjs.js:2167:9)

It's one of these things that seem like they ought to work but don't.

Yeah, you need an auxillary ref and a watcher if you want to use v-model it's better to just emit the correct values from the radio buttons directly.

What can I use to label the data instead of a v-model?

frostburn · 2023-11-29T05:24:52Z

src/views/AnalysisView.vue

@@ -87,7 +97,8 @@ const matrix = computed(() => {
    props.scale.head(MAX_SCALE_SIZE).mergeOptions({
      centsFractionDigits: 1,
      decimalFractionDigits: 3,
-    })
+    }), 
+    0


This whole change should be reverted as I misread your intention when suggesting it.

frostburn · 2023-11-29T05:26:32Z

This is now 13 commits long. They should all be squashed into one so that we get a readable commit history in main.

frostburn · 2023-11-29T06:03:18Z

src/analysis.ts

@@ -105,10 +105,10 @@ export function utonalFundamental(frequencies: number[], maxDivisor = 23) {
 // Interval matrix a.k.a the modes of a scale
 export function intervalMatrix(scale: Scale) {
  const result = [];
-  const degrees = [...Array(scale.size + 1).keys()];
+  const columns = [...Array(scale.size + 1).keys()];


A little unrelated, but OK...

frostburn · 2023-11-29T06:06:18Z

src/App.vue

@@ -783,7 +784,9 @@ onMounted(() => {
  if ("midiOctaveOffset" in storage) {
    midiOctaveOffset.value = parseInt(storage.getItem("midiOctaveOffset")!);
  }
-
+  if ("intervalMatrixIndexing" in storage) {
+    intervalMatrixIndexing.value = parseInt(storage.getItem("intervalMatrixIndexing")!);


parseInt(null) results in NaN so that exclamation mark is definitely in the wrong here. (Remember to test after running window.localStorage.clear() in the console.)

frostburn · 2023-11-29T06:06:47Z

GitHubActions seems to have something to say about your code style.

frostburn · 2023-11-29T06:16:14Z

The commit message is too long. Use git commit --amend to reword it. (Also handy to avoid generating new commits so you avoid the rebases).

…of the top row of the interval matrix in Analysis view.

frostburn · 2023-11-29T06:32:27Z

src/App.vue

-
+  if ("intervalMatrixIndexing" in storage) {
+    intervalMatrixIndexing.value = parseInt(
+      storage.getItem("intervalMatrixIndexing")!


Still doing naughty things and storing a NaN on first load.

This really should be parseInt(storage.getItem("intervalMatrixIndexing") ?? "0", 10)

frostburn · 2023-11-29T06:33:06Z

Commits need squashing again.

frostburn · 2023-11-29T06:41:25Z

Feel free to add your name on the About page.

Prevent a NaN on first load.

frostburn reviewed Nov 28, 2023

View reviewed changes

frostburn reviewed Nov 29, 2023

View reviewed changes

inthar-raven force-pushed the main branch from 27b759d to 995ab97 Compare November 29, 2023 06:00

frostburn reviewed Nov 29, 2023

View reviewed changes

Issue xenharmonic-devs#411, add radio buttons to change the indexing …

44c2479

…of the top row of the interval matrix in Analysis view.

frostburn reviewed Nov 29, 2023

View reviewed changes

inthar-raven force-pushed the main branch from 70afee5 to 44c2479 Compare November 29, 2023 06:33

inthar-raven added 2 commits November 29, 2023 01:53

Update App.vue

de61bd3

Prevent a NaN on first load.

Added my name on About page

6a28f41

frostburn merged commit fe1717d into xenharmonic-devs:main Nov 29, 2023
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #411 #501

Issue #411 #501

inthar-raven commented Nov 27, 2023

frostburn Nov 28, 2023

inthar-raven Nov 28, 2023

inthar-raven Nov 28, 2023

frostburn Nov 28, 2023

frostburn Nov 28, 2023

frostburn Nov 28, 2023

frostburn Nov 28, 2023

frostburn commented Nov 28, 2023

frostburn Nov 29, 2023

frostburn Nov 29, 2023

frostburn commented Nov 29, 2023

frostburn Nov 29, 2023

frostburn commented Nov 29, 2023

frostburn Nov 29, 2023

inthar-raven Nov 29, 2023

inthar-raven Nov 29, 2023

frostburn Nov 29, 2023

inthar-raven Nov 29, 2023

frostburn Nov 29, 2023

frostburn commented Nov 29, 2023

frostburn Nov 29, 2023

frostburn Nov 29, 2023

frostburn commented Nov 29, 2023

frostburn commented Nov 29, 2023

frostburn Nov 29, 2023

frostburn Nov 29, 2023

frostburn commented Nov 29, 2023

frostburn commented Nov 29, 2023

Issue #411 #501

Issue #411 #501

Conversation

inthar-raven commented Nov 27, 2023

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

frostburn commented Nov 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frostburn commented Nov 29, 2023

Choose a reason for hiding this comment

frostburn commented Nov 29, 2023

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

frostburn commented Nov 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frostburn commented Nov 29, 2023

frostburn commented Nov 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frostburn commented Nov 29, 2023

frostburn commented Nov 29, 2023