Selaa lähdekoodia

Sort the inventory by colour, not colour name (#2729)

The Color column was missing from the page's sort-extractor map, so its
header ignored clicks. Sorts by family first — rainbow, then browns,
then neutrals light to dark — with the hue sort running inside each.

The issue asked for a straight hue/saturation/lightness sort. Measured
against a real 30-spool inventory that puts Titan Gray (hue 210, sat
0.04) among the blues and a warm grey next to the reds, and splits the
oranges around brown. Neutrals order by lightness because their hue is
noise.

Families come from the classifier that already names colours missing
from the catalog, so the Color and Color Name columns cannot disagree.
maziggy 1 kuukausi sitten
vanhempi
sitoutus
dbf674561c

Tiedoston diff-näkymää rajattu, sillä se on liian suuri
+ 0 - 0
CHANGELOG.md


+ 171 - 0
frontend/src/__tests__/pages/InventoryPageColorSort.test.tsx

@@ -0,0 +1,171 @@
+/**
+ * Sorting the Inventory's Color column (#2729, reporter @macwhiz).
+ *
+ * The column existed and was visible by default, but was absent from the
+ * page's sort-extractor map, so clicking its header did nothing at all. These
+ * tests drive the real header rather than the sort key directly — the unit
+ * tests in utils/colors.test.ts cover the ordering itself — so a regression
+ * that drops the extractor again fails here even though the key still works.
+ */
+
+import { describe, it, expect, beforeEach, vi } from 'vitest';
+import { screen, waitFor, fireEvent } from '@testing-library/react';
+import { render } from '../utils';
+import InventoryPageRouter from '../../pages/InventoryPage';
+import { http, HttpResponse } from 'msw';
+import { server } from '../mocks/server';
+
+const baseSpool = {
+  subtype: null,
+  brand: 'eSun',
+  extra_colors: null,
+  effect_type: null,
+  label_weight: 1000,
+  core_weight: 250,
+  core_weight_catalog_id: null,
+  slicer_filament: null,
+  slicer_filament_name: null,
+  nozzle_temp_min: null,
+  nozzle_temp_max: null,
+  note: null,
+  added_full: null,
+  last_used: null,
+  encode_time: null,
+  tag_uid: null,
+  tray_uuid: null,
+  data_origin: null,
+  tag_type: null,
+  archived_at: null,
+  created_at: '2025-01-01T00:00:00Z',
+  updated_at: '2025-01-01T00:00:00Z',
+  k_profiles: [] as never[],
+  cost_per_kg: null,
+  last_scale_weight: null,
+  last_weighed_at: null,
+  storage_location: null,
+  category: null,
+  low_stock_threshold_pct: null,
+  spoolman_id: null,
+  spoolman_filament_id: null,
+  material: 'PLA',
+  weight_used: 0,
+};
+
+// Deliberately seeded in an order no single field would produce, and with the
+// two near-neutrals that a straight hue sort scatters into the blues and reds.
+//
+// The colour name rides on ``brand`` as well: the Color column is a swatch with
+// no text and the Color Name column is hidden by default, so Brand is what
+// makes the rendered order legible in an assertion.
+const spool = (id: number, name: string, rgba: string | null) => ({
+  ...baseSpool,
+  id,
+  color_name: name,
+  brand: name,
+  rgba,
+});
+
+const SPOOLS = [
+  spool(1, 'Titan Gray', '5F6367FF'),
+  spool(2, 'Sky Blue', '56B7E6FF'),
+  spool(3, 'Black', '000000FF'),
+  spool(4, 'Red', 'FF0000FF'),
+  spool(5, 'Peanut Brown', '875718FF'),
+  spool(6, 'No Colour', null),
+];
+
+const MOCK_SETTINGS = {
+  currency: 'USD',
+  language: 'en',
+  date_format: 'system',
+  time_format: 'system',
+  low_stock_threshold: 20.0,
+  spoolman_enabled: false,
+  spoolman_url: '',
+};
+
+function setupHandlers() {
+  server.use(
+    http.get('/api/v1/settings/', () => HttpResponse.json(MOCK_SETTINGS)),
+    http.get('/api/v1/settings/spoolman', () =>
+      HttpResponse.json({ spoolman_enabled: 'false', spoolman_url: '' }),
+    ),
+    http.get('/api/v1/inventory/spools', () => HttpResponse.json(SPOOLS)),
+    http.get('/api/v1/inventory/assignments', () => HttpResponse.json([])),
+    http.get('/api/v1/inventory/catalog', () => HttpResponse.json([])),
+    http.get('/api/v1/inventory/color-catalog', () => HttpResponse.json([])),
+    http.get('/api/v1/inventory/colors', () => HttpResponse.json([])),
+    http.get('/api/v1/inventory/spool-catalog', () => HttpResponse.json([])),
+    http.get('/api/v1/printers/', () => HttpResponse.json([])),
+  );
+}
+
+/** Table body rows, header excluded. */
+const dataRows = () => screen.getAllByRole('row').slice(1);
+
+/** Position of the row whose text contains ``name``, or -1. */
+const rowIndexOf = (name: string) =>
+  dataRows().findIndex((row) => (row.textContent ?? '').includes(name));
+
+describe('InventoryPage — sorting by colour', () => {
+  beforeEach(() => {
+    setupHandlers();
+    // The page persists sort state per browser; start every test unsorted.
+    vi.mocked(localStorage.getItem).mockReturnValue(null);
+  });
+
+  it('sorts the Color column into rainbow order with neutrals last', async () => {
+    render(<InventoryPageRouter />);
+    await waitFor(() => expect(dataRows().length).toBe(SPOOLS.length));
+
+    // Make the Color Name column visible so the order is readable, then sort.
+    fireEvent.click(screen.getByRole('columnheader', { name: /^color$/i }));
+
+    await waitFor(() => {
+      // Red -> Cyan -> Brown -> the neutrals -> the spool with no colour.
+      const positions = ['Red', 'Sky Blue', 'Peanut Brown', 'Titan Gray', 'Black', 'No Colour'].map(
+        rowIndexOf,
+      );
+      expect(positions.every((p) => p >= 0)).toBe(true);
+      expect(positions).toEqual([...positions].sort((a, b) => a - b));
+    });
+  });
+
+  it('reverses on a second click and clears on a third', async () => {
+    render(<InventoryPageRouter />);
+    await waitFor(() => expect(dataRows().length).toBe(SPOOLS.length));
+
+    const header = screen.getByRole('columnheader', { name: /^color$/i });
+    fireEvent.click(header);
+    await waitFor(() => expect(rowIndexOf('Red')).toBeLessThan(rowIndexOf('Black')));
+
+    fireEvent.click(header);
+    await waitFor(() => expect(rowIndexOf('Black')).toBeLessThan(rowIndexOf('Red')));
+
+    // Third click clears the sort, restoring the order the API returned.
+    fireEvent.click(header);
+    await waitFor(() => expect(rowIndexOf('Titan Gray')).toBe(0));
+  });
+
+  it('persists the colour sort across a remount', async () => {
+    const { unmount } = render(<InventoryPageRouter />);
+    await waitFor(() => expect(dataRows().length).toBe(SPOOLS.length));
+
+    fireEvent.click(screen.getByRole('columnheader', { name: /^color$/i }));
+    await waitFor(() =>
+      expect(vi.mocked(localStorage.setItem).mock.calls.some(
+        ([key, value]) => key === 'bambuddy-inventory-sort' && String(value).includes('rgba'),
+      )).toBe(true),
+    );
+
+    unmount();
+    vi.mocked(localStorage.getItem).mockImplementation((key) =>
+      key === 'bambuddy-inventory-sort' ? '{"column":"rgba","direction":"asc"}' : null,
+    );
+
+    render(<InventoryPageRouter />);
+    await waitFor(() => {
+      expect(rowIndexOf('Red')).toBeLessThan(rowIndexOf('Black'));
+    });
+  });
+});

+ 107 - 0
frontend/src/__tests__/utils/colors.test.ts

@@ -1,5 +1,7 @@
 import { describe, it, expect, beforeEach } from 'vitest';
 import {
+  colorFamily,
+  colorSortKey,
   hexToColorName,
   getColorName,
   resolveSpoolColorName,
@@ -113,3 +115,108 @@ describe('resolveSpoolColorName', () => {
     expect(resolveSpoolColorName('A99-Z9', '00000000')).toBe('Clear');
   });
 });
