Merge pull request #60 from ShortArrow/fix/select-bounce

Carry pending previews in state messages (fixes the segment bounce)

Author: ShortArrow

src/panel.ts

@@ -268,20 +268,22 @@ export async function toggleSettingsPanel(): Promise<void> {
 }
 
 /**
- * The compiled conversions, as text to paste into the webview script.
+ * The compiled conversions and the pending merge, as text to paste into the
+ * webview script.
  *
  * A panel whose sliders cannot convert is still worth showing, so an unreadable
  * file costs the sliders and nothing else: the picker keeps working.
  */
 function readInlineLib(extensionPath: string): string {
-  try {
-    return fs.readFileSync(
-      path.join(extensionPath, "out", "colorConvert.js"),
-      "utf8"
-    );
-  } catch {
-    return "";
-  }
+  return ["colorConvert.js", "panelState.js"]
+    .map((file) => {
+      try {
+        return fs.readFileSync(path.join(extensionPath, "out", file), "utf8");
+      } catch {
+        return "";
+      }
+    })
+    .join("\n");
 }
 
 class ColorPanelProvider implements vscode.WebviewViewProvider {
@@ -323,12 +325,22 @@ class ColorPanelProvider implements vscode.WebviewViewProvider {
     });
   }
 
+  /**
+   * Tell the webview what is saved and what is still only proposed.
+   *
+   * The pending map travels with the saved values because the script displays
+   * one over the other: a state message posted while a row is staged used to
+   * carry the saved value alone, and the row snapped back to it.
+   */
   postState() {
     this.view?.webview.postMessage({
       type: "state",
       toggles: currentToggles(),
       selects: currentSelects(),
       rows: currentRows(),
+      pending: Object.fromEntries(
+        getPendingPreviews().map((entry) => [entry.key, entry.value])
+      ),
     });
   }
 

src/panelHtml.ts

@@ -200,6 +200,25 @@ const script = `      const vscode = acquireVsCodeApi();
       function convert(from, to) {
         return exports[from + 'To' + to.charAt(0).toUpperCase() + to.slice(1)];
       }
+      /**
+       * One inlined merge of a saved value with whatever is staged, by the kind
+       * of control asking, or undefined when the library could not be read.
+       *
+       * Assembled like {@link convert} and for the same reason: the panel
+       * script never spells the name, so finding it in the document is proof
+       * the library was really inlined.
+       */
+      function display(kind) {
+        return exports['display' + kind.charAt(0).toUpperCase() + kind.slice(1)];
+      }
+      /**
+       * What one row should show now, falling back to the saved value alone
+       * when the merge could not be inlined.
+       */
+      function shown(kind, saved, key, pending) {
+        const merge = display(kind);
+        return merge ? merge(saved, key, pending || {}) : { value: saved, pending: false };
+      }
       function sliders(key) {
         return document.querySelectorAll('input[data-slider-for="' + key + '"]');
       }
@@ -457,34 +476,39 @@ const script = `      const vscode = acquireVsCodeApi();
         if (!message || message.type !== 'state') {
           return;
         }
