فهرست منبع

fix three documented-behaviour defects found by review

C1 - `load` ignored --zone, contradicting an explicit documented guarantee.
README.md and AGENTS.md both promise "With --zone, commands target exactly
the selected zone", and `save` always honoured it. `load` instead iterated
every zone in the profile, so `--zone logo load <name>` rewrote all three
zones the profile contained: a single-zone restore silently had a
three-zone blast radius, which is the opposite of what the flag is for.
Zones are now filtered through the selection and iterated in a stable
order instead of Go's randomised map order. A selection the profile does
not cover warns rather than reporting success.

C2 - `list` was not JSON when profiles existed. README.md advertises JSON
for `keyboard info`, `info`, `list` and `effect --list`. The empty case
hand-rolled `{"profiles":[]}` via fmt.Println to real stdout; the non-empty
case printed bare newline-separated names. The shape therefore changed with
the contents of profiles/, and an agent parsing the documented shape crashed
on the common case. Both branches now go through encodeJSON, so `list` also
writes to the cobra output writer and the empty case yields `[]`, not null.

C3 - `rgb <subcommand>` was documented in 11 places and does not exist. The
group was flattened to the root in e7dd8f6 and the prose kept the prefix. The
most visible instance told the reader to run `rgb color <hex>` to set a
permanent colour, which exits 1 with "unknown command". The prefix is gone;
all eight commands verified present without it.

Also formatted profile.go, which had been failing gofmt since before this
work and is now edited by this change.
Paul Klumpp 1 هفته پیش
والد
کامیت
445a903d70

+ 2 - 2
AGENTS.md

@@ -80,9 +80,9 @@ ID 39 is `multisplash`; ID 41 is `solid_multisplash`. Supported aliases are
 `off` → `none`, `breathe` → `breathing`, `rainbow` (zone-dependent),
 `off` → `none`, `breathe` → `breathing`, `rainbow` (zone-dependent),
 `rainbow_wave` (Logo/Side), `solid` (zone-dependent), and legacy `static`.
 `rainbow_wave` (Logo/Side), `solid` (zone-dependent), and legacy `static`.
 Do not assume a numeric effect ID is valid on every zone; use the zone-aware
 Do not assume a numeric effect ID is valid on every zone; use the zone-aware
-name resolver or deliberately use `rgb mode` as a raw escape hatch.
+name resolver or deliberately use `mode` as a raw escape hatch.
 
 
-`rgb info` emits a `zones` array containing each selected zone's channel,
+`info` emits a `zones` array containing each selected zone's channel,
 enabled state, effect name and ID, brightness, speed, and color. The
 enabled state, effect name and ID, brightness, speed, and color. The
 top-level summary comes from the first selected zone. A failed zone includes
 top-level summary comes from the first selected zone. A failed zone includes
 an `error` field while other zone results remain available; the command
 an `error` field while other zone results remain available; the command

+ 9 - 9
README.md

@@ -93,7 +93,7 @@ rather than storing the number.
 - **Profile system** — save, load, list, and delete RGB presets as JSON files in `profiles/`
 - **Profile system** — save, load, list, and delete RGB presets as JSON files in `profiles/`
 - **46 backlight effects** — complete Impact 80 effect family mapped to zone-aware effect names
 - **46 backlight effects** — complete Impact 80 effect family mapped to zone-aware effect names
 - **Compatibility aliases** — `off`, `breathe`, `rainbow`, `solid`, `static` resolve to correct effect IDs per zone
 - **Compatibility aliases** — `off`, `breathe`, `rainbow`, `solid`, `static` resolve to correct effect IDs per zone
-- **Reactive & splash effects** — honor `rgb color` and `rgb speed` for key-press illumination
+- **Reactive & splash effects** — honor `color` and `speed` for key-press illumination
 - **Machine-parseable output** — JSON for `keyboard info`, `info`, `list`, and `effect --list`
 - **Machine-parseable output** — JSON for `keyboard info`, `info`, `list`, and `effect --list`
 - **Agent-friendly** — designed for automation, scripting, and CLI-first workflows
 - **Agent-friendly** — designed for automation, scripting, and CLI-first workflows
 - **Multiple devices** — `keyboard info` numbers each keyboard; `--device <n>` targets one
 - **Multiple devices** — `keyboard info` numbers each keyboard; `--device <n>` targets one
