Merge branch 'atomic-edits' of bajdev/mpckit into main

internal/app/app.go

@@ -145,7 +145,7 @@ func (s *Service) SetCloser(f func()) { s.closer = f }
 func (s *Service) Dirty() bool {
 	s.mu.Lock()
 	defer s.mu.Unlock()
-	return s.p.Dirty
+	return s.p.Dirty()
 }
 
 // CloseWindow closes the app without asking about unsaved changes. The
@@ -216,7 +216,7 @@ func (s *Service) state() State {
 	p := s.p
 	st := State{
 		Name:        p.Name,
-		Dirty:       p.Dirty,
+		Dirty:       p.Dirty(),
 		Pads:        make([]Pad, 0, project.Pads),
 		Sequences:   []Sequence{},
 		SampleCount: p.SampleCount(),
@@ -236,7 +236,7 @@ func (s *Service) state() State {
 	}
 	missing := map[string]bool{} // pool file -> source missing, as export sees it
 	for _, f := range p.PoolFiles() {
-		src := p.Sources[f]
+		src, _ := p.Source(f)
 		missing[f] = src == "" || !export.Usable(src)
 	}
 	for slot := 0; slot < project.Pads; slot++ {
@@ -250,7 +250,7 @@ func (s *Service) state() State {
 			Coarse: v.Coarse, Fine: v.Fine, MuteGroup: v.MuteGroup, Start: v.Start, End: v.End}
 		if !v.Empty() {
 			pad.HasNotes = p.PadHasNotes(slot)
-			pad.Source = p.Sources[v.File]
+			pad.Source, _ = p.Source(v.File)
 			pad.Missing = missing[v.File]
 			if !pad.Missing {
 				pad.Audio = audioURL(slot, pad.Source)
@@ -394,7 +394,7 @@ func (s *Service) exported(path string, err error) (ExportResult, error) {
 	if err != nil {
 		return ExportResult{}, err
 	}
-	s.p.Dirty = false
+	s.p.MarkExported()
 	s.exportDir, s.zipPath = "", "" // the next export asks again
 	return ExportResult{Path: path}, nil
 }

internal/app/audio.go

@@ -42,7 +42,8 @@ func (s *Service) serveAudio(w http.ResponseWriter, r *http.Request) {
 		s.mu.Lock()
 		defer s.mu.Unlock()
 		v, err := s.p.Pad(slot)
-		return v, s.p.Sources[v.File], err
+		src, _ := s.p.Source(v.File)
+		return v, src, err
 	}()
 	if err != nil || v.Empty() || src == "" {
 		http.NotFound(w, r)

internal/export/export.go

@@ -65,8 +65,8 @@ func collect(p *project.Project) ([]item, error) {
 	var items []item
 	missing := &MissingError{}
 	for _, f := range p.PoolFiles() {
-		src := p.Sources[f]
-		if src == "" {
+		src, ok := p.Source(f)
+		if !ok || src == "" {
 			missing.Files = append(missing.Files, f)
 			missing.Paths = append(missing.Paths, "no source recorded")
 			continue

internal/export/export_test.go

@@ -328,29 +328,24 @@ func TestLinkedDataFolder(t *testing.T) {
 }
 
 func TestBadPoolName(t *testing.T) {
-	p := buildT1(t)
-	// Load and AssignSample never produce such a name; change the tree
-	// directly to reach the export check.
-	d, err := p.File().Data()
-	must(t, err)
-	src := p.Sources["Linn 9000 kick.wav"]
-	must(t, d.Set("samples[0].path", xpj.NewString("../evil.wav")))
-	p.Sources["../evil.wav"] = src
-
-	dest := filepath.Join(t.TempDir(), "out")
-	must(t, os.Mkdir(dest, 0o755))
+	// Load and AssignSample never produce such a name, and only project can
+	// record a source, so the export check is tested directly: both Folder
+	// and the zip writer call it before writing anything.
+	src := filepath.Join(testdata, "accepted", "MpckitT1"+project.DataSuffix, "Linn 9000 kick.wav")
+	items := []item{{name: "../evil.wav", src: src}}
 	want := `sample "../evil.wav": file name "../evil.wav" contains a folder separator`
-	if _, err := Folder(p, dest, false); err == nil || err.Error() != want {
-		t.Errorf("Folder: got %v, want %q", err, want)
+	if err := checkNames(items); err == nil || err.Error() != want {
+		t.Errorf("checkNames: got %v, want %q", err, want)
 	}
-	if err := Zip(p, filepath.Join(dest, "kit.zip"), false); err == nil || err.Error() != want {
-		t.Errorf("Zip: got %v, want %q", err, want)
+	var b bytes.Buffer
+	if err := writeZip(&b, buildT1(t), nil, items); err == nil || err.Error() != want {
+		t.Errorf("writeZip: got %v, want %q", err, want)
 	}
-	if got := listDir(t, dest); len(got) != 0 {
-		t.Errorf("failed export wrote %v", got)
+	if b.Len() != 0 {
+		t.Errorf("writeZip wrote %d bytes", b.Len())
 	}
-	if _, err := os.Stat(filepath.Join(filepath.Dir(dest), "evil.wav")); err == nil {
-		t.Error("export wrote outside the data folder")
+	if err := checkNames([]item{{name: "kick.wav", src: src}}); err != nil {
+		t.Errorf("checkNames on a plain name: %v", err)
 	}
 }
 
@@ -471,10 +466,13 @@ func TestExportErrors(t *testing.T) {
 		t.Errorf("Zip with a bad name: %v", err)
 	}
 
+	// A pool file with no recorded source: rename one in the tree.
 	unknown := buildT1(t)
-	delete(unknown.Sources, "Linn 9000 kick.wav")
-	_, err := Folder(unknown, dir, false)
-	if err == nil || !strings.Contains(err.Error(), "Linn 9000 kick.wav (no source recorded)") {
+	d, err := unknown.File().Data()
+	must(t, err)
+	must(t, d.Set("samples[0].path", xpj.NewString("other.wav")))
+	_, err = Folder(unknown, dir, false)
+	if err == nil || !strings.Contains(err.Error(), "other.wav (no source recorded)") {
 		t.Errorf("no source: %v", err)
 	}
 

internal/project/pads.go

@@ -134,47 +134,64 @@ func (p *Project) AssignSample(slot int, src string) error {
 		return fmt.Errorf("%s: %w", name, err)
 	}
 
+	// Work out every change before making any, so that a failure leaves
+	// the project as it was. Reuse the pool entry already copied from this
+	// file, even if this pad was its only user.
 	file := p.fileForSource(target)
+	freed := p.freedBy(slot, file)
+	newFile := file
 	if file == "" {
-		// A new pool entry is needed. Clearing the pad first may free one.
-		freed := 0
-		for _, f := range padFiles(p.pad(slot)) {
-			if p.usage(f) == 1 {
-				freed++
-			}
-		}
-		if p.SampleCount()-freed >= MaxSamples {
+		if p.SampleCount()-len(freed) >= MaxSamples {
 			return fmt.Errorf("the project already has %d samples, the most the device allows", MaxSamples)
 		}
+		newFile = p.uniqueFile(poolFileName(name), freed)
 	}
-	// Keep the reused entry even if this pad was its only user.
-	p.clear(slot, file)
-	if file == "" {
-		file = p.uniqueFile(poolFileName(name))
-		if err := p.addPoolEntry(file); err != nil {
-			return err
-		}
-		p.Sources[file] = target
-	}
-
-	pad := p.pad(slot)
+	pad := template.pad.Clone()
 	l, err := layer(pad)
 	if err != nil {
 		return err
 	}
 	var e edit
 	e.set(pad, "tempo", xpj.NewFloat(padTempo))
-	e.set(l, "sampleName", xpj.NewString(stem(file)))
-	e.set(l, "sampleFile", xpj.NewString(file))
+	e.set(l, "sampleName", xpj.NewString(stem(newFile)))
+	e.set(l, "sampleFile", xpj.NewString(newFile))
 	e.set(l, "sliceInfo.End", xpj.NewInt(info.Frames-1))
 	e.set(l, "sliceInfo.LoopCrossfadeLength", xpj.NewInt(crossfadeLength))
-	p.Dirty = true
-	return e.err
+	if e.err != nil {
+		return e.err
+	}
+
+	// Apply. None of these steps can fail: wrap checked that the pool
+	// and the pads are lists.
+	p.clear(slot, file)
+	p.pads().Items[slot] = pad
+	if file == "" {
+		p.addPoolEntry(newFile)
+		p.sources[newFile] = target
+	}
+	p.dirty = true
+	return nil
+}
+
+// freedBy returns the pool files that clearing the pad in slot would drop:
+// those no other pad uses, except keep.
+func (p *Project) freedBy(slot int, keep string) map[string]bool {
+	here := map[string]int{} // layers of this pad using each file
+	for _, f := range padFiles(p.pad(slot)) {
+		here[f]++
+	}
+	freed := map[string]bool{}
+	for f, n := range here {
+		if f != keep && p.usage(f) == n {
+			freed[f] = true
+		}
+	}
+	return freed
 }
 
 // fileForSource returns the pool file already copied from src, if any.
 func (p *Project) fileForSource(src string) string {
-	for file, s := range p.Sources {
+	for file, s := range p.sources {
 		if s == src && p.inPool(file) {
 			return file
 		}
@@ -224,9 +241,12 @@ func stem(file string) string { return strings.TrimSuffix(file, filepath.Ext(fil
 // uniqueFile returns base, or base with a 001, 002 ... suffix before the
 // extension, so that no two pool files share a name. Names are compared
 // without case, as on Windows, macOS and FAT.
-func (p *Project) uniqueFile(base string) string {
+func (p *Project) uniqueFile(base string, freed map[string]bool) string {
 	taken := map[string]bool{}
 	for _, e := range p.pool().Items {
+		if freed[text(e.Lookup("path"))] {
+			continue // about to be dropped
+		}
 		taken[strings.ToLower(text(e.Lookup("path")))] = true
 		taken[strings.ToLower(text(e.Lookup("name")))+"\x00"] = true
 	}
@@ -265,11 +285,11 @@ func poolEntry(file string) *xpj.Node {
 	return e
 }
 
-func (p *Project) addPoolEntry(file string) error {
-	if err := p.pool().Append(poolEntry(file)); err != nil {
-		return err
-	}
-	return p.trackPool().Insert(0, poolEntry(file))
+// addPoolEntry adds file to both pools. Both are lists (checked by wrap),
+// so this cannot fail.
+func (p *Project) addPoolEntry(file string) {
+	_ = p.pool().Append(poolEntry(file))
+	_ = p.trackPool().Insert(0, poolEntry(file))
 }
 
 func (p *Project) removePoolEntry(file string) {
@@ -280,7 +300,7 @@ func (p *Project) removePoolEntry(file string) {
 			}
 		}
 	}
-	delete(p.Sources, file)
+	delete(p.sources, file)
 }
 
 // ClearPad resets the pad in slot to the blank pad and drops pool entries
@@ -293,7 +313,7 @@ func (p *Project) ClearPad(slot int) error {
 		return nil
 	}
 	p.clear(slot, "")
-	p.Dirty = true
+	p.dirty = true
 	return nil
 }
 
@@ -323,7 +343,7 @@ func (p *Project) CopyPad(from, to int) error {
 	src := p.pad(from).Clone()
 	p.clear(to, "")
 	p.pads().Items[to] = src
-	p.Dirty = true
+	p.dirty = true
 	return nil
 }
 
@@ -353,7 +373,7 @@ func (p *Project) SwapPads(a, b int) error {
 	}
 	items := p.pads().Items
 	items[a], items[b] = items[b], items[a]
-	p.Dirty = true
+	p.dirty = true
 	return nil
 }
 
@@ -369,12 +389,19 @@ func (p *Project) SetTune(slot, coarse, fine int) error {
 	if err != nil {
 		return err
 	}
+	if l.Kind != xpj.Object {
+		return fmt.Errorf("project: pad %s layer 1 is %s, not an object", SlotName(slot), l.Kind)
+	}
+	// Setting keys on an object cannot fail, so all three are written.
 	var e edit
 	e.set(l, "coarseTune", xpj.NewInt(int64(coarse)))
 	e.set(l, "fineTune", xpj.NewInt(int64(fine)))
 	e.set(l, "pitch", xpj.NewFloat32(float64(coarse)+float64(fine)/100))
-	p.Dirty = true
-	return e.err
+	if e.err != nil {
+		return e.err
+	}
+	p.dirty = true
+	return nil
 }
 
 // SetMuteGroup sets the pad's mute group; 0 is off.
@@ -385,10 +412,11 @@ func (p *Project) SetMuteGroup(slot, group int) error {
 	if group < 0 || group > MaxMuteGroup {
 		return fmt.Errorf("mute group %d out of range 0-%d", group, MaxMuteGroup)
 	}
-	var e edit
-	e.set(p.pad(slot), "whichMuteGroup", xpj.NewInt(int64(group)))
-	p.Dirty = true
-	return e.err
+	if err := p.pad(slot).SetKey("whichMuteGroup", xpj.NewInt(int64(group))); err != nil {
+		return fmt.Errorf("project: pad %s: %w", SlotName(slot), err)
+	}
+	p.dirty = true
+	return nil
 }
 
 // PadHasNotes reports whether any sequence has a note or automation event

internal/project/project.go

@@ -55,11 +55,12 @@ type Project struct {
 	Name string
 	// SourceDir is the data folder of a loaded project; empty for a new one.
 	SourceDir string
-	// Sources maps each pool file name to the absolute path it is copied
+
+	// sources maps each pool file name to the absolute path it is copied
 	// from at export.
-	Sources map[string]string
-	// Dirty is set by every edit.
-	Dirty bool
+	sources map[string]string
+	// dirty is set by every edit that succeeds; see Dirty.
+	dirty bool
 
 	file *xpj.File
 	data *xpj.Node
@@ -135,7 +136,7 @@ func Load(path string) (*Project, []string, error) {
 		warnings = append(warnings, fmt.Sprintf("project data version %d; MPCkit is tested with %d", v, TestedVersion))
 	}
 	for _, file := range p.PoolFiles() {
-		p.Sources[file] = filepath.Join(dataDir, file)
+		p.sources[file] = filepath.Join(dataDir, file)
 	}
 	return p, warnings, nil
 }
@@ -145,7 +146,7 @@ func wrap(f *xpj.File, name, sourceDir string) (*Project, error) {
 	if err != nil {
 		return nil, err
 	}
-	p := &Project{Name: name, SourceDir: sourceDir, Sources: map[string]string{}, file: f, data: d}
+	p := &Project{Name: name, SourceDir: sourceDir, sources: map[string]string{}, file: f, data: d}
 	for _, l := range []struct {
 		path string
 		node **xpj.Node
@@ -264,6 +265,19 @@ func PlainFileName(name string) error {
 	return nil
 }
 
+// Dirty reports whether the project has changes not yet exported.
+func (p *Project) Dirty() bool { return p.dirty }
+
+// MarkExported records that the project as it is now has been exported.
+func (p *Project) MarkExported() { p.dirty = false }
+
+// Source returns the absolute path the pool file is copied from at export,
+// and whether one is recorded.
+func (p *Project) Source(file string) (string, bool) {
+	src, ok := p.sources[file]
+	return src, ok
+}
+
 // File returns the project file for writing. Callers must not edit it.
 func (p *Project) File() *xpj.File { return p.file }
 
@@ -274,7 +288,7 @@ func (p *Project) SetName(name string) error {
 	}
 	if name != p.Name {
 		p.Name = name
-		p.Dirty = true
+		p.dirty = true
 	}
 	return nil
 }

internal/project/project_test.go

@@ -233,7 +233,7 @@ func TestAssignAndPoolReuse(t *testing.T) {
 	if a.File != "kick.wav" || a.SampleName != "kick" || a.End != 99 {
 		t.Errorf("pad A2 = %+v", a)
 	}
-	if !p.Dirty {
+	if !p.dirty {
 		t.Error("not dirty after assign")
 	}
 	// Reassigning the same source to its only pad keeps the entry.
@@ -246,13 +246,13 @@ func TestAssignAndPoolReuse(t *testing.T) {
 	// temporary folder is behind one).
 	want, err := filepath.EvalSymlinks(kick)
 	must(t, err)
-	if got := p.Sources["kick.wav"]; got != want {
+	if got := p.sources["kick.wav"]; got != want {
 		t.Errorf("source = %q, want %q", got, want)
 	}
 	// Replacing drops the old entry when no pad uses it.
 	snare := writeWAV(t, filepath.Join(dir, "snare.wav"), 50)
 	must(t, p.AssignSample(0, snare))
-	if p.SampleCount() != 1 || p.inPool("kick.wav") || p.Sources["kick.wav"] != "" {
+	if p.SampleCount() != 1 || p.inPool("kick.wav") || p.sources["kick.wav"] != "" {
 		t.Errorf("replace did not drop kick: %d entries", p.SampleCount())
 	}
 }
@@ -287,7 +287,7 @@ func TestSampleLimit(t *testing.T) {
 	p := newProject(t)
 	// With one sample per pad the pool can only fill up when it also holds
 	// entries no pad uses, as a loaded project may. Simulate one.
-	must(t, p.addPoolEntry("unused.wav"))
+	p.addPoolEntry("unused.wav")
 	for i := 0; i < MaxSamples-1; i++ {
 		must(t, p.AssignSample(i, writeWAV(t, filepath.Join(dir, fmt.Sprintf("s%03d.wav", i)), 4)))
 	}
@@ -324,7 +324,7 @@ func TestAssignRejectsBadFiles(t *testing.T) {
 	if err := p.AssignSample(128, writeWAV(t, filepath.Join(dir, "ok.wav"), 4)); err == nil {
 		t.Error("slot 128 accepted")
 	}
-	if p.Dirty || p.SampleCount() != 0 {
+	if p.dirty || p.SampleCount() != 0 {
 		t.Error("failed assigns changed the project")
 	}
 }
@@ -379,12 +379,12 @@ func TestClearCopyMoveSwap(t *testing.T) {
 	}
 
 	// No-ops and range checks.
-	p.Dirty = false
+	p.dirty = false
 	must(t, p.ClearPad(100))
 	must(t, p.CopyPad(1, 1))
 	must(t, p.MovePad(1, 1))
 	must(t, p.SwapPads(1, 1))
-	if p.Dirty {
+	if p.dirty {
 		t.Error("no-op marked the project dirty")
 	}
 	for _, err := range []error{p.CopyPad(-1, 0), p.MovePad(0, 128), p.SwapPads(200, 0), p.ClearPad(128), p.SetTune(128, 0, 0), p.SetMuteGroup(-1, 0)} {
@@ -503,10 +503,10 @@ func TestLoadDemoFile(t *testing.T) {
 	if len(warnings) != 0 {
 		t.Errorf("warnings: %v", warnings)
 	}
-	if p.Name != "DemoFile" || p.Dirty || p.SampleCount() != 6 {
-		t.Errorf("name %q dirty %v samples %d", p.Name, p.Dirty, p.SampleCount())
+	if p.Name != "DemoFile" || p.dirty || p.SampleCount() != 6 {
+		t.Errorf("name %q dirty %v samples %d", p.Name, p.dirty, p.SampleCount())
 	}
-	for file, src := range p.Sources {
+	for file, src := range p.sources {
 		if _, err := os.Stat(src); err != nil {
 			t.Errorf("source for %s: %v", file, err)
 		}
@@ -552,7 +552,7 @@ func TestBlankPadsAndTemplate(t *testing.T) {
 func TestNames(t *testing.T) {
 	p := newProject(t)
 	must(t, p.SetName("My Kit 2"))
-	if p.Name != "My Kit 2" || !p.Dirty {
+	if p.Name != "My Kit 2" || !p.dirty {
 		t.Error("SetName")
 	}
 	for _, bad := range []string{"", "  ", "a:b", "a?", "end.", "end ", "CON", "com1.txt", "tab\there"} {
@@ -610,8 +610,8 @@ func TestAssignFollowsLink(t *testing.T) {
 	want, err := filepath.EvalSymlinks(kick)
 	must(t, err)
 	// The pool file is named after the link; it is copied from the target.
-	if got := p.PoolFiles(); len(got) != 1 || got[0] != "favourite.wav" || p.Sources["favourite.wav"] != want {
-		t.Errorf("pool %q, source %q; want favourite.wav from %q", got, p.Sources["favourite.wav"], want)
+	if got := p.PoolFiles(); len(got) != 1 || got[0] != "favourite.wav" || p.sources["favourite.wav"] != want {
+		t.Errorf("pool %q, source %q; want favourite.wav from %q", got, p.sources["favourite.wav"], want)
 	}
 	// The target itself is the same source: no second pool entry.
 	must(t, p.AssignSample(1, kick))
@@ -619,3 +619,119 @@ func TestAssignFollowsLink(t *testing.T) {
 		t.Errorf("%d pool entries, want 1", p.SampleCount())
 	}
 }
+
+// loadDemo loads a copy of the device's DemoFile, with its data folder,
+// after applying change to its data.
+func loadDemo(t *testing.T, change func(d *xpj.Node)) *Project {
+	t.Helper()
+	dir := t.TempDir()
+	f, err := xpj.ReadFile(filepath.Join(testdata, "device", "DemoFile.xpj"))
+	must(t, err)
+	d, err := f.Data()
+	must(t, err)
+	change(d)
+	path := filepath.Join(dir, "DemoFile.xpj")
+	must(t, f.WriteFile(path))
+	p, _, err := Load(path)
+	must(t, err)
+	return p
+}
+
+// unchanged fails the test if p differs from the encoding and sources
+// taken before, or has become dirty.
+func unchanged(t *testing.T, p *Project, before []byte, sources map[string]string) {
+	t.Helper()
+	if !bytes.Equal(encode(t, p), before) {
+		t.Error("the project file changed")
+	}
+	if fmt.Sprint(p.sources) != fmt.Sprint(sources) {
+		t.Errorf("sources changed: %v, want %v", p.sources, sources)
+	}
+	if p.Dirty() {
+		t.Error("dirty after a failed edit")
+	}
+}
+
+func TestFailedEditsChangeNothing(t *testing.T) {
+	dir := t.TempDir()
+	good := writeWAV(t, filepath.Join(dir, "good.wav"), 100)
+	notWAV := filepath.Join(dir, "notes.wav")
+	must(t, os.WriteFile(notWAV, []byte("not a WAV file"), 0o644))
+
+	p := loadDemo(t, func(d *xpj.Node) {
+		// A2's first layer is not an object, B5 is not an object at all,
+		// and a clip of the first sequence has no value.
+		must(t, d.Set("tracks[0].program.drum.instruments[1].layersv[0]", xpj.NewInt(7)))
+		must(t, d.Set("tracks[0].program.drum.instruments[20]", xpj.NewInt(7)))
+		clip, err := d.Get("sequences[0].value.trackClipMaps[0][3]")
+		must(t, err)
+		clip.DeleteKey("value")
+	})
+	before := encode(t, p)
+	sources := map[string]string{}
+	for k, v := range p.sources {
+		sources[k] = v
+	}
+	seq := p.Sequences()[1] // sequences[0] in the file is slot 1
+
+	for _, c := range []struct {
+		name string
+		edit func() error
+	}{
+		{"assign a missing file", func() error { return p.AssignSample(0, filepath.Join(dir, "gone.wav")) }},
+		{"assign a file that is not a WAV", func() error { return p.AssignSample(0, notWAV) }},
+		{"tune a pad whose layer is not an object", func() error { return p.SetTune(1, 3, 0) }},
+		{"mute group on a pad that is not an object", func() error { return p.SetMuteGroup(20, 2) }},
+		{"sequence whose clip has no value", func() error { return p.SetSequence(seq.Slot, "Renamed", 99, 8) }},
+		{"bad slot", func() error { return p.SetTune(Pads, 0, 0) }},
+	} {
+		t.Run(c.name, func(t *testing.T) {
+			if err := c.edit(); err == nil {
+				t.Fatal("no error")
+			}
+			unchanged(t, p, before, sources)
+		})
+	}
+
+	// The sample limit: a full pool refuses a new file and changes nothing.
+	full := newProject(t)
+	for i := range MaxSamples - 1 {
+		must(t, full.AssignSample(i, writeWAV(t, filepath.Join(dir, fmt.Sprintf("g%03d.wav", i)), 4)))
+	}
+	full.addPoolEntry("unused.wav") // a 128th entry no pad uses
+	full.MarkExported()
+	before = encode(t, full)
+	sources = map[string]string{}
+	for k, v := range full.sources {
+		sources[k] = v
+	}
+	if err := full.AssignSample(MaxSamples-1, good); err == nil {
+		t.Fatal("129th sample accepted")
+	}
+	unchanged(t, full, before, sources)
+}
+
+func TestAssignReplacesBrokenPad(t *testing.T) {
+	// AssignSample starts from the blank pad, so a pad with no layers is
+	// replaced whole rather than failing halfway.
+	p := loadDemo(t, func(d *xpj.Node) {
+		must(t, d.Set("tracks[0].program.drum.instruments[5].layersv", xpj.NewArray()))
+	})
+	kick := writeWAV(t, filepath.Join(t.TempDir(), "kick.wav"), 100)
+	must(t, p.AssignSample(5, kick))
+	if v := padOf(t, p, 5); v.File != "kick.wav" || v.End != 99 {
+		t.Errorf("pad A6 = %+v", v)
+	}
+}
+
+func TestAssignReusesFreedName(t *testing.T) {
+	// Replacing a pad's only sample frees its name for the new file, as
+	// before edits were made all-or-nothing.
+	dir := t.TempDir()
+	p := newProject(t)
+	must(t, p.AssignSample(0, writeWAV(t, filepath.Join(dir, "a", "kick.wav"), 10)))
+	must(t, p.AssignSample(0, writeWAV(t, filepath.Join(dir, "b", "kick.wav"), 20)))
+	if got := p.PoolFiles(); len(got) != 1 || got[0] != "kick.wav" {
+		t.Errorf("pool %q, want [kick.wav]", got)
+	}
+}

internal/project/sequences.go

@@ -1,6 +1,7 @@
 package project
 
 import (
+	"errors"
 	"fmt"
 	"sort"
 
@@ -86,8 +87,10 @@ func (p *Project) AddSequence(slot int) error {
 	if err := setLength(v, 2); err != nil {
 		return err
 	}
-	p.Dirty = true
-	return p.sequences().Insert(0, s)
+	// The edits above were on a copy; inserting into a list cannot fail.
+	_ = p.sequences().Insert(0, s)
+	p.dirty = true
+	return nil
 }
 
 // RemoveSequence deletes the sequence in slot. The last sequence cannot be
@@ -101,14 +104,22 @@ func (p *Project) RemoveSequence(slot int) error {
 	if len(p.sequences().Items) == 1 {
 		return fmt.Errorf("cannot remove the only sequence")
 	}
-	if err := p.sequences().Remove(i); err != nil {
-		return err
-	}
-	p.Dirty = true
+	// If it is the current sequence, the lowest other slot becomes current.
+	next := -1
 	if cur := p.data.Lookup("currentSequence"); cur != nil && num(cur) == int64(slot) {
-		lowest := p.Sequences()[0].Slot
-		return p.data.SetKey("currentSequence", xpj.NewInt(int64(lowest)))
+		for _, q := range p.Sequences() {
+			if q.Slot != slot {
+				next = q.Slot
+				break
+			}
+		}
+	}
+	// Neither step can fail: i is in range and data is an object.
+	_ = p.sequences().Remove(i)
+	if next >= 0 {
+		_ = p.data.SetKey("currentSequence", xpj.NewInt(int64(next)))
 	}
+	p.dirty = true
 	return nil
 }
 
@@ -129,38 +140,64 @@ func (p *Project) SetSequence(slot int, name string, bpm float64, bars int) erro
 		return fmt.Errorf("length must be 1-%d bars", MaxBars)
 	}
 	v := p.sequences().Items[i].Lookup("value")
-	var e edit
-	e.set(v, "name", xpj.NewString(name))
-	e.set(v, "bpm", xpj.NewFloat(bpm))
-	if e.err != nil {
-		return e.err
-	}
-	if err := setLength(v, bars); err != nil {
+	clips, err := lengthClips(v)
+	if err != nil {
 		return err
 	}
+	// wrap checked that value is an object, and lengthClips that each clip
+	// has a value object, so none of these writes can fail.
+	_ = v.SetKey("name", xpj.NewString(name))
+	_ = v.SetKey("bpm", xpj.NewFloat(bpm))
+	writeLength(v, clips, bars)
 	p.lastBPM = bpm
-	p.Dirty = true
+	p.dirty = true
 	return nil
 }
 
 // setLength writes the 26 fields that hold a sequence's length: four on the
-// sequence and two on each of its 11 clips.
+// sequence and two on each of its 11 clips. It writes nothing if the
+// sequence does not have that shape.
 func setLength(v *xpj.Node, bars int) error {
-	pulses := int64(bars) * PulsesPerBar
-	var e edit
-	e.set(v, "lengthBars", xpj.NewInt(int64(bars)))
-	e.set(v, "loopEndBar", xpj.NewInt(int64(bars)))
-	e.set(v, "lengthPulses", xpj.NewInt(pulses))
-	e.set(v, "loopEndPulses", xpj.NewInt(pulses))
+	clips, err := lengthClips(v)
+	if err != nil {
+		return err
+	}
+	writeLength(v, clips, bars)
+	return nil
+}
+
+// lengthClips returns the value object of each of the sequence's clips,
+// checking that every object setLength writes to is there.
+func lengthClips(v *xpj.Node) ([]*xpj.Node, error) {
+	if v == nil || v.Kind != xpj.Object {
+		return nil, errors.New("project: sequence value is not an object")
+	}
 	clips, err := v.Get("trackClipMaps[0]")
 	if err != nil {
-		return fmt.Errorf("project: %w", err)
+		return nil, fmt.Errorf("project: %w", err)
+	}
+	var values []*xpj.Node
+	for i, c := range clips.Items {
+		cv := c.Lookup("value")
+		if cv == nil || cv.Kind != xpj.Object {
+			return nil, fmt.Errorf("project: clip %d has no value object", i)
+		}
+		values = append(values, cv)
 	}
-	for _, c := range clips.Items {
-		e.set(c, "value.endPulses", xpj.NewInt(pulses))
-		e.set(c, "value.loopEndPulses", xpj.NewInt(pulses))
+	return values, nil
+}
+
+// writeLength writes a length lengthClips has checked; it cannot fail.
+func writeLength(v *xpj.Node, clips []*xpj.Node, bars int) {
+	pulses := int64(bars) * PulsesPerBar
+	_ = v.SetKey("lengthBars", xpj.NewInt(int64(bars)))
+	_ = v.SetKey("loopEndBar", xpj.NewInt(int64(bars)))
+	_ = v.SetKey("lengthPulses", xpj.NewInt(pulses))
+	_ = v.SetKey("loopEndPulses", xpj.NewInt(pulses))
+	for _, cv := range clips {
+		_ = cv.SetKey("endPulses", xpj.NewInt(pulses))
+		_ = cv.SetKey("loopEndPulses", xpj.NewInt(pulses))
 	}
-	return e.err
 }
 
 // hasNotes reports whether a sequence value holds any note event.

pull-requests/atomic-edits/all-or-nothing-edits.md

@@ -0,0 +1,59 @@
+# All-or-nothing edits
+
+Branch: `atomic-edits` → `main`
+
+PR 6 of the review fixes plan (`docs/review-fixes-plan.md`), part of the 0.5.3 series; the version does not change in this PR.
+
+## What changed
+
+**Each edit checks first, then changes the project (M3).** An edit that fails now leaves the project exactly as it was, and only an edit that succeeds marks it as having unsaved changes.
+- **`AssignSample`** works out everything before touching the project: which pool entry to reuse, which entries replacing the pad frees, the sample limit, the new file name, and the new pad, built in full from the blank pad. Only then does it replace the pad and add the pool entry. None of those last steps can fail.
+  - The name of a sample freed by the replacement can still be reused, as before: replacing `kick.wav` with a different `kick.wav` keeps the name instead of making it `kick001.wav`. A new `freedBy` helper works this out, and also handles a file used by two layers of the same pad.
+- **`SetTune`** checks the pad's first layer is an object before writing. Before, if it was not, the project was marked changed anyway.
+- **`SetMuteGroup`** marks the project changed only if the write worked.
+- **`SetSequence`** looks up and checks every clip before writing anything. Before, if a clip had no `value`, the new name, BPM and length were already half written when it failed. The length write is split into `lengthClips` (check) and `writeLength` (write); `setLength`, used by `AddSequence`, uses both.
+- **`AddSequence` and `RemoveSequence`** set `dirty` after the change rather than before. `RemoveSequence` picks the next current sequence before removing anything.
+
+**Project fields are written only by `project` (M4).**
+- `Dirty` and `Sources` are now the unexported `dirty` and `sources`.
+- New methods replace them: `Dirty()` reads the flag, `MarkExported()` clears it after an export, and `Source(file)` returns a pool file's source and whether one is recorded.
+- `app` and `export` use these. The CLI and tools did not use either field.
+
+**Tests.**
+- **`TestFailedEditsChangeNothing`:** each of these edits fails on a loaded DemoFile and leaves the encoded project, the sources and `Dirty()` unchanged:
+  - assigning a missing file;
+  - assigning a file that is not a WAV;
+  - tuning a pad whose layer is not an object;
+  - setting the mute group of a pad that is not an object;
+  - editing a sequence with a broken clip;
+  - a bad slot;
+  - assigning a 129th sample.
+  
+  Against the old ordering, the tune, mute-group and sequence cases fail.
+- **`TestAssignReplacesBrokenPad`:** the plan's example, a pad whose `layersv` is empty. `AssignSample` does not fail on it: it starts from the blank pad, so the broken pad is replaced whole.
+- **`TestAssignReusesFreedName`:** the freed-name behaviour above.
+- **Export tests:** the two that set `Sources` directly now rename a pool entry in the tree, or call the name check and the zip writer directly.
+
+## Notes
+
+- **The plan's example does not fail (see the test above).** `AssignSample` replaces the pad with the blank pad before writing, so a pad with no layers is simply replaced. The failures that can actually happen are now all covered.
+- **The second-line name check in export.** Only `project` can record a source now, so `Folder` and `Zip` can no longer be handed a bad pool name from outside, and their check is reached only if `project` itself has a bug. The test calls the check and the zip writer directly. `internal/export` coverage is 85.9%, still above PR 3's 85%.
+- **One test still writes a field from outside `project`:** `export_test.go` sets `Name` to `a/b` to test the export name check, since `SetName` refuses such a name. No non-test code outside `project` writes a project field. `Name` and `SourceDir` stay exported, as the plan only names `Dirty` and `Sources`.
+- **Nothing changes in the files MPCkit writes.** The golden tests (MpckitT1, T3, Demo02) and the round-trip tests pass unchanged.
+
+## Testing
+
+**On the server, every command with `GOTOOLCHAIN=go1.27.4`:**
+- `go test ./...` passes, including the golden and round-trip tests.
+- `go vet ./...` (also with `GOOS=windows`) and `staticcheck ./...` are clean.
+- `govulncheck ./...`: "No vulnerabilities found."
+- `npm install`, `npm run check` (0 errors, 0 warnings) and `npm run build` pass. The lockfile was not committed. The bindings did not change.
+- `wails3 build` builds, and the app starts under Xvfb.
+
+**On the laptop (PowerShell, in the repo):**
+1. `git fetch; git checkout atomic-edits`
+2. `go test ./...`: all `ok`.
+3. `wails3 dev`: load `testdata\device\DemoFile.xpj`. The footer shows no unsaved changes.
+4. Assign a file that is not a WAV to pad A1, for example by renaming a text file to `.wav`. An error shows, A1 keeps its sample, and the footer still shows no unsaved changes.
+5. Assign a real WAV to A1: it replaces the sample, and the footer shows unsaved changes.
+6. Change a tune, a mute group, and a sequence's name, BPM and length. Each works as before. Export to folder, then load the export: the changes are there.