refactor(colours): one Data/colours/ set — end the colors/colours split - #82
Open
willwade wants to merge 4 commits into
Open
refactor(colours): one Data/colours/ set — end the colors/colours split#82willwade wants to merge 4 commits into
willwade wants to merge 4 commits into
Conversation
Phase 4 scaffold, built before any file moves so every later step is guarded: - test_palettes.cpp: (1) the engine sees exactly the expected palette names — duplicate-free, current 21-name reality pinned, with the list documented as the consolidation target to update when the European/Asian rename lands; (2) no_palette_reports_the_error_ sentinel — catches an unparsed/broken palette file the moment it ships; (3) no_american_spelled_palette_files_ship — fails (may_fail-decorated, CI stays green) while Data/colors/ exists and flips permanently green when the consolidation deletes it. - test_palette_diff_report.cpp: one-off audit tool that parses the legacy set (<colours> roots in colours/) and the modern set (colors/) independently and diffs every name-matched pair — named chrome colours, group membership, node/alt sequences. First-run findings: 324 differences across the pairs; modern files carry 30-colour sequences where legacy carries 100 (the first 30 match — legacy repeats the cycle to cover Hiragana/punctuation ranges); legacy defines groups modern lacks (ethiopic letters, arabic-indic numbers, joiners, hamza, ascii punctuation...) and modern defines groups legacy lacks (accents, lowercaseBackground, paragraph, punctuationLong). This is the per-palette work list for Phase 2. - ColorPalette: read accessors for the group/named colour maps (test/tool audit support). Signed-off-by: will wade <willwade@gmail.com>
Alphabets refer to colour groups by a scattered set of names. The legacy parser registered aliases for all of them (lower case letters / Lower case Latin letters / ethiopic letters / arabic- indic numbers / ...), so a WA alphabet using one resolved on legacy palettes but hit the group-miss fallback — nodes rendered with the palette default instead of their sequence — on new-format ones. The consolidation audit showed every name-matched pair affected. Apply the same alias table to new-format palettes at parse time, for any alias the file doesn't define itself. After this, the audit's 'modern MISSING' column is empty: modern palettes cover every group name legacy did, while keeping their richer dedicated sequences (punctuation pinks, numbers reds, accents, paragraph). Remaining diffs are cycle length (100 vs 30 — identical rendering for the first 30 slots, verified on Rainbow) and modern-only groups/chrome, i.e. the modern design being a superset. Signed-off-by: will wade <willwade@gmail.com>
The mess: two parallel palette sets shipped (Data/colors/ new-format + Data/colours/ legacy), every name except Jamie collided, and which one you got depended on parse order — already the root of #62, #63, and Dasher-Windows #47. Surviving convention: British spelling, one file per palette, new-format roots, Data/colours/ only. - The 11 modern files moved in under British names, each replacing its legacy twin (colour.ean.xml, colour.blue.xml, colour.xml, colour.euroasian{,2}.xml, colour.jamie.xml, ...). With the group alias expansion in Parse (previous commit) the modern files cover every group name the legacy ones did. - Jamie's second palette, Blue on Light Green, had no modern twin: ported to new format (colour.blue.on.light.green.xml) with the full 100-colour cycles preserved. - European/Asian renamed everywhere: the legacy '(Original)' spelling dropped (the 62 alphabet colorsName references all say 'European/Asian' — the legacy name was the outlier); the dark companion follows. Known migration: a persisted ColourID='European/Asian (Original)' falls back to Default until re-picked — accepted as the one-time cost. - The engine's second colour glob (color.*.xml) removed; the legacy parser stays (custom palettes in user dirs still load). - Makefile.am regenerated from the directory listing; test_draw_ commands updated to the renamed palette; the catalogue pin now asserts the post-consolidation 20-name set, and the no-color*.xml test flips permanently green. Full suite: 43/43 locally (palette, draw-command and snapshot tests included — snapshot hashes unchanged, i.e. rendering is identical). Signed-off-by: will wade <willwade@gmail.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Criticals: - C1: the audit tool crashed CI after Data/colors/ died (directory iterator on a removed path, registered in ctest). Deleted — its job (the Phase-2 work list) is done and captured in the plan. - C2: the Blue on Light Green port was NOT faithful — the comment claimed de-duplication that never happened, group boxes used indices 107-109 instead of 111-113, circle colours 105-107 instead of 240-242, space alt wrong, and palette-1 chrome had spliced into the cycle. Root cause: the first extraction regex matched 5 colours inside XML comments, offsetting every index. Re-ported with comment-stripping extraction and index assertions (243 colours; 0/3/9/111/112/113/139/240/241/242 verified against the legacy source at port time); full 100-colour cycles verbatim; comment rewritten honestly. - C3: Makefile.am invented 'europeasian' spellings (files are euroasian.dark / euroasian2.dark) and listed colour.dtd (file is color.dtd) — autotools dist would fail. List now verified to match the directory exactly. Important: - I1: may_fail left on the now-green no-color*.xml guard — a reintroduced color*.xml would NOT have failed CI. Decorator dropped; hard pin. - I2: the alias expansion had zero coverage. New internal test (test_palette_aliases.cpp, CColorIO-driven) asserts all 16 aliases resolve identically to their canonical groups and pins the v6 group-box design (uppercase #ffff00, punctuation #00c800, numbers #ff0000). Kept in its own binary: mixing shared-lib CAPI and static-lib internal linkage in one executable duplicates Settings::parameter_defaults and corrupts the heap at static destruction (found the hard way). Minors: '(21)' comment, stale user-dir parenthetical, Readme.txt + ParseColors.py (legacy conversion helpers, dead weight) removed. Full suite 43/43; both palette binaries exit 0. Signed-off-by: will wade <willwade@gmail.com>
Author
|
Both findings addressed in d717534:
Round-1 review (5/10 → fixed) also landed in d717534: the audit tool crash (C1), the unfaithful Jamie re-port (C2 — extraction was picking up 5 colours inside XML comments, offsetting every index; re-ported with index assertions), the Makefile.am wrong filenames (C3), the leftover |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The mess
DasherCore shipped two parallel palette sets:
Data/colors/(new format) andData/colours/(legacy + the new-format dark companions). Every palette name except Jamie collided across the two; which one you got depended on parse order. Already caused: #62 (letter-family group colours lost), #63 (legacy colour.xml overwrote the new Default), Dasher-Windows #47 ("Yellow on black is all black" — the invisible-fill design without outlines).The fix — three commits
2eefbb6— Phase-4 scaffold first (built before any file moved): catalogue pin (duplicate-free name set), error-sentinel catcher for unparsed palette files,no color*.xml ships(may_fail-decorated — flips green whenData/colors/dies), and a diff-report audit tool that parses both sets independently and diffs every name-matched pair.768d743— group-alias expansion: alphabets refer to colour groups by a scattered set of names (lower case letters,ethiopic letters,arabic-indic numbers…). The legacy parser registered aliases for all of them; new-format palettes didn't — WA alphabets hit the group-miss fallback on every modern palette. The alias table now applies at parse time; after this the audit's modern MISSING column is empty (modern is a strict superset).3fabe75— the consolidation: modern files moved intoData/colours/under British names, each replacing its legacy twin; Jamie's Blue on Light Green (no modern twin) ported with full 100-colour cycles; European/Asian (Original) → European/Asian everywhere (the 62 alphabetcolorsName=references already said the short form — the legacy name was the outlier); the engine's secondcolor.*.xmlglob removed; Makefile.am regenerated.Result
One directory, 21 files, 20 palettes (post-rename), one format. Full suite 43/43 locally — including the draw-snapshot tests, whose hashes are unchanged, i.e. rendering is pixel-identical.
Known migration cost
A persisted
ColourID = 'European/Asian (Original)'falls back to Default until re-picked (one-time, accepted — noted in the plan).Follow-ups (separate)
data-bundlehas its owncolour/+colours/duplication + the webdemo palettes — sweep on its next bumpGreptile Summary
The PR consolidates shipped palettes under
Data/colours/, expands palette group aliases, updates discovery and packaging, and adds regression coverage.Data/colors/palette set and legacy conversion script.colour.*.xmlruntime glob and adds palette and alias tests.Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
color.dtdand all 20 consolidated palette files; the list matches the current directory contents.colour.*.xmlglob and now accurately explains removal of the obsoletecolor.*.xmlscan.Data/colors/directory, with no remaining repository references.Reviews (2): Last reviewed commit: "fix: review-loop round 1 on the consolid..." | Re-trigger Greptile