@@ -150,15 +150,15 @@ Backlight uses the complete Impact 80 catalog:
 
 
 Effects fall into categories that behave differently:
 Effects fall into categories that behave differently:
 
 
-- **Static** effects (`solid_color`, `none`) display a steady output. The color is set by `rgb color` and is constant.
-- **Reactive** effects (`solid_reactive*`) light up on key press. They honor the color set by `rgb color` — the pressed key's LEDs flash in the configured color. Use `rgb speed` to adjust how long the illumination lasts.
-- **Breathing** effects (`breathing`, `hue_breathing`) slowly fade in and out. They do not honor `rgb color` directly; `hue_breathing` cycles through the full hue range.
-- **Dynamic/stream** effects (`band_*`, `cycle_*`, `rainbow_*`, `starlight*`, `raindrops`, `jellybean_raindrops`, `flower_blooming`, `pixel_flow`, `riverflow`) animate independently. Most ignore `rgb color` and use their own color palettes. `hue_breathing`, `hue_pendulum`, and `hue_wave` cycle through hues.
-- **Splash** effects (`splash`, `multisplash`, `solid_splash`, `solid_multisplash`) react to key presses like reactive effects but with a splash pattern. `solid_splash` and `solid_multisplash` honor `rgb color`.
-- **Gradient** effects (`gradient_up_down`, `gradient_left_right`) create a color gradient across the keyboard. They do not honor `rgb color`.
+- **Static** effects (`solid_color`, `none`) display a steady output. The color is set by `color` and is constant.
+- **Reactive** effects (`solid_reactive*`) light up on key press. They honor the color set by `color` — the pressed key's LEDs flash in the configured color. Use `speed` to adjust how long the illumination lasts.
+- **Breathing** effects (`breathing`, `hue_breathing`) slowly fade in and out. They do not honor `color` directly; `hue_breathing` cycles through the full hue range.
+- **Dynamic/stream** effects (`band_*`, `cycle_*`, `rainbow_*`, `starlight*`, `raindrops`, `jellybean_raindrops`, `flower_blooming`, `pixel_flow`, `riverflow`) animate independently. Most ignore `color` and use their own color palettes. `hue_breathing`, `hue_pendulum`, and `hue_wave` cycle through hues.
+- **Splash** effects (`splash`, `multisplash`, `solid_splash`, `solid_multisplash`) react to key presses like reactive effects but with a splash pattern. `solid_splash` and `solid_multisplash` honor `color`.
+- **Gradient** effects (`gradient_up_down`, `gradient_left_right`) create a color gradient across the keyboard. They do not honor `color`.
 - **Alphas mods** (`alphas_mods`) colors modifier keys differently from alphanumeric keys. The colors are built-in and cannot be changed.
 - **Alphas mods** (`alphas_mods`) colors modifier keys differently from alphanumeric keys. The colors are built-in and cannot be changed.
 
 
-To set a permanent color, use `solid_color` and then `rgb color <hex>`.
+To set a permanent color, use `solid_color` and then `color <hex>`.
 
 
 ## Compatibility Aliases
 ## Compatibility Aliases
 Effect names are resolved independently for each target zone. Compatibility
 Effect names are resolved independently for each target zone. Compatibility
