Sfoglia il codice sorgente

write a names file, not a definition file, and tell the two apart

A VIA definition belongs to the manufacturer. Writing one meant authoring
a menu description for someone else's keyboard and handing the user a
nested JSON array to type a name into: to set one name you had to find
menus[0].content[1].options[7][0] in a document the manufacturer wrote.
`generate` now writes a names file instead, which is flat — a channel, an
effect ID, a name — and merges over the definition per effect, so the
same command starts a board nobody published a file for and corrects a
name on a board they did.

That drops the shadowing hazard along with it. Refusing to write a VIA
definition over a built-in one existed only because the tool was writing
the manufacturer's kind of file; a names file cannot shadow anything but
its own names, and overriding one of them is the point.

A names file declares "kind": "names" and LoadDefinitionsDir skips a file
carrying it. That is not belt and braces: both kinds are JSON, both carry
the board's identifiers, and a names file has no menus, so without the
declaration it parses as a definition and takes the board's place — the
user directory is searched before the built-in ones. That was not
hypothetical. The first version of the tests had no names-directory
override and wrote two boards' names files and their notes into the
user's real per-user directory, and putting both temporary directories
on the same path hid the parsing bug behind it. Both are fixed, the
directory override is documented as something a new writable directory
needs in the same commit, and generateSetup now hands out two.

The shadowing refusal and its test are gone rather than adapted: they
guarded a hazard that no longer exists.
Paul-Dieter Klumpp 1 settimana fa
parent
commit
1ecc3201bd

+ 29 - 0
AGENTS.md

@@ -378,6 +378,35 @@ things about it are not to be undone:
   fills the names in. That is the honest state, and it is the one the board was
   already in.
 
+**A definition file is the manufacturer's, and a names file is yours.** They are
+two formats in two directories — `definitions/` and `names/` — and the rule behind
+that is that the tool writes no definition of its own. A VIA definition is a menu
+description for someone else's keyboard; authoring one meant handing the user a
+nested `menus[0].content[1].options[7][0]` to type a single name into, which is not
+a user interface. A names file is flat — a channel, an effect ID and a name — and
+`Names.Apply` merges it over whatever the definition said, per effect, so
+overriding one name does not mean restating the ones that were right.
+
+Four things about that split are not to be undone:
+
+- `Names.Apply` works on a **nil** catalog. A board with no definition is the case
+  the file exists for, and a nil dereference there is a panic, not an error.
+- Each name carries a `Source`: `SourceVendor` for a name the vendor published,
+  `SourceUser` for one a person typed. A name is the one value the tool cannot
+  read back off a keyboard, so the distinction has to be visible wherever a name
+  is printed.
+- A names file declares `"kind": "names"`, and `LoadDefinitionsDir` skips a file
+  carrying it. Without that a names file parses as a definition — it has the
+  board's identifiers and no menus — and shadows the board's real file, because
+  the user directory is searched before the built-in ones. A names file dropped
+  into `definitions/` is caught by the declaration and nowhere else.
+- A **new writable directory needs its own test override in the same commit.**
+  `names/` had none, and the tests wrote two boards' names files and their notes
+  into the user's real per-user directory. `forceNamesDir` is what stops it, and
+  `generateSetup` gives the names and definitions directories *different*
+  temporary directories, because one test directory for both hides exactly the bug
+  above.
+
 There is no shared database of effect names to be had, and do not plan around
 one. The closest thing is VIA's collection, `the-via/keyboards`, and a board's
 names live in an `id_qmk_rgb_matrix_effect` dropdown under `menus` — not under a

+ 48 - 13
README.md