+
+describe('colorSortKey (#2729)', () => {
+  // Sorting the swatch column. Ascending puts Red first and the neutrals last.
+  const sortNames = (spools: { rgba: string | null; name: string }[]) =>
+    [...spools]
+      .sort((a, b) => colorSortKey(a.rgba).localeCompare(colorSortKey(b.rgba)))
+      .map((s) => s.name);
+
+  it('walks the rainbow, then browns, then neutrals light to dark', () => {
+    expect(
+      sortNames([
+        { rgba: '000000FF', name: 'black' },
+        { rgba: 'C8C8C8FF', name: 'silver' },
+        { rgba: 'FFFFFFFF', name: 'white' },
+        { rgba: '875718FF', name: 'brown' },
+        { rgba: '8B00FFFF', name: 'purple' },
+        { rgba: '6EE53CFF', name: 'green' },
+        { rgba: 'FF0000FF', name: 'red' },
+        { rgba: 'FF6A13FF', name: 'orange' },
+        { rgba: '56B7E6FF', name: 'cyan' },
+      ]),
+    ).toEqual(['red', 'orange', 'green', 'cyan', 'purple', 'brown', 'white', 'silver', 'black']);
+  });
+
+  it('keeps near-neutral greys out of the colours', () => {
+    // The regression the issue's own algorithm would ship. Measured on a real
+    // 30-spool inventory: a straight hue sort put Titan Gray (hue 210, sat
+    // 0.04) between Sky Blue and Purple, and 8B8889 (hue 340, sat 0.01)
+    // between Purple and Burgundy Red. Both must land with the neutrals.
+    expect(
+      sortNames([
+        { rgba: '56B7E6FF', name: 'sky blue' },
+        { rgba: '5F6367FF', name: 'titan gray' },
+        { rgba: '8B00FFFF', name: 'purple' },
+        { rgba: '8B8889FF', name: 'unnamed grey' },
+        { rgba: '951E23FF', name: 'burgundy red' },
+      ]),
+    ).toEqual(['burgundy red', 'sky blue', 'purple', 'unnamed grey', 'titan gray']);
+  });
+
+  it('groups browns together instead of splitting the oranges', () => {
+    // Dark Chocolate is hue 22 and would otherwise sit between two oranges.
+    expect(
+      sortNames([
+        { rgba: 'FF6A13FF', name: 'orange' },
+        { rgba: '4D3324FF', name: 'dark chocolate' },
+        { rgba: 'B39B84FF', name: 'iridium gold' },
+      ]),
+    ).toEqual(['orange', 'iridium gold', 'dark chocolate']);
+  });
+
+  it('sorts by hue within a family', () => {
+    expect(
+      sortNames([
+        { rgba: '875718FF', name: 'peanut brown' },
+        { rgba: 'B15533FF', name: 'terracotta' },
+        { rgba: '4D3324FF', name: 'dark chocolate' },
+      ]),
+    ).toEqual(['terracotta', 'dark chocolate', 'peanut brown']);
+  });
+
+  it('orders neutrals light to dark rather than by their meaningless hue', () => {
+    // 3A3A3A and 5F6367 are both Dark Gray; their hues (0 and 210) carry no
+    // information, so lightness has to decide.
+    expect(
+      sortNames([
+        { rgba: '3A3A3AFF', name: 'darker' },
+        { rgba: '5F6367FF', name: 'lighter' },
+      ]),
+    ).toEqual(['lighter', 'darker']);
+  });
+
+  it('sorts spools with no recorded colour last', () => {
+    expect(
+      sortNames([
+        { rgba: null, name: 'missing' },
+        { rgba: '', name: 'empty' },
+        { rgba: 'FF0000FF', name: 'red' },
+        { rgba: '000000FF', name: 'black' },
+      ]),
+    ).toEqual(['red', 'black', 'missing', 'empty']);
+  });
+
+  it('files a fully transparent colour as Clear, after the neutrals', () => {
+    expect(colorFamily('00000000')).toBe('Clear');
+    expect(sortNames([
+      { rgba: '00000000', name: 'clear' },
+      { rgba: '000000FF', name: 'black' },
+    ])).toEqual(['black', 'clear']);
+  });
+
+  it('produces equal keys for identical colours so sorting stays stable', () => {
+    expect(colorSortKey('FFFFFFFF')).toBe(colorSortKey('ffffffff'));
+  });
+});
+
+describe('colorFamily / hexToColorName agreement', () => {
+  it('names a colour with the same family it sorts under', () => {
+    // One classifier backs both, so the Color column can never sort a spool
+    // into a family the Color Name column disagrees with.
+    for (const hex of ['FF0000FF', '875718FF', '5F6367FF', 'FFFFFFFF', '00000000', 'zzz']) {
+      expect(hexToColorName(hex)).toBe(colorFamily(hex) ?? 'Unknown');
+    }
+  });
+});