@@ -178,7 +178,7 @@ where each effect has its own header file (e.g.
 
 
 ID 39 is `multisplash`; ID 41 is the distinct `solid_multisplash` name.
 ID 39 is `multisplash`; ID 41 is the distinct `solid_multisplash` name.
 
 
-`rgb info` reports a `zones` array with each zone's channel, enabled state,
+`info` reports a `zones` array with each zone's channel, enabled state,
 effect name and ID, brightness, speed, and color. The top-level `enabled`,
 effect name and ID, brightness, speed, and color. The top-level `enabled`,
 `mode`, `brightness`, and `speed` fields summarize the first selected zone.
 `mode`, `brightness`, and `speed` fields summarize the first selected zone.
 If one zone cannot be queried, its record contains an `error`, other records
 If one zone cannot be queried, its record contains an `error`, other records

+ 51 - 0
cmd/qmk-rgb-tool/flags_test.go

@@ -0,0 +1,51 @@
+package main
+
+import (
+	"strings"
+	"testing"
+)
+
+// The --device flag accepts the 1-based number that `keyboard info` prints.
+// Its help text is the only place a user learns that, so it must not
+// describe the HID path the flag rejects.
+func TestDeviceFlagUsageDescribesANumberNotAPath(t *testing.T) {
+	flag := newRootCommand().PersistentFlags().Lookup("device")
+	if flag == nil {
+		t.Fatal("--device flag not found")
+	}
+
+	if strings.Contains(flag.Usage, "path") {
+		t.Errorf("--device usage = %q, must not mention a path; the flag rejects HID paths", flag.Usage)
+	}
+	if !strings.Contains(flag.Usage, "number") {
+		t.Errorf("--device usage = %q, want it to say the argument is a number", flag.Usage)
+	}
+}
+
+// Cobra reads a back-quoted word in a flag's usage as the value placeholder
+// and renders it where the type name would go, turning `--device into
+// `--device keyboard info`. The help must therefore contain no backticks.
+func TestFlagUsageHasNoValuePlaceholder(t *testing.T) {
+	for _, name := range []string{"device", "zone"} {
+		flag := newRootCommand().PersistentFlags().Lookup(name)
+		if flag == nil {
+			t.Fatalf("--%s flag not found", name)
+		}
+		if strings.Contains(flag.Usage, "`") {
+			t.Errorf("--%s usage = %q, backticks make cobra render a bogus value placeholder", name, flag.Usage)
+		}
+	}
+}
+
+func TestZoneFlagUsageListsEveryZone(t *testing.T) {
+	flag := newRootCommand().PersistentFlags().Lookup("zone")
+	if flag == nil {
+		t.Fatal("--zone flag not found")
+	}
+
+	for _, zone := range []string{"logo", "backlight", "side"} {
+		if !strings.Contains(flag.Usage, zone) {
+			t.Errorf("--zone usage = %q, want it to list %q", flag.Usage, zone)
+		}
+	}
+}

+ 1 - 1
cmd/qmk-rgb-tool/main.go

@@ -24,7 +24,7 @@ func newRootCommand() *cobra.Command {
 		SilenceErrors: true,
 		SilenceErrors: true,
 		SilenceUsage:  true,
 		SilenceUsage:  true,
 	}
 	}
-	cmd.PersistentFlags().StringVar(&targetDevice, "device", "", "HID device path to use")
+	cmd.PersistentFlags().StringVar(&targetDevice, "device", "", "Keyboard number as printed by 'keyboard info', starting at 1")
 	cmd.PersistentFlags().StringVar(&targetZone, "zone", "", "RGB lighting zone (logo, backlight, side)")
 	cmd.PersistentFlags().StringVar(&targetZone, "zone", "", "RGB lighting zone (logo, backlight, side)")
 	cmd.PersistentPreRunE = func(*cobra.Command, []string) error {
 	cmd.PersistentPreRunE = func(*cobra.Command, []string) error {
 		_, err := selectedZones()
 		_, err := selectedZones()

+ 38 - 10
cmd/qmk-rgb-tool/profile.go

@@ -5,18 +5,20 @@ import (
 	"fmt"
 	"fmt"
 	"os"
 	"os"
 	"path/filepath"
 	"path/filepath"
+	"sort"
 	"strings"
 	"strings"
 
 
+	"github.com/spf13/cobra"
 	intrgb "netdome.biz/paul/impact-80/internal/rgb"
 	intrgb "netdome.biz/paul/impact-80/internal/rgb"
 	"netdome.biz/paul/impact-80/internal/via"
 	"netdome.biz/paul/impact-80/internal/via"
-	"github.com/spf13/cobra"
 )
 )
 
 
-const profilesDir = "profiles"
+// profilesDir is a var so tests can redirect it at a temp directory.
+var profilesDir = "profiles"
 
 
 type Profile struct {
 type Profile struct {
-	Name    string                    `json:"name"`
-	Version int                       `json:"version"`
+	Name    string                        `json:"name"`
+	Version int                           `json:"version"`
 	Zones   map[intrgb.Zone]*ZoneSettings `json:"zones"`
 	Zones   map[intrgb.Zone]*ZoneSettings `json:"zones"`
 }
 }
 
 
@@ -206,7 +208,25 @@ func NewProfileLoadCmd() *cobra.Command {
 				return err
 				return err
 			}
 			}
 
 
