Fix the god damn theme color setting (#7992)

Fix the god damn theme color

Author: lee-at-zoo-corp

e2e/playwright/test-utils.ts

@@ -1448,3 +1448,94 @@ export async function pinchFromCenter(
 
   await locator.dispatchEvent('touchend')
 }
+
+// Primarily machinery to click and drag a slider.
+// THIS ASSUMES THE SLIDER IS ALWAYS HORIZONTAL.
+export const inputRangeValueToCoordinate = async function (
+  locator: Locator,
+  valueTargetStr: string
+): Promise<{
+  startPoint: { x: number; y: number }
+  offsetFromStartPoint: { x: number; y: number }
+}> {
+  const bb = await locator.boundingBox()
+  if (bb === null)
+    throw new Error("Bounding box is null, can't do fucking shit")
+  const editable = await locator.isEditable()
+  if (!editable) throw new Error('Cannot slide range, element is not editable')
+  const visible = await locator.isVisible()
+  if (!visible) throw new Error('Cannot slide range, element is not visible')
+  await locator.scrollIntoViewIfNeeded()
+  const maybeMin = await locator.getAttribute('min')
+  const maybeMax = await locator.getAttribute('max')
+  const maybeStep = await locator.getAttribute('step')
+
+  let low = maybeMin ? Number(maybeMin) : 0
+  low = Number.isNaN(low) ? 0 : low
+
+  let upp = maybeMax ? Number(maybeMax) : 10
+  upp = Number.isNaN(upp) ? 10 : upp
+
+  let step = maybeMax ? Number(maybeStep) : 1
+  step = Number.isNaN(step) ? 1 : step
+
+  let value = Number(valueTargetStr)
+  if (Number.isNaN(value)) throw new Error('value must be a number')
+
+  // into step
+  value = value - (value % step)
+
+  // half step down
+  value -= Math.trunc(step / 2)
+
+  const scale = await locator.page().evaluate(() => window.devicePixelRatio)
+  // measured this value in gimp
+  const nub = { width: 14 * scale }
+
+  // 4 is the internal border + padding of the slider
+  let offsetX = (bb.width - 4 - nub.width / 2) * (value / (upp + low))
+
+  // map to coordinate
+  // relative to element
+  const offsetFromStartPoint = {
+    x: Math.trunc(offsetX + 0.5),
+    // need to land on the scroll nub
+    y: bb.height * 0.5,
+  }
+
+  const startPoint = {
+    x: bb.x + nub.width / 2,
+    y: bb.y,
+  }
+
+  return {
+    startPoint,
+    offsetFromStartPoint,
+  }
+}
+
+export const inputRangeSlideFromCurrentTo = async function (
+  locator: Locator,
+  valueNext: string
+) {
+  const valueCurrent = await locator.inputValue()
+  // Can't click it if the computer can't see it!
+  await locator.scrollIntoViewIfNeeded()
+  const from = await inputRangeValueToCoordinate(locator, valueCurrent)
+  const to = await inputRangeValueToCoordinate(locator, valueNext)
+  // I (lee) want to force the mouse to be up, but who knows who needs otherwise.
+  // await locator.page.mouse.up()
+  const page = locator.page()
+  await page.mouse.move(
+    from.startPoint.x + from.offsetFromStartPoint.x,
+    from.startPoint.y + from.offsetFromStartPoint.y,
+    { steps: 10 }
+  )
+  await page.mouse.down()
+  await page.mouse.move(
+    to.startPoint.x + to.offsetFromStartPoint.x,
+    to.startPoint.y + to.offsetFromStartPoint.y,
+    { steps: 10 }
+  )
+  await page.mouse.up()
+}

e2e/playwright/testing-settings.spec.ts

@@ -22,13 +22,33 @@ import {
   lowerRightMasks,
   settingsToToml,
   tomlToSettings,
+  inputRangeSlideFromCurrentTo,
 } from '@e2e/playwright/test-utils'
 import { expect, test } from '@e2e/playwright/zoo-test'
+import type { Page } from '@playwright/test'
+
+const settingsSwitchTab = (page: Page) => async (tab: 'user' | 'proj') => {
+  const projectSettingsTab = page.getByRole('radio', { name: 'Project' })
+  const userSettingsTab = page.getByRole('radio', { name: 'User' })
+  const settingTheme = page.getByTestId('theme')
+  switch (tab) {
+    case 'user':
+      await userSettingsTab.click()
+      await expect(settingTheme).toBeVisible()
+      break
+    case 'proj':
+      await projectSettingsTab.click()
+      await expect(settingTheme).not.toBeVisible()
+      break
+    default:
+      const _: never = tab
+  }
+}
 
 test.describe(
   'Testing settings',
   {
-    tag: ['@macos', '@windows'],
+    tag: ['@linux', '@macos', '@windows'],
   },
   () => {
     test('Stored settings are validated and fall back to defaults', async ({
@@ -199,17 +219,18 @@ test.describe(
       })
 
       // Selectors and constants
-      const projectSettingsTab = page.getByRole('radio', { name: 'Project' })
-      const userSettingsTab = page.getByRole('radio', { name: 'User' })
       const resetButton = (level: SettingsLevel) =>
         page.getByRole('button', {
           name: `Reset ${level}-level settings`,
         })
       const themeColorSetting = page.locator('#themeColor').getByRole('slider')
+
       const settingValues = {
         default: '259',
-        user: '120',
-        project: '50',
+        // Because it's a slider, sometimes the values cannot physically be
+        // dragged to. You need to adjust this until it works.
+        user: '48',
+        project: '77',
       }
       const resetToast = (level: SettingsLevel) =>
         page.getByText(`${level}-level settings were reset`)
@@ -222,21 +243,31 @@ test.describe(
       })
 
       await test.step('Set up theme color', async () => {
-        // Verify we're looking at the project-level settings,
-        // and it's set to default value
-        await expect(projectSettingsTab).toBeChecked()
-        await expect(themeColorSetting).toHaveValue(settingValues.default)
-
-        // Set project-level value to 50
-        await themeColorSetting.fill(settingValues.project)
-
-        // Set user-level value to 120
-        await userSettingsTab.click()
-        await themeColorSetting.fill(settingValues.user)
-        await projectSettingsTab.click()
+        // Verify we're looking at the project-level settings
+        await settingsSwitchTab(page)('proj')
+        await themeColorSetting.fill(settingValues.default)
+
+        // Set project-level value
+        await inputRangeSlideFromCurrentTo(
+          themeColorSetting,
+          settingValues.project
+        )
+        await expect(themeColorSetting).toHaveValue(settingValues.project)
+
+        // Set user-level value
+        // It's the same component so this could fill too soon.
+        // We need to confirm to wait the user settings tab is loaded.
+        await settingsSwitchTab(page)('user')
+        await inputRangeSlideFromCurrentTo(
+          themeColorSetting,
+          settingValues.user
+        )
+        await expect(themeColorSetting).toHaveValue(settingValues.user)
       })
 
       await test.step('Reset project settings', async () => {
+        await settingsSwitchTab(page)('proj')
+
         // Click the reset settings button.
         await resetButton('project').click()
 
@@ -247,14 +278,17 @@ test.describe(
         await expect(themeColorSetting).toHaveValue(settingValues.user)
 
         await test.step(`Check that the user settings did not change`, async () => {
-          await userSettingsTab.click()
+          await settingsSwitchTab(page)('user')
           await expect(themeColorSetting).toHaveValue(settingValues.user)
         })
 
         await test.step(`Set project-level again to test the user-level reset`, async () => {
-          await projectSettingsTab.click()
-          await themeColorSetting.fill(settingValues.project)
-          await userSettingsTab.click()
+          await settingsSwitchTab(page)('proj')
+          await inputRangeSlideFromCurrentTo(
+            themeColorSetting,
+            settingValues.project
+          )
+          await settingsSwitchTab(page)('user')
         })
       })
 
@@ -269,7 +303,7 @@ test.describe(
         await expect(themeColorSetting).toHaveValue(settingValues.default)
 
         await test.step(`Check that the project settings did not change`, async () => {
-          await projectSettingsTab.click()
+          await settingsSwitchTab(page)('proj')
           await expect(themeColorSetting).toHaveValue(settingValues.project)
         })
       })
@@ -303,7 +337,7 @@ test.describe(
           projectDirName,
           SETTINGS_FILE_NAME
         )
-        const userThemeColor = '120'
+        const userThemeColor = '175'
         const projectThemeColor = '50'
         const settingsOpenButton = page.getByRole('link', {
           name: 'settings Settings',
@@ -322,7 +356,7 @@ test.describe(
           await settingsOpenButton.click()
           // The user tab should be selected by default on home
           await expect(userSettingsTab).toBeChecked()
-          await themeColorSetting.fill(userThemeColor)
+          await inputRangeSlideFromCurrentTo(themeColorSetting, userThemeColor)
           await expect(logoLink).toHaveCSS('--primary-hue', userThemeColor)
           await settingsCloseButton.click()
           await expect
@@ -339,7 +373,10 @@ test.describe(
           await settingsOpenButton.click()
           // The project tab should be selected by default within a project
           await expect(projectSettingsTab).toBeChecked()
-          await themeColorSetting.fill(projectThemeColor)
+          await inputRangeSlideFromCurrentTo(
+            themeColorSetting,
+            projectThemeColor
+          )
           await expect(logoLink).toHaveCSS('--primary-hue', projectThemeColor)
           await settingsCloseButton.click()
           // Make sure that the project settings file has been written to before continuing
@@ -631,7 +668,7 @@ test.describe(
       })
 
       await test.step(`Reset unit setting`, async () => {
-        await userSettingsTab.click()
+        await settingsSwitchTab(page)('user')
         await defaultUnitSection.hover()
         await defaultUnitRollbackButton.click()
         await projectSettingsTab.hover()
@@ -666,7 +703,7 @@ test.describe(
 
       // Go to the user tab
       await userSettingsTab.hover()
-      await userSettingsTab.click()
+      await settingsSwitchTab(page)('user')
       await page.waitForTimeout(1000)
 
       await test.step('Change modeling default unit within user tab', async () => {

src/components/Settings/SettingsSection.tsx

@@ -25,6 +25,7 @@ export function SettingsSection({
 }: SettingsSectionProps) {
   return (
     <section
+      data-testid={id}
       id={id}
       className={
         'group p-2 pl-0 grid grid-cols-2 gap-6 items-start ' +

src/lib/settings/initialSettings.tsx

@@ -1,4 +1,4 @@
-import { useRef } from 'react'
+import { useRef, useEffect } from 'react'
 
 import type { CameraOrbitType } from '@rust/kcl-lib/bindings/CameraOrbitType'
 import type { CameraProjectionType } from '@rust/kcl-lib/bindings/CameraProjectionType'
@@ -158,19 +158,41 @@ export function createSettings() {
         defaultValue: '264.5',
         description: 'The hue of the primary theme color for the app',
         validate: (v) => Number(v) >= 0 && Number(v) < 360,
+
+        // The same component instance is used across settings panes / tabs.
         Component: ({ value, updateValue }) => {
-          const preview = (e: React.SyntheticEvent) =>
-            e.isTrusted &&
-            'value' in e.currentTarget &&
+          const refInput = useRef<HTMLInputElement>(null)
+
+          const updateColorDot = (value: string) => {
             document.documentElement.style.setProperty(
               `--primary-hue`,
-              String(e.currentTarget.value)
+              String(value)
             )
-          const save = (e: React.SyntheticEvent) =>
+          }
+
+          useEffect(() => {
+            if (refInput.current === null) return
+            refInput.current.value = value
+            updateColorDot(value)
+          }, [value])
+
+          const preview = (e: React.SyntheticEvent) =>
             e.isTrusted &&
             'value' in e.currentTarget &&
-            e.currentTarget.value &&
-            updateValue(String(e.currentTarget.value))
+            updateColorDot(String(e.currentTarget.value))
+
+          const save = (e: React.SyntheticEvent) => {
+            if (
+              e.isTrusted &&
+              'value' in e.currentTarget &&
+              e.currentTarget.value
+            ) {
+              const valueNext = String(e.currentTarget.value)
+              updateValue(valueNext)
+              updateColorDot(valueNext)
+            }
+          }
+
           return (
             <div className="flex item-center gap-4 px-2 m-0 py-0">
               <div
@@ -181,11 +203,11 @@ export function createSettings() {
               />
               <input
                 type="range"
+                ref={refInput}
                 onInput={preview}
                 onMouseUp={save}
                 onKeyUp={save}
                 onPointerUp={save}
-                defaultValue={value}
                 min={0}
                 max={259}
                 step={1}