+        const pending = message.pending || {};
         (message.toggles || []).forEach((toggle) => {
+          const entry = shown('toggle', toggle.value, toggle.key, pending);
           const box = switchOf(toggle.key);
           if (box) {
-            box.checked = toggle.value;
+            box.checked = entry.value;
           }
-          markPending(toggle.key, false);
+          markPending(toggle.key, entry.pending);
         });
         (message.selects || []).forEach((select) => {
-          markSelected(select.key, select.value);
-          markPending(select.key, false);
+          const entry = shown('value', select.value, select.key, pending);
+          markSelected(select.key, entry.value);
+          markPending(select.key, entry.pending);
         });
         message.rows.forEach((row) => {
-          markPending(row.key, false);
+          const entry = shown('value', row.savedColor, row.key, pending);
+          const color = entry.value;
+          markPending(row.key, entry.pending);
           const swatch = document.querySelector('[data-swatch="' + row.key + '"]');
           if (swatch) {
-            swatch.style.background = row.savedColor;
-            swatch.title = row.savedColor;
+            swatch.style.background = color;
+            swatch.title = color;
           }
           const input = document.querySelector('input[type="color"][data-key="' + row.key + '"]');
-          if (input && /^#[0-9a-fA-F]{6}$/.test(row.savedColor)) {
-            input.value = row.savedColor;
+          if (input && /^#[0-9a-fA-F]{6}$/.test(color)) {
+            input.value = color;
           }
-          showOnSliders(row.key, /^#[0-9a-fA-F]{6}$/.test(row.savedColor) ? row.savedColor : '#000000');
-          // The saved value wins over whatever is being typed: a reset has to
-          // reach a field the reader still has the caret in.
+          showOnSliders(row.key, /^#[0-9a-fA-F]{6}$/.test(color) ? color : '#000000');
+          // The displayed value wins over whatever is being typed: a reset has
+          // to reach a field the reader still has the caret in.
           const field = document.querySelector('[data-hex-for="' + row.key + '"]');
           if (field) {
-            field.value = row.savedColor;
+            field.value = color;
             field.classList.remove('invalid');
           }
         });
@@ -558,15 +582,18 @@ const style = `      body { font-family: var(--vscode-font-family); font-size: v
  *
  * Flipping a switch or dragging a picker only previews; a row marks itself
  * pending until a state message says its value was saved. The footer button
- * commits every pending row at once.
+ * commits every pending row at once. A state message names what is saved and
+ * what is staged separately, and every control shows the staged value over the
+ * saved one, so a resync arriving mid-edit cannot undo a choice.
  *
  * The select rows sit between the two: they are settings of the editor rather
  * than of this extension, and choosing one stages it without any local
  * rendering, because VS Code draws those numbers itself.
  *
- * @param inlineLib compiled color conversions, pasted verbatim into the nonced
- *   script behind an exports shim so the sliders convert with the very code the
- *   unit tests cover; an empty string leaves the row usable through its picker
+ * @param inlineLib the compiled color conversions and pending merge, pasted
+ *   verbatim into the nonced script behind an exports shim so the panel runs
+ *   the very code the unit tests cover; an empty string leaves the row usable
+ *   through its picker
  */
 export function renderPanelHtml(
   toggles: PanelToggle[],

src/panelState.ts

@@ -0,0 +1,57 @@
+/**
+ * What a control shows, once the staged value is laid over the saved one.
+ *
+ * A state message names what is saved, and separately what the panel is still
+ * proposing; the two are merged here so the webview and the unit tests reach
+ * the same answer instead of two implementations that agree until they drift.
+ * Like the color conversions, the module imports nothing and stays inside
+ * ES2020: its compiled CommonJS is inlined into the webview script, where
+ * there is no module loader and no node standard library.
+ */
+
+/** Settings the panel is proposing, keyed by configuration name. */
+export interface PendingMap {
+  [key: string]: string | boolean;
+}
+
+/** One value to display, and whether it is staged rather than saved. */
+export interface DisplayEntry {
+  value: string;
+  pending: boolean;
+}
+
+/**
+ * The text one row shows: the staged value when there is one, else the saved.
+ *
+ * An entry of the wrong type is not this row's: previews of switches and of
+ * enumerated settings share one store, so a boolean parked under this key is
+ * ignored rather than rendered.
+ *
+ * @param saved the value the configuration currently holds
+ * @param key the configuration name this row edits
+ * @param pending everything the panel is proposing
+ */
+export function displayValue(
+  saved: string,
+  key: string,
+  pending: PendingMap
+): DisplayEntry {
+  const staged = pending[key];
+  if (typeof staged === "string") {
+    return { value: staged, pending: true };
+  }
+  return { value: saved, pending: false };
+}
+
+/** The state one switch shows, by the same rule as {@link displayValue}. */
+export function displayToggle(
+  saved: boolean,
+  key: string,
+  pending: PendingMap
+): { value: boolean; pending: boolean } {
+  const staged = pending[key];
+  if (typeof staged === "boolean") {
+    return { value: staged, pending: true };
+  }
+  return { value: saved, pending: false };
+}

src/test/panelMessages.test.ts

@@ -2,7 +2,9 @@ import * as assert from 'assert';
 import { beforeEach, describe, it } from 'mocha';
 import { PanelMessageDeps, handlePanelMessage } from '../panel';
 import {
+  PendingPreview,
   clearAllPreviews,
+  getPendingPreviews,
   getPreviewColor,
   getPreviewToggle,
   setPreviewColor,
@@ -163,6 +165,18 @@ describe('Test panel message handling', () => {
     assert.strictEqual(counts.postState, 1);
   });
 
+  it('Must have the staged select pending while that state is posted', async () => {
+    const staged: PendingPreview[][] = [];
+    const { deps } = recordingDeps();
+    await handlePanelMessage(
+      { type: 'preview', key: 'editor.lineNumbers', value: 'relative' },
+      { ...deps, postState: () => staged.push(getPendingPreviews()) }
+    );
+    assert.deepStrictEqual(staged, [
+      [{ key: 'editor.lineNumbers', value: 'relative' }],
+    ]);
+  });
+
   it('Must ignore a select value the setting does not offer', async () => {
     const { deps, saves, counts } = recordingDeps();
     await handlePanelMessage(

src/test/panelState.test.ts

@@ -0,0 +1,53 @@
+import * as assert from 'assert';
+import { describe, it } from 'mocha';
+import { displayToggle, displayValue } from '../panelState';
+
+describe('Test panel display values', () => {
+  it('Must show the saved value while nothing is pending', () => {
+    assert.deepStrictEqual(displayValue('on', 'editor.lineNumbers', {}), {
+      value: 'on',
+      pending: false,
+    });
+  });
+
+  it('Must show the pending value over the saved one', () => {
+    assert.deepStrictEqual(
+      displayValue('on', 'editor.lineNumbers', {
+        'editor.lineNumbers': 'relative',
+      }),
+      { value: 'relative', pending: true }
+    );
+  });
+
+  it('Must ignore a pending entry that is not a string', () => {
+    assert.deepStrictEqual(displayValue('on', 'k', { k: true }), {
+      value: 'on',
+      pending: false,
+    });
+  });
+
+  it('Must show a pending switch over its saved state', () => {
+    assert.deepStrictEqual(displayToggle(false, 'enableRainbow', { enableRainbow: true }), {
+      value: true,
+      pending: true,
+    });
+    assert.deepStrictEqual(displayToggle(false, 'enableRainbow', {}), {
+      value: false,
+      pending: false,
+    });
+  });
+
+  it('Must ignore a pending entry that is not a boolean', () => {
+    assert.deepStrictEqual(displayToggle(true, 'k', { k: '#fff' }), {
+      value: true,
+      pending: false,
+    });
+  });
+
+  it('Must leave a row alone while another key is pending', () => {
+    assert.deepStrictEqual(displayValue('on', 'a', { b: 'off' }), {
+      value: 'on',
+      pending: false,
+    });
+  });
+});

src/test/panelView.test.ts

@@ -51,6 +51,10 @@ describe('Test color panel view', () => {
       (html as string).includes('hexToHsl'),
       'resolved panel html does not inline the color conversions'
     );
+    assert.ok(
+      (html as string).includes('displayValue'),
+      'resolved panel html does not inline the pending merge'
+    );
     assert.ok(
       (html as string).includes('data-plane-for="foreground"'),
       'resolved panel html has no picking plane for foreground'