-			for zone, settings := range p.Zones {
+			selected, err := selectedZones()
+			if err != nil {
+				return err
+			}
+
+			// Iterate the profile in a deterministic order and honour --zone,
+			// so `--zone logo load <name>` restores the logo and leaves the
+			// other zones untouched.
+			profileZones := make([]intrgb.Zone, 0, len(p.Zones))
+			for zone := range p.Zones {
+				profileZones = append(profileZones, zone)
+			}
+			sort.Slice(profileZones, func(i, j int) bool {
+				return profileZones[i] < profileZones[j]
+			})
+
+			applied := 0
+			for _, zone := range filterZones(profileZones, selected) {
+				settings := p.Zones[zone]
 				channel := via.LEDType(zone.Channel())
 				channel := via.LEDType(zone.Channel())
 
 
 				// Resolve effect name to ID
 				// Resolve effect name to ID
@@ -239,6 +259,12 @@ func NewProfileLoadCmd() *cobra.Command {
 						}
 						}
 					}
 					}
 				}
 				}
+				applied++
+			}
+
+			if applied == 0 {
+				fmt.Fprintf(cmd.ErrOrStderr(),
+					"Warning: profile %q has no settings for the selected zone(s); nothing applied\n", name)
 			}
 			}
 			return nil
 			return nil
 		},
 		},
@@ -250,17 +276,19 @@ func NewProfileListCmd() *cobra.Command {
 	return &cobra.Command{
 	return &cobra.Command{
 		Use:   "list",
 		Use:   "list",
 		Short: "List saved profiles",
 		Short: "List saved profiles",
+		Args:  cobra.NoArgs,
 		RunE: func(cmd *cobra.Command, args []string) error {
 		RunE: func(cmd *cobra.Command, args []string) error {
 			names, err := ListProfiles()
 			names, err := ListProfiles()
 			if err != nil {
 			if err != nil {
 				return err
 				return err
 			}
 			}
-			if len(names) == 0 {
-				fmt.Println(`{"profiles":[]}`)
-				return nil
+			if names == nil {
+				names = []string{}
 			}
 			}
-			fmt.Println(strings.Join(names, "\n"))
-			return nil
+			type list struct {
+				Profiles []string `json:"profiles"`
+			}
+			return encodeJSON(cmd.OutOrStdout(), list{Profiles: names})
 		},
 		},
 	}
 	}
 }
 }

+ 80 - 0
cmd/qmk-rgb-tool/profile_list_test.go