+ 5 - 1
frontend/src/pages/InventoryPage.tsx

@@ -24,7 +24,7 @@ import { LocationsModal } from '../components/LocationsModal';
 import { BulkEditSpoolsModal } from '../components/BulkEditSpoolsModal';
 import { useToast } from '../contexts/ToastContext';
 import { useAuth } from '../contexts/AuthContext';
-import { resolveSpoolColorName } from '../utils/colors';
+import { colorSortKey, resolveSpoolColorName } from '../utils/colors';
 import { getCurrencySymbol } from '../utils/currency';
 import { formatDateInput, parseUTCDate, type DateFormat } from '../utils/date';
 import { formatSlotLabel } from '../utils/amsHelpers';
@@ -412,6 +412,10 @@ const columnSortValues: Record<string, (spool: InventorySpool, assignmentMap: Re
   material: (s) => (s.material || '').toLowerCase(),
   subtype: (s) => (s.subtype || '').toLowerCase(),
   color_name: (s) => (s.color_name || '').toLowerCase(),
+  // Sorts the swatch column itself (#2729). Multi-colour spools sort on their
+  // primary colour — extra_colors are gradient stops, and a spool has to sit in
+  // exactly one place in the list.
+  rgba: (s) => colorSortKey(s.rgba),
   brand: (s) => (s.brand || '').toLowerCase(),
   slicer_filament: (s) => (s.slicer_filament_name || s.slicer_filament || '').toLowerCase(),
   location: (s, am) => {

+ 115 - 11
frontend/src/utils/colors.ts

@@ -50,21 +50,56 @@ export function __resetColorCatalogForTests(): void {
 }
 
 /**
- * Convert hex color to basic color name using HSL analysis.
- * Used as fallback when hex is not in the runtime catalog.
+ * Colour families, in the order the Inventory's Color column sorts them (#2729).
+ *
+ * Chromatic families run in rainbow order with Brown after them (it is a dark
+ * orange by hue and would otherwise split the oranges in half), then the
+ * neutrals light-to-dark, then Clear.
  */
-export function hexToColorName(hex: string | null | undefined): string {
-  if (!hex || hex.length < 6) return 'Unknown';
+export const COLOR_FAMILY_ORDER = [
+  'Red',
+  'Orange',
+  'Yellow',
+  'Green',
+  'Cyan',
+  'Blue',
+  'Purple',
+  'Pink',
+  'Brown',
+  'White',
+  'Light Gray',
+  'Gray',
+  'Dark Gray',
+  'Black',
+  'Clear',
+] as const;
+
+export type ColorFamily = (typeof COLOR_FAMILY_ORDER)[number];
+
+/** Families whose hue carries no meaning — see ``colorSortKey``. */
+const ACHROMATIC_FAMILIES = new Set<ColorFamily>([
+  'White',
+  'Light Gray',
+  'Gray',
+  'Dark Gray',
+  'Black',
+  'Clear',
+]);
+
+interface Hsl {
+  h: number;
+  s: number;
+  l: number;
+}
+
+/** Parse 6/8-char hex (with or without '#') to HSL, or null if unparseable. */
+function hexToHsl(hex: string | null | undefined): Hsl | null {
+  if (!hex || hex.length < 6) return null;
   const cleanHex = hex.replace('#', '');
-  // Alpha=00 → fully transparent. Name it 'Clear' before falling through to
-  // RGB-based naming, otherwise #00000000 (Bambu's transparent code) would
-  // resolve to 'Black' via the HSL fallback (#1545).
-  if (cleanHex.length === 8 && cleanHex.substring(6, 8).toLowerCase() === '00') {
-    return 'Clear';
-  }
   const r = parseInt(cleanHex.substring(0, 2), 16);
   const g = parseInt(cleanHex.substring(2, 4), 16);
   const b = parseInt(cleanHex.substring(4, 6), 16);
+  if (Number.isNaN(r) || Number.isNaN(g) || Number.isNaN(b)) return null;
 
   const max = Math.max(r, g, b) / 255;
   const min = Math.min(r, g, b) / 255;
@@ -81,7 +116,29 @@ export function hexToColorName(hex: string | null | undefined): string {
     else if (max === gNorm) h = ((bNorm - rNorm) / d + 2) / 6;
     else h = ((rNorm - gNorm) / d + 4) / 6;
   }
-  h = h * 360;
+
+  return { h: h * 360, s, l };
+}
+
+/**
+ * Classify a hex colour into one of ``COLOR_FAMILY_ORDER``, or null if the
+ * value can't be parsed.
+ *
+ * This is the single source of truth for both the fallback colour *name* shown
+ * when a hex isn't in the catalog and the *order* the Color column sorts in, so
+ * the two cannot drift into disagreeing about what counts as brown or grey.
+ */
+export function colorFamily(hex: string | null | undefined): ColorFamily | null {
+  if (!hex || hex.length < 6) return null;
+  const cleanHex = hex.replace('#', '');
+  // Alpha=00 → fully transparent. Classify as 'Clear' before looking at RGB,
+  // otherwise #00000000 (Bambu's transparent code) would come out 'Black' (#1545).
+  if (cleanHex.length === 8 && cleanHex.substring(6, 8).toLowerCase() === '00') {
+    return 'Clear';
+  }
+  const hsl = hexToHsl(cleanHex);
+  if (!hsl) return null;
+  const { h, s, l } = hsl;
 
   if (l < 0.15) return 'Black';
   if (l > 0.85) return 'White';
@@ -103,6 +160,53 @@ export function hexToColorName(hex: string | null | undefined): string {
   return 'Pink';
 }
 
+/**
+ * Convert hex color to basic color name using HSL analysis.
+ * Used as fallback when hex is not in the runtime catalog.
+ */
+export function hexToColorName(hex: string | null | undefined): string {
+  return colorFamily(hex) ?? 'Unknown';
+}
+
+/**
+ * Sort key placing a spool colour in rainbow order (#2729, reporter @macwhiz).
+ *
+ * Returns a fixed-width string so it drops into the Inventory table's existing
+ * ``string | number`` comparison with no extra plumbing, ascending = Red first.
+ *
+ * The issue asked for a straight hue → saturation → lightness sort, which does
+ * not survive contact with a real inventory: a near-neutral still has a hue and
+ * it can be anything. Measured against a 30-spool inventory, Titan Gray
+ * (5F6367, hue 210°, saturation 0.04) landed between Sky Blue and Purple, and
+ * 8B8889 (hue 340°, saturation 0.01) landed between Purple and Burgundy Red.
+ * Black, white and silver happen to clump correctly — zero saturation sorts
+ * first within hue 0 — but anything a shade off neutral flies into the colours.
+ *
+ * So the family from ``colorFamily`` leads, and the continuous sort the issue
+ * asked for runs inside each family. Within a neutral family hue is discarded
+ * rather than sorted on, for the same reason it is not trusted to pick the
+ * family: ordering greys by 210° vs 340° is ordering them by noise. Neutrals go
+ * light-to-dark instead, matching the order of the families themselves.
+ *
+ * Unparseable or missing colours sort last in ascending order, so a spool with
+ * no colour recorded never leads the list.
+ */
+export function colorSortKey(rgba: string | null | undefined): string {
+  const family = colorFamily(rgba);
+  if (!family) return '99|0000|0000|0000';
+
+  const rank = String(COLOR_FAMILY_ORDER.indexOf(family)).padStart(2, '0');
+  const hsl = hexToHsl(rgba) ?? { h: 0, s: 0, l: 0 };
+  const pad4 = (n: number) => String(Math.round(n)).padStart(4, '0');
+
+  if (ACHROMATIC_FAMILIES.has(family)) {
+    // Lightness descending, so lighter shades lead within the family just as
+    // White leads Black across families. Hue is deliberately zeroed.
+    return `${rank}|0000|${pad4(1000 - hsl.l * 1000)}|${pad4(hsl.s * 1000)}`;
+  }
+  return `${rank}|${pad4(hsl.h * 10)}|${pad4(hsl.s * 1000)}|${pad4(hsl.l * 1000)}`;
+}
+
 /**
  * Get color name from hex color.
  * Looks up the runtime color catalog (backend-sourced), then falls back to HSL.

Tiedoston diff-näkymää rajattu, sillä se on liian suuri
+ 0 - 0
static/assets/index-BtvqfqPQ.js


+ 1 - 1
static/index.html

@@ -26,7 +26,7 @@
 
     <!-- Splash screens for iOS -->
     <link rel="apple-touch-startup-image" href="/img/android-chrome-512x512.png" />
-    <script type="module" crossorigin src="/assets/index-JnTmfHJF.js"></script>
+    <script type="module" crossorigin src="/assets/index-BtvqfqPQ.js"></script>
     <link rel="stylesheet" crossorigin href="/assets/index-D1VjN2fo.css">
   </head>
   <body>

Kaikkia tiedostoja ei voida näyttää, sillä liian monta tiedostoa muuttui tässä diffissä