@@ -58,6 +58,9 @@ there:
 | macOS | `~/Library/Application Support/qmk-rgb-tool/` |
 | Windows | `%AppData%\qmk-rgb-tool\` |
 
+Inside it, `definitions/` holds the files a vendor published, `names/` the names
+you wrote, and `profiles/` the profiles.
+
 One directory per kind, and not a search. A definition is written by
 `keyboard fetch` and read by every command, so a search path would mean the
 file a fetch produced is not the file the next command reads. A profile is
@@ -548,19 +551,51 @@ today.
 
 ### Naming Them Yourself
 
-`keyboard definitions generate` is what a board with no definition file is for.
-It asks the keyboard how many effect IDs each channel takes, writes a definition
-with one **unnamed** option per slot, and writes the spellings other keyboards'
-definitions use for the same numbers into a `.spotted.txt` note beside it.
+A VIA definition file belongs to the manufacturer. The tool reads the ones a
+vendor publishes and fetches the ones VIA's collection carries, and it writes
+none of its own: authoring a menu description for someone else's keyboard, and
+handing you a nested JSON array to type one name into, is not a user interface.
+So your names live in their own file, in a directory of their own:
+
+```jsonc
+{
+  "kind": "names",
+  "name": "Impact 80",
+  "vendorId": "0x36B0",
+  "productId": "0x309F",
+  "channels": {
+    "rgb_matrix": {
+      "7": "my_chevron"
+    }
+  }
+}
+```
 
-The generated definition names no effect on purpose. A name the tool wrote would
-be indistinguishable from the manufacturer's, because nothing in a file can be
-read back off a keyboard to check it — and a wrong name is worse than none, since
-the tool would then report an effect as set when it has set something else. An
-option with an empty name is a slot without a name, so the channel reports
-`0 effects` until a name is filled in, which is the honest state and the one the
-board was already in. Open the file, write a name into each `options` entry, and
-the channel resolves from then on.
+`keyboard definitions generate` writes one of those for the connected board: it
+asks the keyboard how many effect IDs each channel takes and writes a line per ID
+with nothing in it, plus a `.spotted.txt` note beside it carrying the spellings
+other keyboards' definitions use for the same numbers. Open the file, write a name
+on a line, and the channel resolves it.
+
+Three things about that file:
+
+- **A name you write replaces the vendor's for that one effect ID** and leaves
+  every other entry alone, so the same command starts a board nobody published a
+  file for *and* corrects a name on a board they did. A board with no definition
+  is exactly the board this is for: the file the manufacturer would have
+  published is the one missing, not the names.
+- **A name is the one value the tool cannot read back off a keyboard.** Every
+  value it writes it reads back and reports what the keyboard actually applied;
+  a name has no register. So a name the tool chose would be indistinguishable
+  from one the vendor published, and nothing in a file can be checked. A name you
+  write is a fact about you and is reported as such.
+- **An empty value states a slot, not a name**, and is skipped — so a generated
+  file reports `0 effects` until you fill it in, which is the honest state and the
+  one the board was already in.
+
+The file declares `"kind": "names"`. Both kinds of file are JSON and both carry
+the board's identifiers, so a names file without that would parse as a definition
+and take the board's place, and the definitions directory is searched first.
 
 ### A Keyboard Running Vial
 
@@ -1004,7 +1039,7 @@ board", and that file is gone, so the field would have been unanswerable:
 | `qmk-rgb-tool effect <zone>`           | With no name, list the effects that zone has; `effect all` lists every channel |
 | `qmk-rgb-tool keyboard fetch`         | Download the VIA definition for the connected keyboard. A definition already stored for that board is not replaced, because it may be one you have edited; `--force` overwrites it |
 | `qmk-rgb-tool keyboard definitions`   | List every definition in use, from the per-user directory and built into the binary, each marked with which it is |
-| `qmk-rgb-tool keyboard definitions generate` | Ask the keyboard how many effect IDs each channel takes and write a definition with one **unnamed** option per slot, plus a `.spotted.txt` note naming the spellings other keyboards' definitions use for the same numbers. The names are yours to write in; see [Where Effect Names Cannot Come From](#where-effect-names-cannot-come-from) |
+| `qmk-rgb-tool keyboard definitions generate` | Ask the keyboard how many effect IDs each channel takes and write a names file in `names/` with a line per ID and nothing in it, plus a `.spotted.txt` note naming the spellings other keyboards' definitions use for the same numbers. A name you write replaces the vendor's for that one effect and leaves the rest alone; see [Where Effect Names Cannot Come From](#where-effect-names-cannot-come-from) |
 | `qmk-rgb-tool brightness <zone> <val>` | Set brightness (0–255) on the named zones, verified by read-back |
 | `qmk-rgb-tool speed <zone> <val>`     | Set effect speed (0–255) on the named zones, verified by read-back; on `logo` and `side` only 0, 1 and 4 are reachable |
 | `qmk-rgb-tool color <zone> <hex>`     | Set color (e.g. `ff0000`) on the named zones |

+ 76 - 6
cmd/qmk-rgb-tool/catalog.go

@@ -2,6 +2,9 @@ package main
 
 import (
 	"fmt"
+	"os"
+	"path/filepath"
+	"strings"
 	"sync"
 
 	"netdome.biz/paul/qmk-rgb/definitions"
@@ -64,7 +67,7 @@ func resolveCatalog(target targetDeviceData) (*intrgb.Catalog, string, error) {
 				definitionFlag, def.Name, def.VendorID, def.ProductID,
 				target.Device.VendorID, target.Device.ProductID)
 		}
-		return def.Catalog, def.Path, nil
+		return withNames(def.Catalog, def.VendorID, def.ProductID), def.Path, nil
 	}
 
 	catalog, source, err := resolveCatalogFor(target.Device.VendorID, target.Device.ProductID)
@@ -86,13 +89,14 @@ func resolveCatalogFor(vendorID, productID uint16) (*intrgb.Catalog, string, err
 	}
 
 	if def := intrgb.FindDefinition(candidateDefinitions(), vendorID, productID); def != nil {
-		return def.Catalog, def.Path, nil
+		return withNames(def.Catalog, def.VendorID, def.ProductID), def.Path, nil
 	}
 
-	// No definition for this board, not in a directory and not built in: the
-	// keyboard holds numbers, not names, and a board with no names is driven
-	// through raw IDs.
-	return nil, "", nil
+	// No definition for this board, not in a directory and not built in. A names
+	// file may still hold names for it, and that is a board somebody wrote them
+	// for; a board with neither holds numbers rather than names and is driven
+	// through raw effect IDs.
+	return withNames(nil, vendorID, productID), "", nil
 }
 
 // loadedDefinition returns the definition file for a board, from the file
@@ -165,3 +169,69 @@ func ensureDefinitionsDir() (string, error) {
 	}
 	return dir, nil
 }
+
+// NamesDir is the name of the directory a user's own effect names are kept in. It
+// is not the definitions directory, because the two hold different things and only
+// one of them is the manufacturer's: a definition file is what a board's vendor
+// published, and a names file is what a person wrote. Merging them into one file
+// would make the tool author a document in someone else's name, and it would make
+// every name the user supplies indistinguishable from one the vendor published.
+const namesDir = "names"
+
+// namesPath is where a user's names for effect IDs live, and where a command that
+// writes one puts it.
+func namesPath() string {
+	if namesDirOverride != "" {
+		return namesDirOverride
+	}
+	return filepath.Join(userDataDir(), namesDir)
+}
+
+// ensureNamesDir returns the names directory, creating it if it is not there.
+func ensureNamesDir() (string, error) {
+	dir, err := ensureDataDir(namesPath())
+	if err != nil {
+		return "", fmt.Errorf("create %s: %w", namesPath(), err)
+	}
+	return dir, nil
+}
+
+// candidateNames returns the names files in the per-user directory. A directory
+// that is not there yet is not an error: a user who has written none has none.
+func candidateNames() []*intrgb.Names {
+	entries, err := os.ReadDir(namesPath())
+	if err != nil {
+		return nil
+	}
+	var out []*intrgb.Names
+	for _, entry := range entries {
+		if entry.IsDir() || !strings.HasSuffix(entry.Name(), ".json") {
+			continue
+		}
+		names, err := intrgb.LoadNamesFile(filepath.Join(namesPath(), entry.Name()))
+		if err != nil {
+			// A file the user wrote and that does not load is worth saying, but
+			// not from here: a lookup is not the place to report it, and a broken
+			// file for one board must not stop a name resolving for another. The
+			// `names` command lists what is there and reports the broken one.
+			continue
+		}
+		out = append(out, names)
+	}
+	return out
+}
+
+// withNames returns the catalog for a board with the user's names over the top of
+// whatever the definition said, and nil when there is neither. A names file is
+// consulted for every board, whether or not a definition exists, because the two
+// are independent: a user may override one name of a board whose definition the
+// tool has, and name a board whose definition nobody published.
+func withNames(catalog *intrgb.Catalog, vendorID, productID uint16) *intrgb.Catalog {
+	for _, names := range candidateNames() {
+		if !names.Matches(vendorID, productID) {
+			continue
+		}
+		return names.Apply(catalog)
+	}
+	return catalog
+}

+ 5 - 2
cmd/qmk-rgb-tool/datadir.go

@@ -32,11 +32,14 @@ const dataDirName = "qmk-rgb-tool"
 // test cannot rely on which platform it runs on.
 var userConfigDir = os.UserConfigDir
 
-// The two overrides short-circuit the lookup, which is how the tests point the
-// lookup at a temporary directory. Empty means resolve it.
+// The overrides short-circuit the lookup, which is how the tests point a lookup
+// at a temporary directory. Empty means resolve it. Each directory has its own
+// because the directories are separate, and a test that exercises one of them is
+// not thereby exercising another.
 var (
 	profilesDirOverride    string
 	definitionsDirOverride string
+	namesDirOverride       string
 )
 
 // userDataDir is this tool's directory under the platform's configuration

+ 109 - 56
cmd/qmk-rgb-tool/definition_generate.go

@@ -1,7 +1,6 @@
 package main
 
 import (
-	"encoding/json"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -9,6 +8,7 @@ import (
 	"strings"
 
 	"github.com/spf13/cobra"
+	intdevice "netdome.biz/paul/qmk-rgb/internal/device"
 	intrgb "netdome.biz/paul/qmk-rgb/internal/rgb"
 	"netdome.biz/paul/qmk-rgb/internal/spotted"
 	"netdome.biz/paul/qmk-rgb/internal/via"
@@ -65,30 +65,37 @@ const (
 )
 
 // NewKeyboardDefinitionsGenerateCmd is `keyboard definitions generate`. It writes
-// a definition for a board that has none, with the effect slots the keyboard
-// reports and no names in them, plus a note beside it saying which spellings
+// a names file for the connected board: one line per effect ID the keyboard
+// reports, with no names in them, plus a note beside it saying which spellings
 // other keyboards' definitions use for the same numbers.
 //
-// The names are the user's to write, and the command does not write them into
-// the definition: a name it put there would read as the manufacturer's own, and
-// nothing in a file can be read back off a keyboard to check it. An option with
-// an empty name is a slot without a name, so the channel reports no effects
-// until one is filled in — which is the honest state, and the same one a board
-// with no definition reports.
+// It writes a names file and not a definition file, because a definition belongs
+// to the manufacturer. Writing one meant authoring a menu description for
+// someone else's keyboard and handing the user a nested JSON array to type a
+// single name into, and a name the tool put in a file it had written itself would
+// read as the manufacturer's own. A names file is flat — a channel, an effect ID
+// and a name — and it overrides the definition where it has one, so the same
+// command starts a board nobody published a file for and corrects a name on a
+// board they did.
+//
+// The names are the user's to write. A name is the one value this tool cannot
+// read back off a keyboard, so a name the tool chose would be indistinguishable
+// from one the board's vendor published. An empty name states a slot without
+// naming it, and is skipped, so the channel reports no effects until one is
+// filled in.
 func NewKeyboardDefinitionsGenerateCmd() *cobra.Command {
 	cmd := &cobra.Command{
 		Use:   "generate",
-		Short: "Write a definition for the connected keyboard, with the effect names left to you",
+		Short: "Write a names file for the connected keyboard, with the effect names left to you",
 		Long: experimentalVial + ": the Vial path below has not been run against a Vial keyboard.\n\n" +
 			"Ask the keyboard how many effect IDs each of its lighting channels takes, then\n" +
-			"write a definition file with one unnamed option per ID, and a note beside it\n" +
+			"write a names file with one line per ID and no name in it, and a note beside it\n" +
 			"saying which spellings other keyboards' definitions use for the same numbers.\n\n" +
-			"The names are yours to write: open the file and put one in each options entry,\n" +
-			"and the channel starts resolving them. Nothing in a file can be read back off a\n" +
-			"keyboard, so a name the tool wrote would be indistinguishable from the\n" +
-			"manufacturer's, and a wrong name is worse than none: the tool reports an effect\n" +
-			"as unknown and set by number until you fill it in.\n\n" +
-			"A definition already stored for this keyboard is not replaced, because it is\n" +
+			"The names are yours to write: open the file and put one on each line, and the\n" +
+			"channel starts resolving them. A name you write for an ID the board's definition\n" +
+			"also names replaces that one name and leaves the rest alone, so this also corrects\n" +
+			"a board whose definition this tool has.\n\n" +
+			"A names file already stored for this keyboard is not replaced, because it is\n" +
 			"probably the one you wrote. Edit it, or pass --force.\n\n" +
 			"A keyboard running Vial firmware is asked for its rgb_matrix effect IDs instead of\n" +
 			"being written to, which is the better answer, and it numbers them in its own\n" +
@@ -113,31 +120,11 @@ func runKeyboardDefinitionsGenerate(cmd *cobra.Command, args []string) error {
 		return fmt.Errorf("this build cannot read a keyboard's effect range")
 	}
 
-	stored := storedDefinitionsFor(definitionsPath(), target.Device.VendorID, target.Device.ProductID)
+	stored := storedNamesFor(target.Device.VendorID, target.Device.ProductID)
 	if len(stored) > 0 && !generateForce {
-		return fmt.Errorf("a definition for %s (0x%04X/0x%04X) is already at %s; it is probably the one "+
-			"you wrote, so generate does not replace it, use --force to overwrite it or edit that file",
-			stored[0].Name, target.Device.VendorID, target.Device.ProductID, describeDataDir(stored[0].Path))
-	}
-
-	// The built-in set is checked as well, and for a different reason than the
-	// directory above. A definition the binary carries is the vendor's own file and
-	// the user directory shadows it, so a scaffold written for such a board would
-	// take it from the names it has to none — a loss the generated file could not
-	// report, because the file is the one that is read. `fetch` has the same
-	// shadowing and is allowed it, because a fetched file is the vendor's file too
-	// and carries more than a scaffold does.
-	if !generateForce {
-		for _, def := range builtInDefinitions() {
-			if !def.Matches(target.Device.VendorID, target.Device.ProductID) {
-				continue
-			}
-			return fmt.Errorf("this binary carries a definition for %s (0x%04X/0x%04X) that names %d "+
-				"effects, and a file in the definitions directory would shadow it; a generated one names "+
-				"none, so the board would go from %d names to no effect names at all, use --force to do "+
-				"that anyway, or edit the built-in file's copy in the definitions directory",
-				def.Name, def.VendorID, def.ProductID, namedEffectCount(def), namedEffectCount(def))
-		}
+		return fmt.Errorf("a names file for 0x%04X/0x%04X is already at %s; it is probably the one you "+
+			"wrote, so generate does not replace it, use --force to overwrite it or edit that file",
+			target.Device.VendorID, target.Device.ProductID, describeDataDir(stored[0].Path))
 	}
 
 	if len(channels) == 0 {
@@ -182,24 +169,16 @@ func runKeyboardDefinitionsGenerate(cmd *cobra.Command, args []string) error {
 		writeCandidates(notes, ch, ids, from)
 	}
 
-	dir, err := ensureDefinitionsDir()
+	dir, err := ensureNamesDir()
 	if err != nil {
 		return err
 	}
-	base := definitionFileName(&fetchedDefinition{
-		Definition: &intrgb.Definition{
-			Name:      name,
-			VendorID:  target.Device.VendorID,
-			ProductID: target.Device.ProductID,
-		},
-	})
-
-	encoded, err := json.MarshalIndent(doc, "", "  ")
+	path := filepath.Join(dir, namesFileName(target.Device))
+	body, err := renderNamesFile(name, target.Device, slots)
 	if err != nil {
 		return err
 	}
-	path := filepath.Join(dir, base)
-	if err := os.WriteFile(path, append(encoded, '\n'), 0o644); err != nil {
+	if err := os.WriteFile(path, body, 0o644); err != nil {
 		return fmt.Errorf("write %s: %w", path, err)
 	}
 
@@ -212,9 +191,9 @@ func runKeyboardDefinitionsGenerate(cmd *cobra.Command, args []string) error {
 	if len(stored) > 0 {
 		verb = "Replaced"
 	}
-	fmt.Fprintf(cmd.OutOrStdout(), "%s a definition for %s (%s/%s) to %s\n",
-		verb, doc.Name, doc.VendorID, doc.ProductID, describeDataDir(path))
-	fmt.Fprintf(cmd.OutOrStdout(), "Every effect is an unnamed option. Open the file and write a name into each one:\n")
+	fmt.Fprintf(cmd.OutOrStdout(), "%s a names file for %s (%04X/%04X) to %s\n",
+		verb, name, target.Device.VendorID, target.Device.ProductID, describeDataDir(path))
+	fmt.Fprintf(cmd.OutOrStdout(), "Every effect is a line with no name in it. Open the file and write one in:\n")
 	for _, ch := range channels {
 		ids, ok := slots[ch]
 		if !ok {
@@ -224,6 +203,9 @@ func runKeyboardDefinitionsGenerate(cmd *cobra.Command, args []string) error {
 			ch.Subsystem(), len(ids), describeSlotRange(ids), sources[ch])
 	}
 	fmt.Fprintf(cmd.OutOrStdout(), "\nWhat other keyboards call the same numbers is in %s\n", describeDataDir(notePath))
+	if def := loadedDefinition(target.Device.VendorID, target.Device.ProductID); def != nil {
+		fmt.Fprintf(cmd.OutOrStdout(), "\nThis board also has a definition naming %d effects; a name you write here\nreplaces that one name and leaves the others alone.\n", namedEffectCount(def))
+	}
 	return nil
 }
 
@@ -430,3 +412,74 @@ func namedEffectCount(def *intrgb.Definition) int {
 	}
 	return total
 }
+
+// storedNamesFor returns the names files already in the per-user directory that
+// describe a board.
+func storedNamesFor(vendorID, productID uint16) []*intrgb.Names {
+	var out []*intrgb.Names
+	for _, names := range candidateNames() {
+		if names.Matches(vendorID, productID) {
+			out = append(out, names)
+		}
+	}
+	return out
+}
+
+// namesFileName names a names file after the board, the same way a definition
+// file is named so that a directory of either is readable.
+func namesFileName(device intdevice.Device) string {
+	var b strings.Builder
+	for _, r := range strings.ToLower(device.Name) {
+		switch {
+		case r >= 'a' && r <= 'z', r >= '0' && r <= '9':
+			b.WriteRune(r)
+		default:
+			b.WriteRune('_')
+		}
+	}
+	slug := strings.Trim(b.String(), "_")
+	if slug == "" {
+		slug = "keyboard"
+	}
+	return fmt.Sprintf("%s_0x%04X_0x%04X.json", slug, device.VendorID, device.ProductID)
+}
+
+// renderNamesFile writes the file as a person edits it: one effect ID per line,
+// with an empty name to fill in. A generated file therefore has no names in it
+// and the board reports none, which is the state it was in before the file
+// existed. The keys are written in ID order because that is the order a person
+// works through them in, and the channels in channel order for the same reason.
+func renderNamesFile(name string, device intdevice.Device, slots map[via.Channel][]uint16) ([]byte, error) {
+	var b strings.Builder
+	fmt.Fprintf(&b, "{\n")
+	// The kind is what tells a names file apart from a definition file. Both are
+	// JSON, both carry the board's identifiers, and a names file in a definitions
+	// directory would otherwise parse as a definition and take the board's place.
+	fmt.Fprintf(&b, "  \"kind\": %q,\n", intrgb.NamesFileKind)
+	fmt.Fprintf(&b, "  \"name\": %q,\n", name)
+	fmt.Fprintf(&b, "  \"vendorId\": \"0x%04X\",\n", device.VendorID)
+	fmt.Fprintf(&b, "  \"productId\": \"0x%04X\",\n", device.ProductID)
+	fmt.Fprintf(&b, "  \"channels\": {\n")
+	written := 0
+	for _, ch := range via.LightingChannels {
+		ids, ok := slots[ch]
+		if !ok || len(ids) == 0 {
+			continue
+		}
+		if written > 0 {
+			fmt.Fprintf(&b, ",\n")
+		}
+		written++
+		fmt.Fprintf(&b, "    %q: {\n", ch.Subsystem())
+		for i, id := range ids {
+			comma := ","
+			if i == len(ids)-1 {
+				comma = ""
+			}
+			fmt.Fprintf(&b, "      \"%d\": \"%s\"%s\n", id, "", comma)
+		}
+		fmt.Fprintf(&b, "    }")
+	}
+	fmt.Fprintf(&b, "\n  }\n}\n")
+	return []byte(b.String()), nil
+}

+ 179 - 260
cmd/qmk-rgb-tool/definition_generate_test.go

@@ -1,10 +1,8 @@
 package main
 
 import (
-	"encoding/json"
 	"os"
 	"path/filepath"
-	"strconv"
 	"strings"
 	"testing"
 
@@ -12,17 +10,18 @@ import (
 	intvia "netdome.biz/paul/qmk-rgb/internal/via"
 )
 
-// generateStub is a keyboard that answers an effect range, so the generated
-// scaffold can be checked without hardware. The Vial fields are what a keyboard
-// running Vial firmware would answer; zero versions read as a keyboard that is
-// not Vial.
+// generateStub is a keyboard that answers an effect range, so a generated names
+// file can be checked without hardware. The Vial fields are what a keyboard
+// running Vial firmware would answer; a zero version reads as one that is not
+// Vial.
 type generateStub struct {
 	tops    map[intvia.Channel]int
 	vial    bool
 	vialIDs []uint16
-	vialErr error
 }
 
+func (g generateStub) EffectTop(ch intvia.Channel) (int, error) { return g.tops[ch], nil }
+
 func (g generateStub) VialVersion() (uint32, bool, error) {
 	if !g.vial {
 		return 0, false, nil
@@ -30,16 +29,7 @@ func (g generateStub) VialVersion() (uint32, bool, error) {
 	return 6, true, nil
 }
 
-func (g generateStub) VialEffectIDs() ([]uint16, error) {
-	if g.vialErr != nil {
-		return nil, g.vialErr
-	}
-	return g.vialIDs, nil
-}
-
-func (g generateStub) EffectTop(ch intvia.Channel) (int, error) {
-	return g.tops[ch], nil
-}
+func (g generateStub) VialEffectIDs() ([]uint16, error) { return g.vialIDs, nil }
 
 func (g generateStub) GetValue(intvia.Channel, uint8) ([]byte, error) { return []byte{0}, nil }
 
@@ -51,16 +41,15 @@ func (g generateStub) DetectChannels() ([]intvia.Channel, error) { return nil, n
 
 func (g generateStub) Close() error { return nil }
 
-// stubGenerateTarget makes the connected keyboard a fixed one. The identifiers
-// are a parameter because a board the binary carries a definition for is the
-// case `generate` has to refuse, and a stub for the wrong board would test
-// nothing.
-func stubGenerateTarget(t *testing.T, vendorID, productID uint16, channels []intvia.Channel, tops map[intvia.Channel]int) {
+// stubGenerateTarget makes the connected keyboard a fixed one. The identifiers are
+// a parameter because a board the binary carries a definition for is a different
+// case from one it does not, and a stub for the wrong board would test nothing.
+func stubGenerateTarget(t *testing.T, vendorID, productID uint16, channels []intvia.Channel, stub generateStub) {
 	t.Helper()
 	original := openTarget
 	t.Cleanup(func() { openTarget = original })
 	openTarget = func(string) (rgbProtocol, targetDeviceData, []intvia.Channel, error) {
-		return generateStub{tops: tops}, stubTargetData(vendorID, productID), channels, nil
+		return stub, stubTargetData(vendorID, productID), channels, nil
 	}
 }
 
@@ -70,181 +59,53 @@ func forceGenerateRestore(t *testing.T) func() {
 	return func() { generateForce = original }
 }
 
-// The whole point of the command: a generated file names no effect, so a board
-// with a generated definition reports the same "no names" it reported before one
-// existed. A scaffold that claimed names would be a guess the tool could not
-// read back off the keyboard.
-func TestGeneratedDefinitionNamesNoEffectUntilTheUserFillsItIn(t *testing.T) {
-	dir := t.TempDir()
-	t.Cleanup(definitionFlagRestore(t))
-	t.Cleanup(forceDefinitionsDir(t, dir))
-	t.Cleanup(forceGenerateRestore(t))
-	stubGenerateTarget(t, 0x1234, 0x5678, []intvia.Channel{intvia.ChannelRgbMatrix}, map[intvia.Channel]int{intvia.ChannelRgbMatrix: 45})
-
-	cmd := NewKeyboardDefinitionsGenerateCmd()
-	var out, errOut strings.Builder
-	cmd.SetOut(&out)
-	cmd.SetErr(&errOut)
-
-	if err := cmd.Execute(); err != nil {
-		t.Fatalf("definitions generate error = %v (stderr %q)", err, errOut.String())
-	}
-
-	path := onlyDefinition(t, dir)
-	def, err := intrgb.ParseDefinition(path, []byte(mustRead(t, path)))
-	if err != nil {
-		t.Fatalf("ParseDefinition(%s) error = %v; the generated file has to load", path, err)
-	}
-	if got := len(def.Catalog.Effects(intvia.ChannelRgbMatrix)); got != 0 {
-		t.Errorf("effects on rgb_matrix = %d, want 0 until the names are written in", got)
-	}
-	// The slots are still there, which is the other half: the file states which
-	// IDs exist and leaves the naming to the user.
-	var file struct {
-		Menus []struct {
-			Content []struct {
-				Label   string `json:"label"`
-				Type    string `json:"type"`
-				Content []any  `json:"content"`
-				Options []any  `json:"options"`
-			} `json:"content"`
-		} `json:"menus"`
-	}
-	if err := json.Unmarshal([]byte(mustRead(t, path)), &file); err != nil {
-		t.Fatalf("unmarshal generated file: %v", err)
-	}
-	var options []any
-	for _, menu := range file.Menus {
-		for _, entry := range menu.Content {
-			if entry.Type == "dropdown" {
-				options = entry.Options
-			}
-		}
-	}
-	if len(options) != 46 {
-		t.Fatalf("effect options = %d, want 46, one per ID from 0 to 45", len(options))
-	}
-	for i, option := range options {
-		pair, ok := option.([]any)
-		if !ok || len(pair) != 2 {
-			t.Fatalf("option %d = %v, want a name and a number", i, option)
-		}
-		if name, _ := pair[0].(string); name != "" {
-			t.Errorf("option %d is named %q, want a slot with no name", i, name)
-		}
-		if number, _ := pair[1].(float64); int(number) != i {
-			t.Errorf("option %d carries ID %v, want the slot numbered %d", i, pair[1], i)
-		}
-	}
-
-	// Every control has to be addressed by a value key the parser recognises. A
-	// key built by trimming a suffix and appending without the underscore is
-	// `id_qmk_rgb_matrixbrightness`, which names nothing, and nothing in the
-	// output above would say so.
-	wantKeys := map[string]bool{
-		"id_qmk_rgb_matrix_brightness":   false,
-		"id_qmk_rgb_matrix_effect":       false,
-		"id_qmk_rgb_matrix_effect_speed": false,
-	}
-	for _, menu := range file.Menus {
-		for _, entry := range menu.Content {
-			key, _ := entry.Content[0].(string)
-			if _, ok := wantKeys[key]; !ok {
-				t.Errorf("control %q is addressed by %q, which is not a VIA value key", entry.Label, key)
-				continue
-			}
-			wantKeys[key] = true
-		}
-	}
-	for key, found := range wantKeys {
-		if !found {
-			t.Errorf("no control addresses %q", key)
-		}
-	}
+// forceNamesDir points the names directory at a test directory. Without it a test
+// that writes a names file writes into the user's real data directory, which is
+// what happened when this override did not exist yet.
+func forceNamesDir(t *testing.T, dir string) func() {
+	t.Helper()
+	original := namesDirOverride
+	namesDirOverride = dir
+	return func() { namesDirOverride = original }
 }
 
-// A definition a user has written is the one command they would have run to write
-// it, so it must survive a second run of the command.
-func TestGenerateDoesNotReplaceAStoredDefinition(t *testing.T) {
-	dir := t.TempDir()
+// generateSetup points every writable directory at one temporary directory, so a
+// test cannot reach the user's.
+// generateSetup points the writable directories at temporary ones and returns the
+// names directory. They are two directories on purpose: a names file in the
+// definitions directory parses as a definition — it carries the board's
+// identifiers and no menus — and takes the board's place, which is exactly what
+// TestNamesFileInTheDefinitionsDirectoryIsNotADefinition is about.
+func generateSetup(t *testing.T) string {
+	t.Helper()
+	names := t.TempDir()
 	t.Cleanup(definitionFlagRestore(t))
-	t.Cleanup(forceDefinitionsDir(t, dir))
+	t.Cleanup(forceDefinitionsDir(t, t.TempDir()))
+	t.Cleanup(forceNamesDir(t, names))
 	t.Cleanup(forceGenerateRestore(t))
-	stubGenerateTarget(t, 0x1234, 0x5678, []intvia.Channel{intvia.ChannelRgbMatrix}, map[intvia.Channel]int{intvia.ChannelRgbMatrix: 45})
-
-	written := `{"name":"Test Board","vendorId":"0x1234","productId":"0x5678","menus":[],"note":"hand written"}`
-	if err := os.WriteFile(filepath.Join(dir, "test_board.json"), []byte(written), 0o644); err != nil {
-		t.Fatal(err)
-	}
-
-	cmd := NewKeyboardDefinitionsGenerateCmd()
-	var out, errOut strings.Builder
-	cmd.SetOut(&out)
-	cmd.SetErr(&errOut)
-
-	err := cmd.Execute()
-	if err == nil {
-		t.Fatal("definitions generate = nil error, want a refusal to replace a stored definition")
-	}
-	if !strings.Contains(err.Error(), "--force") {
-		t.Errorf("error = %q, want it to offer --force", err)
-	}
-	if got := mustRead(t, filepath.Join(dir, "test_board.json")); got != written {
-		t.Errorf("stored file = %q, want it untouched (%q)", got, written)
-	}
+	return names
 }
 
-// The note beside the file is what carries the names, because a JSON file cannot
-// hold them: the tool would have to read comments and VIA's parser would reject
-// them. It has to say who wrote what, or a name is a guess wearing a count.
-func TestGeneratedNoteCarriesTheSpellingsAndWhoWroteThem(t *testing.T) {
-	dir := t.TempDir()
-	t.Cleanup(definitionFlagRestore(t))
-	t.Cleanup(forceDefinitionsDir(t, dir))
-	t.Cleanup(forceGenerateRestore(t))
-	stubGenerateTarget(t, 0x1234, 0x5678, []intvia.Channel{intvia.ChannelRgbMatrix}, map[intvia.Channel]int{intvia.ChannelRgbMatrix: 45})
-
+func runGenerate(t *testing.T) (string, string) {
+	t.Helper()
 	cmd := NewKeyboardDefinitionsGenerateCmd()
 	var out, errOut strings.Builder
 	cmd.SetOut(&out)
 	cmd.SetErr(&errOut)
 	if err := cmd.Execute(); err != nil {
-		t.Fatalf("definitions generate error = %v", err)
-	}
-
-	note := mustRead(t, strings.TrimSuffix(onlyDefinition(t, dir), ".json")+spottedNoteSuffix)
-	if !strings.Contains(note, "rgb_matrix (channel 3), 46 slots, IDs 0 to 45") {
-		t.Errorf("note = %q, want it to name the channel, the count and the ID range", note)
-	}
-	if !strings.Contains(note, string(slotsMeasured)) {
-		t.Errorf("note = %q, want it to say where the slots came from", note)
-	}
-	// The measurement behind the names: a spelling, how many boards wrote it, and
-	// which manufacturer most of them were.
-	if !strings.Contains(note, "rainbow_moving_chevron") {
-		t.Errorf("note = %q, want the spellings other definitions use", note)
-	}
-	if !strings.Contains(note, "boards:") {
-		t.Errorf("note = %q, want a count of boards next to every name", note)
-	}
-	if !strings.Contains(note, "keychron") {
-		t.Errorf("note = %q, want the manufacturer behind most of a name's spellings", note)
-	}
-	// Effect 23 is where the collection disagrees about what the number even is,
-	// so the note has to show the runner-up rather than pick a winner.
-	if !strings.Contains(note, "ID 23 ") {
-		t.Errorf("note = %q, want a line for ID 23", note)
+		t.Fatalf("definitions generate error = %v (stderr %q)", err, errOut.String())
 	}
+	return out.String(), errOut.String()
 }
 
-func onlyDefinition(t *testing.T, dir string) string {
+func onlyNamesFile(t *testing.T, dir string) string {
 	t.Helper()
 	matches, err := filepath.Glob(filepath.Join(dir, "*.json"))
 	if err != nil {
 		t.Fatal(err)
 	}
 	if len(matches) != 1 {
-		t.Fatalf("definitions dir = %v, want one JSON file", matches)
+		t.Fatalf("names dir = %v, want one file", matches)
 	}
 	return matches[0]
 }
@@ -258,93 +119,123 @@ func mustRead(t *testing.T, path string) string {
 	return string(data)
 }
 
-// A generated scaffold names no effect, so writing one for a board the binary
-// carries a definition for takes that board from 46 named effects to none — and
-// the file in the user directory is the one that is read. This is the whole
-// reason the built-in set is checked and not only the directory.
-func TestGenerateDoesNotShadowABuiltInDefinition(t *testing.T) {
-	dir := t.TempDir()
-	t.Cleanup(definitionFlagRestore(t))
-	t.Cleanup(forceDefinitionsDir(t, dir))
-	t.Cleanup(forceGenerateRestore(t))
-	// The Impact 80, whose definition is built into the binary.
-	stubGenerateTarget(t, 0x36B0, 0x309F, []intvia.Channel{intvia.ChannelRgbMatrix}, map[intvia.Channel]int{intvia.ChannelRgbMatrix: 45})
+// A generated file names no effect, so a board reports the same "no names" it
+// reported before one existed. A name the tool wrote would be indistinguishable
+// from the manufacturer's, and nothing in a file can be read back off a keyboard
+// to check it.
+func TestGeneratedNamesFileNamesNoEffectUntilTheUserFillsItIn(t *testing.T) {
+	dir := generateSetup(t)
+	stubGenerateTarget(t, 0x1234, 0x5678, []intvia.Channel{intvia.ChannelRgbMatrix},
+		generateStub{tops: map[intvia.Channel]int{intvia.ChannelRgbMatrix: 45}})
 
-	builtIn := 0
-	for _, def := range builtInDefinitions() {
-		if def.Matches(0x36B0, 0x309F) {
-			builtIn = namedEffectCount(def)
+	runGenerate(t)
+
+	path := onlyNamesFile(t, dir)
+	names, err := intrgbLoadNamesFile(path)
+	if err != nil {
+		t.Fatalf("LoadNamesFile(%s) error = %v; the generated file has to load", path, err)
+	}
+	if got := len(names.Effects(intvia.ChannelRgbMatrix)); got != 0 {
+		t.Errorf("effects on rgb_matrix = %d, want 0 until the names are written in", got)
+	}
+	// The slots are still there, which is the other half: the file says which IDs
+	// the board has and leaves the naming to the user.
+	body := mustRead(t, path)
+	for _, want := range []string{`"0": ""`, `"7": ""`, `"45": ""`} {
+		if !strings.Contains(body, want) {
+			t.Errorf("generated file has no empty slot %s", want)
 		}
 	}
-	if builtIn == 0 {
-		t.Fatal("the binary carries no definition for 0x36B0/0x309F, so there is nothing to shadow")
+	// And it is one line per effect, not a nested menu description to dig through.
+	if lines := strings.Count(body, "\n"); lines > 60 {
+		t.Errorf("generated file is %d lines, want roughly one per effect", lines)
+	}
+}
+
+// A names file is the one the user would have run the command to write, so a
+// second run must not replace it.
+func TestGenerateDoesNotReplaceAStoredNamesFile(t *testing.T) {
+	dir := generateSetup(t)
+	stubGenerateTarget(t, 0x1234, 0x5678, []intvia.Channel{intvia.ChannelRgbMatrix},
+		generateStub{tops: map[intvia.Channel]int{intvia.ChannelRgbMatrix: 45}})
+
+	written := `{"vendorId":"0x1234","productId":"0x5678","channels":{"rgb_matrix":{"7":"mine"}}}`
+	if err := os.WriteFile(filepath.Join(dir, "board.json"), []byte(written), 0o644); err != nil {
+		t.Fatal(err)
 	}
 
 	cmd := NewKeyboardDefinitionsGenerateCmd()
 	var out, errOut strings.Builder
 	cmd.SetOut(&out)
 	cmd.SetErr(&errOut)
-
 	err := cmd.Execute()
 	if err == nil {
-		t.Fatal("definitions generate = nil error, want a refusal to shadow a built-in definition")
+		t.Fatal("definitions generate = nil error, want a refusal to replace a stored names file")
 	}
-	if !strings.Contains(err.Error(), "carries a definition") {
-		t.Errorf("error = %q, want it to say the definition is one the binary carries", err)
+	if !strings.Contains(err.Error(), "--force") {
+		t.Errorf("error = %q, want it to offer --force", err)
 	}
-	if !strings.Contains(err.Error(), strconv.Itoa(builtIn)) {
-		t.Errorf("error = %q, want it to name how many effects would be lost (%d)", err, builtIn)
+	if got := mustRead(t, filepath.Join(dir, "board.json")); got != written {
+		t.Errorf("stored file = %q, want it untouched (%q)", got, written)
 	}
-	if entries, _ := filepath.Glob(filepath.Join(dir, "*.json")); len(entries) != 0 {
-		t.Errorf("definitions dir = %v, want nothing written", entries)
+}
+
+// The note beside the file is what carries the names, because a JSON file cannot:
+// the tool would have to read comments and VIA's parser would reject them. It has
+// to say who wrote what, or a name is a guess wearing a count.
+func TestGeneratedNoteCarriesTheSpellingsAndWhoWroteThem(t *testing.T) {
+	dir := generateSetup(t)
+	stubGenerateTarget(t, 0x1234, 0x5678, []intvia.Channel{intvia.ChannelRgbMatrix},
+		generateStub{tops: map[intvia.Channel]int{intvia.ChannelRgbMatrix: 45}})
+
+	runGenerate(t)
+
+	note := mustRead(t, strings.TrimSuffix(onlyNamesFile(t, dir), ".json")+spottedNoteSuffix)
+	for _, want := range []string{
+		"rgb_matrix (channel 3)",
+		"rainbow_moving_chevron",
+		"boards:",
+		"keychron",
+		string(slotsMeasured),
+	} {
+		if !strings.Contains(note, want) {
+			t.Errorf("note does not carry %q", want)
+		}
 	}
 }
 
-// A Vial keyboard lists its own rgb_matrix effect IDs, and the tool must use
-// those rather than writing above the top: the list is the firmware answering
-// without being touched.
+// A Vial keyboard lists its own rgb_matrix effect IDs, and the tool must use those
+// rather than writing above the top: the list is the firmware answering without
+// being touched.
 func TestGenerateUsesVialsOwnListWhenTheFirmwareIsVial(t *testing.T) {
-	dir := t.TempDir()
-	t.Cleanup(definitionFlagRestore(t))
-	t.Cleanup(forceDefinitionsDir(t, dir))
-	t.Cleanup(forceGenerateRestore(t))
-
-	original := openTarget
-	t.Cleanup(func() { openTarget = original })
-	openTarget = func(string) (rgbProtocol, targetDeviceData, []intvia.Channel, error) {
-		stub := generateStub{
+	dir := generateSetup(t)
+	stubGenerateTarget(t, 0x1234, 0x5678,
+		[]intvia.Channel{intvia.ChannelRgbMatrix, intvia.ChannelRgblight},
+		generateStub{
 			tops:    map[intvia.Channel]int{intvia.ChannelRgbMatrix: 45, intvia.ChannelRgblight: 6},
 			vial:    true,
 			vialIDs: []uint16{0, 1, 2, 40, 45},
-		}
-		return stub, stubTargetData(0x1234, 0x5678), []intvia.Channel{intvia.ChannelRgbMatrix, intvia.ChannelRgblight}, nil
-	}
+		})
 
-	cmd := NewKeyboardDefinitionsGenerateCmd()
-	var out, errOut strings.Builder
-	cmd.SetOut(&out)
-	cmd.SetErr(&errOut)
-	if err := cmd.Execute(); err != nil {
-		t.Fatalf("definitions generate error = %v (stderr %q)", err, errOut.String())
-	}
+	out, _ := runGenerate(t)
 
-	if !strings.Contains(out.String(), "Vial firmware") {
-		t.Errorf("stdout = %q, want it to say the keyboard is Vial", out.String())
+	if !strings.Contains(out, "Vial firmware") {
+		t.Errorf("stdout = %q, want it to say the keyboard is Vial", out)
 	}
-	if !strings.Contains(out.String(), experimentalVial) {
-		t.Errorf("stdout = %q, want the Vial path marked %s", out.String(), experimentalVial)
+	if !strings.Contains(out, experimentalVial) {
+		t.Errorf("stdout = %q, want the Vial path marked %s", out, experimentalVial)
 	}
-	// rgb_matrix comes from Vial and keeps the firmware's own numbering, gaps and
-	// all; rgblight has no Vial list and falls back to the clamp.
-	if !strings.Contains(out.String(), "0, 1, 2, 40, 45") {
-		t.Errorf("stdout = %q, want the rgb_matrix slots in the firmware's numbering", out.String())
+	// rgb_matrix keeps the firmware's own numbering, gaps and all; rgblight has no
+	// Vial list and falls back to the clamp.
+	if !strings.Contains(out, "0, 1, 2, 40, 45") {
+		t.Errorf("stdout = %q, want the rgb_matrix slots in the firmware's numbering", out)
 	}
-	if !strings.Contains(out.String(), string(slotsReported)) || !strings.Contains(out.String(), string(slotsMeasured)) {
-		t.Errorf("stdout = %q, want both sources named per channel", out.String())
+	if !strings.Contains(out, string(slotsReported)) || !strings.Contains(out, string(slotsMeasured)) {
+		t.Errorf("stdout = %q, want both sources named per channel", out)
 	}
 
 	// The note must not put QMK's spellings next to numbers that are not QMK's.
-	note := mustRead(t, strings.TrimSuffix(onlyDefinition(t, dir), ".json")+spottedNoteSuffix)
+	note := mustRead(t, strings.TrimSuffix(onlyNamesFile(t, dir), ".json")+spottedNoteSuffix)
 	if strings.Contains(note, "rainbow_moving_chevron") {
 		t.Errorf("note = %q, want no QMK spellings beside a Vial numbering", note)
 	}
@@ -356,34 +247,62 @@ func TestGenerateUsesVialsOwnListWhenTheFirmwareIsVial(t *testing.T) {
 // A Vial keyboard built without VIALRGB_ENABLE answers nothing, and a board that
 // cannot list its effects can still be asked by writing above the top.
 func TestGenerateFallsBackToTheClampWhenVialListsNothing(t *testing.T) {
-	dir := t.TempDir()
-	t.Cleanup(definitionFlagRestore(t))
-	t.Cleanup(forceDefinitionsDir(t, dir))
-	t.Cleanup(forceGenerateRestore(t))
+	dir := generateSetup(t)
+	stubGenerateTarget(t, 0x1234, 0x5678, []intvia.Channel{intvia.ChannelRgbMatrix},
+		generateStub{tops: map[intvia.Channel]int{intvia.ChannelRgbMatrix: 45}, vial: true})
 
-	original := openTarget
-	t.Cleanup(func() { openTarget = original })
-	openTarget = func(string) (rgbProtocol, targetDeviceData, []intvia.Channel, error) {
-		stub := generateStub{
-			tops:    map[intvia.Channel]int{intvia.ChannelRgbMatrix: 45},
-			vial:    true,
-			vialIDs: nil,
-		}
-		return stub, stubTargetData(0x1234, 0x5678), []intvia.Channel{intvia.ChannelRgbMatrix}, nil
-	}
+	out, _ := runGenerate(t)
 
-	cmd := NewKeyboardDefinitionsGenerateCmd()
-	var out, errOut strings.Builder
-	cmd.SetOut(&out)
-	cmd.SetErr(&errOut)
-	if err := cmd.Execute(); err != nil {
-		t.Fatalf("definitions generate error = %v", err)
-	}
-	if !strings.Contains(out.String(), string(slotsMeasured)) {
-		t.Errorf("stdout = %q, want the clamp used when Vial lists nothing", out.String())
+	if !strings.Contains(out, string(slotsMeasured)) {
+		t.Errorf("stdout = %q, want the clamp used when Vial lists nothing", out)
 	}
-	note := mustRead(t, strings.TrimSuffix(onlyDefinition(t, dir), ".json")+spottedNoteSuffix)
+	note := mustRead(t, strings.TrimSuffix(onlyNamesFile(t, dir), ".json")+spottedNoteSuffix)
 	if !strings.Contains(note, "rainbow_moving_chevron") {
 		t.Errorf("note = %q, want the QMK spellings again, which is what the clamp gives", note)
 	}
 }
+
+// The point of storing names apart from the definition: a name the user writes
+// replaces the vendor's for that one effect, and the rest of the file still stands.
+// The board is the Impact 80, whose definition the binary carries, so "the rest"
+// means real names the vendor wrote.
+func TestANameTheUserWroteOverridesOneEffectAndLeavesTheRest(t *testing.T) {
+	generateSetup(t)
+	names := t.TempDir()
+	t.Cleanup(forceNamesDir(t, names))
+
+	body := `{"name":"Impact 80","vendorId":"0x36B0","productId":"0x309F",
+		"channels":{"rgb_matrix":{"7":"my_chevron"}}}`
+	if err := os.WriteFile(filepath.Join(names, "impact80.json"), []byte(body), 0o644); err != nil {
+		t.Fatal(err)
+	}
+
+	catalog, _, err := resolveCatalogFor(0x36B0, 0x309F)
+	if err != nil {
+		t.Fatalf("resolveCatalogFor() error = %v", err)
+	}
+	if got := catalog.EffectName(intvia.ChannelRgbMatrix, 7); got != "my_chevron" {
+		t.Errorf("EffectName(7) = %q, want the user's name", got)
+	}
+	if got := catalog.EffectSource(intvia.ChannelRgbMatrix, 7); got != intrgbSourceUser {
+		t.Errorf("EffectSource(7) = %q, want %q", got, intrgbSourceUser)
+	}
+	// ID 8 is band_pinwheel_sat in the Impact 80's own file, and the user's file
+	// said nothing about it, so it has to still be the vendor's name and carry no
+	// source of its own.
+	if got := catalog.EffectName(intvia.ChannelRgbMatrix, 8); got != "band_pinwheel_sat" {
+		t.Errorf("EffectName(8) = %q, want the vendor's name left alone", got)
+	}
+	if got := catalog.EffectSource(intvia.ChannelRgbMatrix, 8); got != intrgbSourceVendor {
+		t.Errorf("EffectSource(8) = %q, want an empty source for the vendor's", got)
+	}
+}
+
+// intrgbLoadNamesFile and intrgbSourceUser keep the test reading through the
+// internal package without importing it twice under two names.
+func intrgbLoadNamesFile(path string) (*intrgb.Names, error) { return intrgb.LoadNamesFile(path) }
+
+const (
+	intrgbSourceUser   = intrgb.SourceUser
+	intrgbSourceVendor = intrgb.SourceVendor
+)

+ 43 - 0
cmd/qmk-rgb-tool/definition_test.go

@@ -10,6 +10,7 @@ import (
 	"testing"
 
 	intdevice "netdome.biz/paul/qmk-rgb/internal/device"
+	intvia "netdome.biz/paul/qmk-rgb/internal/via"
 )
 
 func TestFetchDefinitionStoresTheFileForTheBoard(t *testing.T) {
@@ -570,3 +571,45 @@ func writeDefinition(t *testing.T, dir, name, content string) {
 		t.Fatal(err)
 	}
 }
+
+// A names file is JSON, carries the board's vendor and product ID, and has no
+// menus — so read as a definition it parses, and the definitions directory is
+// searched before the files built into the binary. A names file sitting there
+// would therefore take the board's place and leave the tool with no names at all.
+// It declares what it is, and that declaration is what this test checks.
+func TestNamesFileInTheDefinitionsDirectoryIsNotADefinition(t *testing.T) {
+	names := generateSetup(t)
+	body := `{"kind":"names","name":"Impact 80","vendorId":"0x36B0","productId":"0x309F",
+		"channels":{"rgb_matrix":{"7":"my_chevron"}}}`
+	if err := os.WriteFile(filepath.Join(definitionsPath(), "oops.json"), []byte(body), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	_ = names
+
+	// The board's real definition, built into the binary, still stands.
+	catalog, _, err := resolveCatalogFor(0x36B0, 0x309F)
+	if err != nil {
+		t.Fatalf("resolveCatalogFor() error = %v", err)
+	}
+	if catalog == nil {
+		t.Fatal("resolveCatalogFor() = nil catalog, want the built-in definition")
+	}
+	if got := len(catalog.Effects(intvia.ChannelRgbMatrix)); got != 46 {
+		t.Errorf("effects on rgb_matrix = %d, want the definition's 46", got)
+	}
+	// And the names file is still read for the name it carries, from its own
+	// directory.
+	if err := os.MkdirAll(namesPath(), 0o755); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.WriteFile(filepath.Join(namesPath(), "mine.json"), []byte(body), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	merged, _, err := resolveCatalogFor(0x36B0, 0x309F)
+	if err != nil {
+		t.Fatalf("resolveCatalogFor() error = %v", err)
+	}
+	if got := merged.EffectName(intvia.ChannelRgbMatrix, 7); got != "my_chevron" {
+		t.Errorf("EffectName(7) = %q, want the user's name from the names directory", got)
+	}
+}

+ 26 - 0
internal/rgb/definition.go

@@ -2,6 +2,7 @@ package rgb
 
 import (
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io/fs"
 	"os"
@@ -53,10 +54,25 @@ type Definition struct {
 	Labels map[uint16]string
 }
 
+// ErrNotADefinition is returned for a file that is deliberately a different kind
+// of document — a names file, which this tool also keeps as JSON. It is separate
+// from a parse failure because the two call for opposite answers: a broken
+// definition is a file the user has to fix, and a names file in the wrong
+// directory is a file to skip or move.
+var ErrNotADefinition = errors.New("not a VIA definition")
+
+// NamesFileKind is the value the "kind" field of a names file carries. A file
+// declaring it is not a definition, whatever its name and wherever it sits, which
+// is the only reliable way to tell the two apart: a names file has the board's
+// identifiers and no menus, so it parses as a definition and would otherwise take
+// the board's place.
+const NamesFileKind = "names"
+
 // definitionFile is the part of a VIA definition this tool reads. VIA serves
 // built definitions that carry vendorProductId as a number and no vendorId and
 // productId pair, so both spellings are optional and at least one is required.
 type definitionFile struct {
+	Kind            string          `json:"kind"`
 	Name            string          `json:"name"`
 	VendorID        string          `json:"vendorId"`
 	ProductID       string          `json:"productId"`
@@ -93,6 +109,9 @@ func ParseDefinition(source string, data []byte) (*Definition, error) {
 	if err := json.Unmarshal(data, &file); err != nil {
 		return nil, fmt.Errorf("parse %s: not a keyboard definition: %w", source, err)
 	}
+	if file.Kind == NamesFileKind {
+		return nil, fmt.Errorf("parse %s: %w, it is a names file (kind %q)", source, ErrNotADefinition, NamesFileKind)
+	}
 
 	vendorID, productID, err := file.identifiers()
 	if err != nil {
@@ -351,6 +370,13 @@ func LoadDefinitionsDir(dir string) ([]*Definition, error) {
 			continue
 		}
 		def, err := LoadDefinition(filepath.Join(dir, entry.Name()))
+		if errors.Is(err, ErrNotADefinition) {
+			// A names file in the definitions directory is not a definition that
+			// failed to parse, it is a different kind of document, and it must not
+			// be read as one. It carries vendorId and productId and no menus, so
+			// without this it parses and shadows the board's real file.
+			continue
+		}
 		if err != nil {
 			return nil, err
 		}

+ 8 - 0
internal/rgb/names.go

@@ -33,6 +33,7 @@ const (
 // what a JSON object key is, and both are read back through the one vocabulary in
 // internal/via rather than a second table here.
 type namesDocument struct {
+	Kind      string                       `json:"kind"`
 	Name      string                       `json:"name,omitempty"`
 	VendorID  string                       `json:"vendorId"`
 	ProductID string                       `json:"productId"`
@@ -92,6 +93,13 @@ func LoadNamesFile(path string) (*Names, error) {
 			if err != nil {
 				return nil, fmt.Errorf("parse %s: %s channel %q: %w", path, key, rawID, err)
 			}
+			// An empty name states a slot and not a name, which is what a file a
+			// user is meant to fill in looks like. It is skipped rather than stored,
+			// so the channel reports no effects for it until a name is written —
+			// the same rule the VIA parser follows for the same reason.
+			if strings.TrimSpace(name) == "" {
+				continue
+			}
 			if names.effects[ch] == nil {
 				names.effects[ch] = map[uint8]string{}
 			}