@@ -0,0 +1,80 @@
+package main
+
+import (
+	"bytes"
+	"encoding/json"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+type profileListOutput struct {
+	Profiles []string `json:"profiles"`
+}
+
+func withProfilesDir(t *testing.T, names ...string) {
+	t.Helper()
+	dir := t.TempDir()
+	for _, n := range names {
+		body := `{"name":"` + n + `","zones":[]}`
+		if err := os.WriteFile(filepath.Join(dir, n+".json"), []byte(body), 0644); err != nil {
+			t.Fatalf("write profile %s: %v", n, err)
+		}
+	}
+	orig := profilesDir
+	profilesDir = dir
+	t.Cleanup(func() { profilesDir = orig })
+}
+
+func runProfileList(t *testing.T) string {
+	t.Helper()
+	var out bytes.Buffer
+	cmd := NewProfileListCmd()
+	cmd.SetOut(&out)
+	cmd.SetErr(&out)
+	if err := cmd.Execute(); err != nil {
+		t.Fatalf("list returned error: %v", err)
+	}
+	return out.String()
+}
+
+// README.md advertises JSON for `list`. The shape must not change with the
+// number of profiles on disk.
+func TestProfileListEmitsJSONWithProfiles(t *testing.T) {
+	withProfilesDir(t, "desk", "movie")
+
+	got := runProfileList(t)
+
+	var parsed profileListOutput
+	if err := json.Unmarshal([]byte(got), &parsed); err != nil {
+		t.Fatalf("list output is not valid JSON: %v (output %q)", err, got)
+	}
+	if len(parsed.Profiles) != 2 {
+		t.Errorf("profiles = %v, want 2 entries", parsed.Profiles)
+	}
+}
+
+func TestProfileListEmitsJSONWithoutProfiles(t *testing.T) {
+	withProfilesDir(t)
+
+	got := runProfileList(t)
+
+	var parsed profileListOutput
+	if err := json.Unmarshal([]byte(got), &parsed); err != nil {
+		t.Fatalf("list output is not valid JSON: %v (output %q)", err, got)
+	}
+	if parsed.Profiles == nil {
+		t.Error("profiles = null, want an empty array so consumers can iterate unconditionally")
+	}
+}
+
+func TestProfileListUsesTheSharedJSONShape(t *testing.T) {
+	withProfilesDir(t, "desk")
+
+	got := runProfileList(t)
+
+	if !strings.Contains(got, "\n  \"profiles\": [") {
+		t.Errorf("list output = %q, want the same two-space indent as the other JSON commands", got)
+	}
+}

+ 93 - 0
cmd/qmk-rgb-tool/profile_load_test.go

@@ -0,0 +1,93 @@
+package main
+
+import (
+	"testing"
+
+	intrgb "netdome.biz/paul/impact-80/internal/rgb"
+)
+
+func sameZones(a, b []intrgb.Zone) bool {
+	if len(a) != len(b) {
+		return false
+	}
+	for i := range a {
+		if a[i] != b[i] {
+			return false
+		}
+	}
+	return true
+}
+
+// README.md: "With `--zone`, commands target exactly the selected zone."
+// `save` already honours it, so `load` must not rewrite zones the user did
+// not select — otherwise a single-zone restore silently touches all three.
+func TestFilterZonesKeepsOnlyTheSelection(t *testing.T) {
+	allZones := []intrgb.Zone{intrgb.ZoneLogo, intrgb.ZoneBacklight, intrgb.ZoneSide}
+
+	tests := []struct {
+		name     string
+		zoneFlag string
+		want     []intrgb.Zone
+	}{
+		{"no flag keeps every zone", "", allZones},
+		{"logo keeps logo", "logo", []intrgb.Zone{intrgb.ZoneLogo}},
+		{"backlight keeps backlight", "backlight", []intrgb.Zone{intrgb.ZoneBacklight}},
+		{"side keeps side", "side", []intrgb.Zone{intrgb.ZoneSide}},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			original := targetZone
+			t.Cleanup(func() { targetZone = original })
+			targetZone = tt.zoneFlag
+
+			selected, err := selectedZones()
+			if err != nil {
+				t.Fatalf("selectedZones() returned error: %v", err)
+			}
+
+			got := filterZones(allZones, selected)
+			if !sameZones(got, tt.want) {
+				t.Errorf("filterZones(all, %v) = %v, want %v", selected, got, tt.want)
+			}
+		})
+	}
+}
+
+// A profile may omit zones the selection names. Those must not be invented.
+func TestFilterZonesDoesNotAddMissingZones(t *testing.T) {
+	original := targetZone
+	t.Cleanup(func() { targetZone = original })
+	targetZone = "backlight"
+
+	profileZones := []intrgb.Zone{intrgb.ZoneSide}
+
+	selected, err := selectedZones()
+	if err != nil {
+		t.Fatalf("selectedZones() returned error: %v", err)
+	}
+
+	got := filterZones(profileZones, selected)
+	if len(got) != 0 {
+		t.Errorf("filterZones(side-only profile, backlight) = %v, want empty", got)
+	}
+}
+
+// filterZones must not reorder or mutate its input.
+func TestFilterZonesDoesNotMutateInput(t *testing.T) {
+	original := targetZone
+	t.Cleanup(func() { targetZone = original })
+	targetZone = "logo"
+
+	input := []intrgb.Zone{intrgb.ZoneSide, intrgb.ZoneLogo}
+	selected, err := selectedZones()
+	if err != nil {
+		t.Fatalf("selectedZones() returned error: %v", err)
+	}
+
+	filterZones(input, selected)
+
+	if !sameZones(input, []intrgb.Zone{intrgb.ZoneSide, intrgb.ZoneLogo}) {
+		t.Errorf("filterZones() mutated its input: %v", input)
+	}
+}

+ 19 - 0
cmd/qmk-rgb-tool/zones.go

@@ -23,3 +23,22 @@ func zoneChannels(zones []intrgb.Zone) []via.LEDType {
 	}
 	}
 	return channels
 	return channels
 }
 }
+
+// filterZones keeps the zones that appear in selected, preserving the order
+// of candidates. It neither adds nor removes zones on its own: a zone the
+// caller does not offer stays absent, so a profile that omits a zone is not
+// padded with defaults.
+func filterZones(candidates, selected []intrgb.Zone) []intrgb.Zone {
+	keep := make(map[intrgb.Zone]bool, len(selected))
+	for _, zone := range selected {
+		keep[zone] = true
+	}
+
+	filtered := make([]intrgb.Zone, 0, len(candidates))
+	for _, zone := range candidates {
+		if keep[zone] {
+			filtered = append(filtered, zone)
+		}
+	}
+	return filtered
+}