0780e0d fix(graphics): close phase2 review findings · vev-vt

M graphics/graphics.go +4 -1


@@ -113,7 +113,8 @@ type AssetSpec = AssetBlob
113	113		// PlacementSpec describes one sparse placement. Source defaults to the full
114	114		// asset. Destination is a pixel rectangle; Dest is accepted as a concise alias
115	115		// for callers that use that spelling. Cells optionally records the cell-space
116		-	// extent associated with the same placement.
	116	+	// extent associated with the same placement. HasCells and HasLayer preserve
	117	+	// explicit zero values when updating an existing placement.
117	118		type PlacementSpec struct {
118	119		Asset AssetID
119	120		AssetID AssetID

@@ -122,7 +123,9 @@ type PlacementSpec struct {
122	123		Dest PixelRect
123	124		Cells CellRect
124	125		CellBounds CellRect
	126	+	HasCells bool
125	127		Layer int64
	128	+	HasLayer bool
126	129		}
127	130
128	131		// Viewport can constrain visible fragments in pixel space, cell space, or

 // PlacementSpec describes one sparse placement. Source defaults to the full
 // asset. Destination is a pixel rectangle; Dest is accepted as a concise alias
 // for callers that use that spelling. Cells optionally records the cell-space
-// extent associated with the same placement.
+// extent associated with the same placement. HasCells and HasLayer preserve
+// explicit zero values when updating an existing placement.
 type PlacementSpec struct {
 	Asset       AssetID
 	AssetID     AssetID
 	Dest        PixelRect
 	Cells       CellRect
 	CellBounds  CellRect
+	HasCells    bool
 	Layer       int64
+	HasLayer    bool
 }
 // Viewport can constrain visible fragments in pixel space, cell space, or

M graphics/scene.go +64 -5


@@ -129,6 +129,51 @@ func (s *Scene) AddAsset(blob AssetBlob) (AssetID, error) {
129	129		// RegisterAsset is an alias for AddAsset.
130	130		func (s *Scene) RegisterAsset(blob AssetBlob) (AssetID, error) { return s.AddAsset(blob) }
131	131
	132	+	// ReplaceAsset atomically replaces an asset while removing all placements that
	133	+	// reference it. If validation or resource limits reject the new asset, the
	134	+	// existing asset and placements remain unchanged.
	135	+	func (s *Scene) ReplaceAsset(id AssetID, blob AssetBlob) (AssetID, error) {
	136	+	if s == nil {
	137	+	return AssetID{}, fmt.Errorf("replace asset: %w", ErrAssetNotFound)
	138	+	}
	139	+	s.mu.Lock()
	140	+	defer s.mu.Unlock()
	141	+	candidate := cloneState(s.state)
	142	+	old, ok := candidate.assets[id]
	143	+	if !ok {
	144	+	return AssetID{}, fmt.Errorf("replace asset: %w", ErrAssetNotFound)
	145	+	}
	146	+	for placementID, placement := range candidate.placements {
	147	+	if placement.asset != id {
	148	+	continue
	149	+	}
	150	+	delete(candidate.placements, placementID)
	151	+	candidate.usage.Placements--
	152	+	}
	153	+	delete(candidate.assets, id)
	154	+	candidate.usage.Assets--
	155	+	candidate.usage.EncodedBytes -= uint64(len(old.encoded))
	156	+	candidate.usage.DecodedPixels -= old.pixels
	157	+	replacement, err := s.prepareAsset(blob, candidate)
	158	+	if err != nil {
	159	+	return AssetID{}, err
	160	+	}
	161	+	replacementID, ok := takeAssetID(candidate)
	162	+	if !ok {
	163	+	return AssetID{}, fmt.Errorf("replace asset: %w", ErrIdentifierOverflow)
	164	+	}
	165	+	replacement.id = replacementID
	166	+	candidate.assets[replacementID] = replacement
	167	+	candidate.usage.Assets++
	168	+	candidate.usage.EncodedBytes += uint64(len(replacement.encoded))
	169	+	candidate.usage.DecodedPixels += replacement.pixels
	170	+	if err := advanceGeneration(candidate); err != nil {
	171	+	return AssetID{}, fmt.Errorf("replace asset: %w", err)
	172	+	}
	173	+	s.state = candidate
	174	+	return replacementID, nil
	175	+	}
	176	+
132	177		// AddEncodedAsset registers an asset from its encoded bytes and decoded pixel
133	178		// dimensions. The input bytes are copied before this method returns.
134	179		func (s *Scene) AddEncodedAsset(encoded []byte, width, height int64) (AssetID, error) {

@@ -418,14 +463,14 @@ func (s Scene) preparePlacement(state sceneState, spec PlacementSpec, existing
418	463		if cells == (CellRect{}) {
419	464		cells = spec.CellBounds
420	465		}
421		-	if existing != nil && cells == (CellRect{}) {
	466	+	if existing != nil && !spec.HasCells && cells == (CellRect{}) {
422	467		cells = existing.cells
423	468		}
424	469		if cells != (CellRect{}) && (!cells.Valid() \|\| cells.Empty()) {
425	470		return placementRecord{}, fmt.Errorf("place: %w", ErrInvalidPlacement)
426	471		}
427	472		layer := spec.Layer
428		-	if existing != nil && layer == 0 {
	473	+	if existing != nil && !spec.HasLayer {
429	474		layer = existing.layer
430	475		}
431	476		return placementRecord{asset: asset, source: source, destination: destination, cells: cells, layer: layer}, nil

@@ -651,7 +696,15 @@ func (p PlacementView) Destination() PixelRect { return p.destination }
651	696		func (p PlacementView) Cells() CellRect { return p.cells }
652	697		func (p PlacementView) Layer() int64 { return p.layer }
653	698		func (p PlacementView) Spec() PlacementSpec {
654		-	return PlacementSpec{Asset: p.asset, Source: p.source, Destination: p.destination, Cells: p.cells, Layer: p.layer}
	699	+	return PlacementSpec{
	700	+	Asset: p.asset,
	701	+	Source: p.source,
	702	+	Destination: p.destination,
	703	+	Cells: p.cells,
	704	+	HasCells: p.cells != (CellRect{}),
	705	+	Layer: p.layer,
	706	+	HasLayer: true,
	707	+	}
655	708		}
656	709
657	710		// Snapshot is an immutable reference to a scene state.

@@ -782,8 +835,14 @@ func viewportParts(viewport any) (PixelRect, CellRect, bool, bool) {
782	835		case CellRect:
783	836		return PixelRect{}, value, false, value.Valid() && !value.Empty()
784	837		case Viewport:
785		-	hasPixels := value.Pixels.Valid() && !value.Pixels.Empty()
786		-	hasCells := value.Cells.Valid() && !value.Cells.Empty()
	838	+	hasPixels := value.Pixels != (PixelRect{})
	839	+	hasCells := value.Cells != (CellRect{})
	840	+	if hasPixels && (!value.Pixels.Valid() \|\| value.Pixels.Empty()) {
	841	+	return PixelRect{}, CellRect{}, false, false
	842	+	}
	843	+	if hasCells && (!value.Cells.Valid() \|\| value.Cells.Empty()) {
	844	+	return PixelRect{}, CellRect{}, false, false
	845	+	}
787	846		return value.Pixels, value.Cells, hasPixels, hasCells
788	847		case *Viewport:
789	848		if value == nil {


@@ -129,6 +129,51 @@ func (s *Scene) AddAsset(blob AssetBlob) (AssetID, error) {
129		// RegisterAsset is an alias for AddAsset.	129		// RegisterAsset is an alias for AddAsset.
130		func (s *Scene) RegisterAsset(blob AssetBlob) (AssetID, error) { return s.AddAsset(blob) }	130		func (s *Scene) RegisterAsset(blob AssetBlob) (AssetID, error) { return s.AddAsset(blob) }
131			131
			132	+	// ReplaceAsset atomically replaces an asset while removing all placements that
			133	+	// reference it. If validation or resource limits reject the new asset, the
			134	+	// existing asset and placements remain unchanged.
			135	+	func (s *Scene) ReplaceAsset(id AssetID, blob AssetBlob) (AssetID, error) {
			136	+	if s == nil {
			137	+	return AssetID{}, fmt.Errorf("replace asset: %w", ErrAssetNotFound)
			138	+	}
			139	+	s.mu.Lock()
			140	+	defer s.mu.Unlock()
			141	+	candidate := cloneState(s.state)
			142	+	old, ok := candidate.assets[id]
			143	+	if !ok {
			144	+	return AssetID{}, fmt.Errorf("replace asset: %w", ErrAssetNotFound)
			145	+	}
			146	+	for placementID, placement := range candidate.placements {
			147	+	if placement.asset != id {
			148	+	continue
			149	+	}
			150	+	delete(candidate.placements, placementID)
			151	+	candidate.usage.Placements--
			152	+	}
			153	+	delete(candidate.assets, id)
			154	+	candidate.usage.Assets--
			155	+	candidate.usage.EncodedBytes -= uint64(len(old.encoded))
			156	+	candidate.usage.DecodedPixels -= old.pixels
			157	+	replacement, err := s.prepareAsset(blob, candidate)
			158	+	if err != nil {
			159	+	return AssetID{}, err
			160	+	}
			161	+	replacementID, ok := takeAssetID(candidate)
			162	+	if !ok {
			163	+	return AssetID{}, fmt.Errorf("replace asset: %w", ErrIdentifierOverflow)
			164	+	}
			165	+	replacement.id = replacementID
			166	+	candidate.assets[replacementID] = replacement
			167	+	candidate.usage.Assets++
			168	+	candidate.usage.EncodedBytes += uint64(len(replacement.encoded))
			169	+	candidate.usage.DecodedPixels += replacement.pixels
			170	+	if err := advanceGeneration(candidate); err != nil {
			171	+	return AssetID{}, fmt.Errorf("replace asset: %w", err)
			172	+	}
			173	+	s.state = candidate
			174	+	return replacementID, nil
			175	+	}
			176	+
132		// AddEncodedAsset registers an asset from its encoded bytes and decoded pixel	177		// AddEncodedAsset registers an asset from its encoded bytes and decoded pixel
133		// dimensions. The input bytes are copied before this method returns.	178		// dimensions. The input bytes are copied before this method returns.
134		func (s *Scene) AddEncodedAsset(encoded []byte, width, height int64) (AssetID, error) {	179		func (s *Scene) AddEncodedAsset(encoded []byte, width, height int64) (AssetID, error) {

@@ -418,14 +463,14 @@ func (s Scene) preparePlacement(state sceneState, spec PlacementSpec, existing
418		if cells == (CellRect{}) {	463		if cells == (CellRect{}) {
419		cells = spec.CellBounds	464		cells = spec.CellBounds
420		}	465		}
421	-	if existing != nil && cells == (CellRect{}) {	466	+	if existing != nil && !spec.HasCells && cells == (CellRect{}) {
422		cells = existing.cells	467		cells = existing.cells
423		}	468		}
424		if cells != (CellRect{}) && (!cells.Valid() \|\| cells.Empty()) {	469		if cells != (CellRect{}) && (!cells.Valid() \|\| cells.Empty()) {
425		return placementRecord{}, fmt.Errorf("place: %w", ErrInvalidPlacement)	470		return placementRecord{}, fmt.Errorf("place: %w", ErrInvalidPlacement)
426		}	471		}
427		layer := spec.Layer	472		layer := spec.Layer
428	-	if existing != nil && layer == 0 {	473	+	if existing != nil && !spec.HasLayer {
429		layer = existing.layer	474		layer = existing.layer
430		}	475		}
431		return placementRecord{asset: asset, source: source, destination: destination, cells: cells, layer: layer}, nil	476		return placementRecord{asset: asset, source: source, destination: destination, cells: cells, layer: layer}, nil

@@ -651,7 +696,15 @@ func (p PlacementView) Destination() PixelRect { return p.destination }
651		func (p PlacementView) Cells() CellRect { return p.cells }	696		func (p PlacementView) Cells() CellRect { return p.cells }
652		func (p PlacementView) Layer() int64 { return p.layer }	697		func (p PlacementView) Layer() int64 { return p.layer }
653		func (p PlacementView) Spec() PlacementSpec {	698		func (p PlacementView) Spec() PlacementSpec {
654	-	return PlacementSpec{Asset: p.asset, Source: p.source, Destination: p.destination, Cells: p.cells, Layer: p.layer}	699	+	return PlacementSpec{
			700	+	Asset: p.asset,
			701	+	Source: p.source,
			702	+	Destination: p.destination,
			703	+	Cells: p.cells,
			704	+	HasCells: p.cells != (CellRect{}),
			705	+	Layer: p.layer,
			706	+	HasLayer: true,
			707	+	}
655		}	708		}
656			709
657		// Snapshot is an immutable reference to a scene state.	710		// Snapshot is an immutable reference to a scene state.

@@ -782,8 +835,14 @@ func viewportParts(viewport any) (PixelRect, CellRect, bool, bool) {
782		case CellRect:	835		case CellRect:
783		return PixelRect{}, value, false, value.Valid() && !value.Empty()	836		return PixelRect{}, value, false, value.Valid() && !value.Empty()
784		case Viewport:	837		case Viewport:
785	-	hasPixels := value.Pixels.Valid() && !value.Pixels.Empty()	838	+	hasPixels := value.Pixels != (PixelRect{})
786	-	hasCells := value.Cells.Valid() && !value.Cells.Empty()	839	+	hasCells := value.Cells != (CellRect{})
			840	+	if hasPixels && (!value.Pixels.Valid() \|\| value.Pixels.Empty()) {
			841	+	return PixelRect{}, CellRect{}, false, false
			842	+	}
			843	+	if hasCells && (!value.Cells.Valid() \|\| value.Cells.Empty()) {
			844	+	return PixelRect{}, CellRect{}, false, false
			845	+	}
787		return value.Pixels, value.Cells, hasPixels, hasCells	846		return value.Pixels, value.Cells, hasPixels, hasCells
788		case *Viewport:	847		case *Viewport:
789		if value == nil {	848		if value == nil {

M graphics/scene_test.go +55 -0


@@ -164,3 +164,58 @@ func TestVisibleFragmentsPreserveScaledSourceMapping(t *testing.T) {
164	164		t.Fatalf("cell clipping = %#v", cellFragments)
165	165		}
166	166		}
	167	+
	168	+	func TestVisibleFragmentsRejectInvalidMixedViewport(t *testing.T) {
	169	+	scene := NewScene(Limits{MaxEncodedBytes: 64, MaxDecodedPixels: 64})
	170	+	assetID, err := scene.AddAsset(testAsset(t, []byte{1}, 2, 2))
	171	+	if err != nil {
	172	+	t.Fatal(err)
	173	+	}
	174	+	if _, err := scene.Place(PlacementSpec{
	175	+	Asset: assetID,
	176	+	Destination: PixelRect{Width: 2, Height: 2},
	177	+	Cells: CellRect{Width: 2, Height: 2},
	178	+	}); err != nil {
	179	+	t.Fatal(err)
	180	+	}
	181	+	for _, viewport := range []Viewport{
	182	+	{Pixels: PixelRect{Width: 2, Height: 2}, Cells: CellRect{Width: -1, Height: 1}},
	183	+	{Pixels: PixelRect{Width: 2, Height: 2}, Cells: CellRect{X: 1}},
	184	+	{Pixels: PixelRect{Width: -1, Height: 1}, Cells: CellRect{Width: 2, Height: 2}},
	185	+	} {
	186	+	if fragments := scene.Snapshot().VisibleFragments(viewport); len(fragments) != 0 {
	187	+	t.Fatalf("invalid mixed viewport %#v produced fragments %#v", viewport, fragments)
	188	+	}
	189	+	}
	190	+	}
	191	+
	192	+	func TestUpdatePlacementCanResetLayerAndClearCells(t *testing.T) {
	193	+	scene := NewScene(Limits{MaxEncodedBytes: 64, MaxDecodedPixels: 64})
	194	+	assetID, err := scene.AddAsset(testAsset(t, []byte{1}, 4, 4))
	195	+	if err != nil {
	196	+	t.Fatal(err)
	197	+	}
	198	+	placementID, err := scene.Place(PlacementSpec{
	199	+	Asset: assetID,
	200	+	Destination: PixelRect{Width: 4, Height: 4},
	201	+	Cells: CellRect{X: 1, Y: 2, Width: 3, Height: 2},
	202	+	Layer: 7,
	203	+	HasLayer: true,
	204	+	})
	205	+	if err != nil {
	206	+	t.Fatal(err)
	207	+	}
	208	+	if err := scene.UpdatePlacement(placementID, PlacementSpec{HasCells: true, HasLayer: true}); err != nil {
	209	+	t.Fatal(err)
	210	+	}
	211	+	placement, ok := scene.Snapshot().Placement(placementID)
	212	+	if !ok {
	213	+	t.Fatal("updated placement missing")
	214	+	}
	215	+	if placement.Layer() != 0 {
	216	+	t.Fatalf("layer = %d, want 0", placement.Layer())
	217	+	}
	218	+	if placement.Cells() != (CellRect{}) {
	219	+	t.Fatalf("cells = %#v, want cleared", placement.Cells())
	220	+	}
	221	+	}


@@ -164,3 +164,58 @@ func TestVisibleFragmentsPreserveScaledSourceMapping(t *testing.T) {
164	t.Fatalf("cell clipping = %#v", cellFragments)	164		t.Fatalf("cell clipping = %#v", cellFragments)
165	}	165		}
166	}	166		}
		167	+
		168	+	func TestVisibleFragmentsRejectInvalidMixedViewport(t *testing.T) {
		169	+	scene := NewScene(Limits{MaxEncodedBytes: 64, MaxDecodedPixels: 64})
		170	+	assetID, err := scene.AddAsset(testAsset(t, []byte{1}, 2, 2))
		171	+	if err != nil {
		172	+	t.Fatal(err)
		173	+	}
		174	+	if _, err := scene.Place(PlacementSpec{
		175	+	Asset: assetID,
		176	+	Destination: PixelRect{Width: 2, Height: 2},
		177	+	Cells: CellRect{Width: 2, Height: 2},
		178	+	}); err != nil {
		179	+	t.Fatal(err)
		180	+	}
		181	+	for _, viewport := range []Viewport{
		182	+	{Pixels: PixelRect{Width: 2, Height: 2}, Cells: CellRect{Width: -1, Height: 1}},
		183	+	{Pixels: PixelRect{Width: 2, Height: 2}, Cells: CellRect{X: 1}},
		184	+	{Pixels: PixelRect{Width: -1, Height: 1}, Cells: CellRect{Width: 2, Height: 2}},
		185	+	} {
		186	+	if fragments := scene.Snapshot().VisibleFragments(viewport); len(fragments) != 0 {
		187	+	t.Fatalf("invalid mixed viewport %#v produced fragments %#v", viewport, fragments)
		188	+	}
		189	+	}
		190	+	}
		191	+
		192	+	func TestUpdatePlacementCanResetLayerAndClearCells(t *testing.T) {
		193	+	scene := NewScene(Limits{MaxEncodedBytes: 64, MaxDecodedPixels: 64})
		194	+	assetID, err := scene.AddAsset(testAsset(t, []byte{1}, 4, 4))
		195	+	if err != nil {
		196	+	t.Fatal(err)
		197	+	}
		198	+	placementID, err := scene.Place(PlacementSpec{
		199	+	Asset: assetID,
		200	+	Destination: PixelRect{Width: 4, Height: 4},
		201	+	Cells: CellRect{X: 1, Y: 2, Width: 3, Height: 2},
		202	+	Layer: 7,
		203	+	HasLayer: true,
		204	+	})
		205	+	if err != nil {
		206	+	t.Fatal(err)
		207	+	}
		208	+	if err := scene.UpdatePlacement(placementID, PlacementSpec{HasCells: true, HasLayer: true}); err != nil {
		209	+	t.Fatal(err)
		210	+	}
		211	+	placement, ok := scene.Snapshot().Placement(placementID)
		212	+	if !ok {
		213	+	t.Fatal("updated placement missing")
		214	+	}
		215	+	if placement.Layer() != 0 {
		216	+	t.Fatalf("layer = %d, want 0", placement.Layer())
		217	+	}
		218	+	if placement.Cells() != (CellRect{}) {
		219	+	t.Fatalf("cells = %#v, want cleared", placement.Cells())
		220	+	}
		221	+	}

M graphics_state.go +1 -0


@@ -36,6 +36,7 @@ func (g *screenGraphicsState) clearPlacements() {
36	36		if g == nil \|\| g.kitty == nil {
37	37		return
38	38		}
	39	+	g.kitty.AbortPendingUpload()
39	40		_, _ = g.kitty.Process(kittygraphics.Command{Controls: kittygraphics.Controls{
40	41		Action: kittygraphics.ActionDelete,
41	42		HasAction: true,


@@ -36,6 +36,7 @@ func (g *screenGraphicsState) clearPlacements() {
36	if g == nil \|\| g.kitty == nil {	36		if g == nil \|\| g.kitty == nil {
37	return	37		return
38	}	38		}
		39	+	g.kitty.AbortPendingUpload()
39	_, _ = g.kitty.Process(kittygraphics.Command{Controls: kittygraphics.Controls{	40		_, _ = g.kitty.Process(kittygraphics.Command{Controls: kittygraphics.Controls{
40	Action: kittygraphics.ActionDelete,	41		Action: kittygraphics.ActionDelete,
41	HasAction: true,	42		HasAction: true,

M protocol/kittygraphics/kittygraphics_test.go +90 -0


@@ -216,6 +216,96 @@ func TestChunkedTransmitAndDisplayRetainsDisplayAction(t *testing.T) {
216	216		}
217	217		}
218	218
	219	+	func TestPlacementFailureEmitsStableProtocolError(t *testing.T) {
	220	+	scene := graphics.NewScene(graphics.Limits{})
	221	+	session := NewSession(scene)
	222	+	if _, err := session.Feed(apc("a=t,i=1,f=32,s=1,v=1,q=2", "AQIDBA")); err != nil {
	223	+	t.Fatal(err)
	224	+	}
	225	+	result, err := session.Feed(apc("a=p,i=1,p=7,w=2,q=0", ""))
	226	+	if !errors.Is(err, graphics.ErrInvalidPlacement) {
	227	+	t.Fatalf("placement error = %v", err)
	228	+	}
	229	+	if got := result.Bytes(); string(got) != "\x1b_Gi=1;EINVAL\x1b\\" {
	230	+	t.Fatalf("placement response = %q", got)
	231	+	}
	232	+	if len(result.Mutations) != 0 \|\| scene.Usage().Placements != 0 {
	233	+	t.Fatalf("failed placement mutated state: result=%#v usage=%#v", result, scene.Usage())
	234	+	}
	235	+	}
	236	+
	237	+	func TestFailedImageReplacementPreservesAssetAndPlacements(t *testing.T) {
	238	+	scene := graphics.NewScene(graphics.Limits{MaxEncodedBytes: 4, MaxDecodedPixels: 4})
	239	+	session := NewSession(scene)
	240	+	if _, err := session.Feed(apc("a=t,i=1,f=32,s=1,v=1,q=2", "AQIDBA")); err != nil {
	241	+	t.Fatal(err)
	242	+	}
	243	+	if _, err := session.Feed(apc("a=p,i=1,p=7,q=2", "")); err != nil {
	244	+	t.Fatal(err)
	245	+	}
	246	+	assetID, ok := session.Image(1)
	247	+	if !ok {
	248	+	t.Fatal("original image mapping missing")
	249	+	}
	250	+	placementID, ok := session.Placement(7)
	251	+	if !ok {
	252	+	t.Fatal("original placement mapping missing")
	253	+	}
	254	+	before := scene.Snapshot()
	255	+	result, err := session.Feed(apc("a=t,i=1,f=32,s=2,v=1,q=0", "AQIDBAUGBwg"))
	256	+	if !errors.Is(err, graphics.ErrEncodedBudget) {
	257	+	t.Fatalf("replacement error = %v", err)
	258	+	}
	259	+	if got := result.Bytes(); string(got) != "\x1b_Gi=1;E2BIG\x1b\\" {
	260	+	t.Fatalf("replacement response = %q", got)
	261	+	}
	262	+	if got, ok := session.Image(1); !ok \|\| got != assetID {
	263	+	t.Fatalf("replacement changed image mapping: id=%v ok=%v want=%v", got, ok, assetID)
	264	+	}
	265	+	if got, ok := session.Placement(7); !ok \|\| got != placementID {
	266	+	t.Fatalf("replacement changed placement mapping: id=%v ok=%v want=%v", got, ok, placementID)
	267	+	}
	268	+	if scene.Snapshot().Generation() != before.Generation() \|\| scene.Usage() != before.Usage() {
	269	+	t.Fatalf("failed replacement changed scene: before=%#v after=%#v", before.Usage(), scene.Usage())
	270	+	}
	271	+	if _, ok := scene.Snapshot().Placement(placementID); !ok {
	272	+	t.Fatal("failed replacement removed original placement")
	273	+	}
	274	+	}
	275	+
	276	+	func TestFailedDisplayedImageReplacementPreservesOldPlacement(t *testing.T) {
	277	+	scene := graphics.NewScene(graphics.Limits{})
	278	+	session := NewSession(scene)
	279	+	if _, err := session.Feed(apc("a=T,i=1,f=32,s=1,v=1,q=2", "AQIDBA")); err != nil {
	280	+	t.Fatal(err)
	281	+	}
	282	+	assetID, ok := session.Image(1)
	283	+	if !ok {
	284	+	t.Fatal("original image mapping missing")
	285	+	}
	286	+	placementID, ok := session.Placement(1)
	287	+	if !ok {
	288	+	t.Fatal("original placement mapping missing")
	289	+	}
	290	+
	291	+	result, err := session.Feed(apc("a=T,i=1,f=32,s=1,v=1,w=2,q=0", "AQIDBA"))
	292	+	if !errors.Is(err, graphics.ErrInvalidPlacement) {
	293	+	t.Fatalf("replacement placement error = %v", err)
	294	+	}
	295	+	if got := result.Bytes(); string(got) != "\x1b_Gi=1;EINVAL\x1b\\" {
	296	+	t.Fatalf("replacement response = %q", got)
	297	+	}
	298	+	if got, ok := session.Image(1); !ok \|\| got != assetID {
	299	+	t.Fatalf("failed displayed replacement changed image: id=%v ok=%v want=%v", got, ok, assetID)
	300	+	}
	301	+	if got, ok := session.Placement(1); !ok \|\| got != placementID {
	302	+	t.Fatalf("failed displayed replacement changed placement: id=%v ok=%v want=%v", got, ok, placementID)
	303	+	}
	304	+	if got := scene.Usage(); got.Assets != 1 \|\| got.Placements != 1 {
	305	+	t.Fatalf("failed displayed replacement usage = %#v", got)
	306	+	}
	307	+	}
	308	+
219	309		func TestDeleteSelectedImagePlacementsAndImageNumberNamespace(t *testing.T) {
220	310		scene := graphics.NewScene(graphics.Limits{})
221	311		session := NewSession(scene)


@@ -216,6 +216,96 @@ func TestChunkedTransmitAndDisplayRetainsDisplayAction(t *testing.T) {
216	}	216		}
217	}	217		}
218		218
		219	+	func TestPlacementFailureEmitsStableProtocolError(t *testing.T) {
		220	+	scene := graphics.NewScene(graphics.Limits{})
		221	+	session := NewSession(scene)
		222	+	if _, err := session.Feed(apc("a=t,i=1,f=32,s=1,v=1,q=2", "AQIDBA")); err != nil {
		223	+	t.Fatal(err)
		224	+	}
		225	+	result, err := session.Feed(apc("a=p,i=1,p=7,w=2,q=0", ""))
		226	+	if !errors.Is(err, graphics.ErrInvalidPlacement) {
		227	+	t.Fatalf("placement error = %v", err)
		228	+	}
		229	+	if got := result.Bytes(); string(got) != "\x1b_Gi=1;EINVAL\x1b\\" {
		230	+	t.Fatalf("placement response = %q", got)
		231	+	}
		232	+	if len(result.Mutations) != 0 \|\| scene.Usage().Placements != 0 {
		233	+	t.Fatalf("failed placement mutated state: result=%#v usage=%#v", result, scene.Usage())
		234	+	}
		235	+	}
		236	+
		237	+	func TestFailedImageReplacementPreservesAssetAndPlacements(t *testing.T) {
		238	+	scene := graphics.NewScene(graphics.Limits{MaxEncodedBytes: 4, MaxDecodedPixels: 4})
		239	+	session := NewSession(scene)
		240	+	if _, err := session.Feed(apc("a=t,i=1,f=32,s=1,v=1,q=2", "AQIDBA")); err != nil {
		241	+	t.Fatal(err)
		242	+	}
		243	+	if _, err := session.Feed(apc("a=p,i=1,p=7,q=2", "")); err != nil {
		244	+	t.Fatal(err)
		245	+	}
		246	+	assetID, ok := session.Image(1)
		247	+	if !ok {
		248	+	t.Fatal("original image mapping missing")
		249	+	}
		250	+	placementID, ok := session.Placement(7)
		251	+	if !ok {
		252	+	t.Fatal("original placement mapping missing")
		253	+	}
		254	+	before := scene.Snapshot()
		255	+	result, err := session.Feed(apc("a=t,i=1,f=32,s=2,v=1,q=0", "AQIDBAUGBwg"))
		256	+	if !errors.Is(err, graphics.ErrEncodedBudget) {
		257	+	t.Fatalf("replacement error = %v", err)
		258	+	}
		259	+	if got := result.Bytes(); string(got) != "\x1b_Gi=1;E2BIG\x1b\\" {
		260	+	t.Fatalf("replacement response = %q", got)
		261	+	}
		262	+	if got, ok := session.Image(1); !ok \|\| got != assetID {
		263	+	t.Fatalf("replacement changed image mapping: id=%v ok=%v want=%v", got, ok, assetID)
		264	+	}
		265	+	if got, ok := session.Placement(7); !ok \|\| got != placementID {
		266	+	t.Fatalf("replacement changed placement mapping: id=%v ok=%v want=%v", got, ok, placementID)
		267	+	}
		268	+	if scene.Snapshot().Generation() != before.Generation() \|\| scene.Usage() != before.Usage() {
		269	+	t.Fatalf("failed replacement changed scene: before=%#v after=%#v", before.Usage(), scene.Usage())
		270	+	}
		271	+	if _, ok := scene.Snapshot().Placement(placementID); !ok {
		272	+	t.Fatal("failed replacement removed original placement")
		273	+	}
		274	+	}
		275	+
		276	+	func TestFailedDisplayedImageReplacementPreservesOldPlacement(t *testing.T) {
		277	+	scene := graphics.NewScene(graphics.Limits{})
		278	+	session := NewSession(scene)
		279	+	if _, err := session.Feed(apc("a=T,i=1,f=32,s=1,v=1,q=2", "AQIDBA")); err != nil {
		280	+	t.Fatal(err)
		281	+	}
		282	+	assetID, ok := session.Image(1)
		283	+	if !ok {
		284	+	t.Fatal("original image mapping missing")
		285	+	}
		286	+	placementID, ok := session.Placement(1)
		287	+	if !ok {
		288	+	t.Fatal("original placement mapping missing")
		289	+	}
		290	+
		291	+	result, err := session.Feed(apc("a=T,i=1,f=32,s=1,v=1,w=2,q=0", "AQIDBA"))
		292	+	if !errors.Is(err, graphics.ErrInvalidPlacement) {
		293	+	t.Fatalf("replacement placement error = %v", err)
		294	+	}
		295	+	if got := result.Bytes(); string(got) != "\x1b_Gi=1;EINVAL\x1b\\" {
		296	+	t.Fatalf("replacement response = %q", got)
		297	+	}
		298	+	if got, ok := session.Image(1); !ok \|\| got != assetID {
		299	+	t.Fatalf("failed displayed replacement changed image: id=%v ok=%v want=%v", got, ok, assetID)
		300	+	}
		301	+	if got, ok := session.Placement(1); !ok \|\| got != placementID {
		302	+	t.Fatalf("failed displayed replacement changed placement: id=%v ok=%v want=%v", got, ok, placementID)
		303	+	}
		304	+	if got := scene.Usage(); got.Assets != 1 \|\| got.Placements != 1 {
		305	+	t.Fatalf("failed displayed replacement usage = %#v", got)
		306	+	}
		307	+	}
		308	+
219	func TestDeleteSelectedImagePlacementsAndImageNumberNamespace(t *testing.T) {	309		func TestDeleteSelectedImagePlacementsAndImageNumberNamespace(t *testing.T) {
220	scene := graphics.NewScene(graphics.Limits{})	310		scene := graphics.NewScene(graphics.Limits{})
221	session := NewSession(scene)	311		session := NewSession(scene)

M protocol/kittygraphics/session.go +113 -31


@@ -181,6 +181,18 @@ func (s *Session) SetPendingPlacement(x, y uint64) {
181	181		}
182	182		}
183	183
	184	+	// AbortPendingUpload discards an in-flight chunked upload without changing
	185	+	// committed images or placements. Screen-level full clears use it to ensure a
	186	+	// continuation arriving after the clear cannot publish stale graphics.
	187	+	func (s *Session) AbortPendingUpload() {
	188	+	if s == nil {
	189	+	return
	190	+	}
	191	+	s.mu.Lock()
	192	+	defer s.mu.Unlock()
	193	+	s.upload = nil
	194	+	}
	195	+
184	196		func (s Session) apply(command Command) ([][]byte, Mutation, error) {
185	197		c := command.Controls
186	198		action := c.Action

@@ -294,25 +306,40 @@ func (s Session) commitUpload(value upload, display bool) ([][]byte, Mutation,
294	306		if s.scene == nil {
295	307		return s.failure(value.controls, value.imageID, ErrNoScene)
296	308		}
297		-	if old, ok := s.images[value.imageID]; ok {
298		-	// Remove adapter references while the old scene snapshot still
299		-	// contains its placements, then remove the scene asset atomically.
300		-	s.removeMappingsForAsset(old)
301		-	if err := s.scene.RemoveAssetCascade(old); err != nil {
	309	+	old, replacing := s.images[value.imageID]
	310	+	if !replacing && uint64(len(s.images)) >= s.limits.MaxImages {
	311	+	return s.failure(value.controls, value.imageID, graphics.ErrTooManyAssets)
	312	+	}
	313	+	if replacing && display {
	314	+	// Validate the prospective placement before replacing the asset. A
	315	+	// rejected T command must leave both the old asset and its placements
	316	+	// available for subsequent use.
	317	+	if err := s.validateReplacementPlacement(value.controls, old, width, height); err != nil {
302	318		return s.failure(value.controls, value.imageID, err)
303	319		}
304	320		}
305		-	if uint64(len(s.images)) >= s.limits.MaxImages {
306		-	return s.failure(value.controls, value.imageID, graphics.ErrTooManyAssets)
	321	+	var assetID graphics.AssetID
	322	+	if replacing {
	323	+	// Scene replacement is copy-on-write: a rejected new asset leaves the
	324	+	// old asset and every placement that references it untouched.
	325	+	assetID, err = s.scene.ReplaceAsset(old, graphics.AssetBlob{
	326	+	Encoded: decoded,
	327	+	Width: width,
	328	+	Height: height,
	329	+	})
	330	+	} else {
	331	+	assetID, err = s.scene.AddAsset(graphics.AssetBlob{
	332	+	Encoded: decoded,
	333	+	Width: width,
	334	+	Height: height,
	335	+	})
307	336		}
308		-	assetID, err := s.scene.AddAsset(graphics.AssetBlob{
309		-	Encoded: decoded,
310		-	Width: width,
311		-	Height: height,
312		-	})
313	337		if err != nil {
314	338		return s.failure(value.controls, value.imageID, err)
315	339		}
	340	+	if replacing {
	341	+	s.removeMappingsForAsset(old)
	342	+	}
316	343		s.images[value.imageID] = assetID
317	344		if value.controls.HasImageNumber {
318	345		s.imageNumbers[value.controls.ImageNumber] = value.imageID

@@ -337,7 +364,8 @@ func (s Session) commitUpload(value upload, display bool) ([][]byte, Mutation,
337	364		delete(s.imageNumbers, value.controls.ImageNumber)
338	365		}
339	366		delete(s.children, childID)
340		-	return responses, nil, placeErr
	367	+	failureResponses, _, failureErr := s.failure(value.controls, value.imageID, placeErr)
	368	+	return failureResponses, nil, failureErr
341	369		}
342	370		return s.success(value.controls, value.imageID, responses...), mutation, nil
343	371		}

@@ -408,59 +436,113 @@ func (s Session) put(c Controls) ([][]byte, Mutation, error) {
408	436		}
409	437		responses, mutation, err := s.place(c, imageID, assetID)
410	438		if err != nil {
411		-	return responses, mutation, err
	439	+	failureResponses, _, failureErr := s.failure(c, imageID, err)
	440	+	return failureResponses, nil, failureErr
412	441		}
413	442		return s.success(c, imageID, responses...), mutation, nil
414	443		}
415	444
416		-	func (s Session) place(c Controls, imageID uint64, assetID graphics.AssetID) ([][]byte, Mutation, error) {
417		-	if s.scene == nil {
418		-	return nil, nil, ErrNoScene
	445	+	func (s *Session) validateReplacementPlacement(c Controls, old graphics.AssetID, width, height int64) error {
	446	+	if _, err := placementSpec(c, old, width, height); err != nil {
	447	+	return err
419	448		}
420		-	asset, ok := s.scene.Snapshot().Asset(assetID)
421		-	if !ok {
422		-	return nil, nil, ErrImageNotFound
	449	+	snapshot := s.scene.Snapshot()
	450	+	removedScene := uint64(0)
	451	+	for _, placement := range snapshot.Placements() {
	452	+	if placement.AssetID() == old {
	453	+	removedScene++
	454	+	}
	455	+	}
	456	+	removedSession := uint64(0)
	457	+	for _, mapped := range s.placements {
	458	+	placement, ok := snapshot.Placement(mapped)
	459	+	if !ok \|\| placement.AssetID() == old {
	460	+	removedSession++
	461	+	}
	462	+	}
	463	+	projectedSession := uint64(len(s.placements)) - removedSession
	464	+	projectedScene := snapshot.Usage().Placements - removedScene
	465	+	needsSlot := true
	466	+	if c.HasPlacementID {
	467	+	if mapped, ok := s.placements[c.PlacementID]; ok {
	468	+	if placement, ok := snapshot.Placement(mapped); ok && placement.AssetID() != old {
	469	+	needsSlot = false
	470	+	}
	471	+	}
423	472		}
424		-	width, height := asset.Width(), asset.Height()
	473	+	if needsSlot && projectedSession >= s.limits.MaxPlacements {
	474	+	return graphics.ErrTooManyPlacements
	475	+	}
	476	+	if needsSlot && projectedScene >= s.scene.Limits().MaxPlacements {
	477	+	return graphics.ErrTooManyPlacements
	478	+	}
	479	+	if s.scene.Generation() >= ^uint64(0)-1 {
	480	+	return graphics.ErrGenerationOverflow
	481	+	}
	482	+	return nil
	483	+	}
	484	+
	485	+	func placementSpec(c Controls, assetID graphics.AssetID, width, height int64) (graphics.PlacementSpec, error) {
425	486		source := graphics.PixelRect{Width: width, Height: height}
426	487		var err error
427	488		if source.X, err = checkedCoordinate(c.SourceX, c.HasSourceX); err != nil {
428		-	return nil, nil, err
	489	+	return graphics.PlacementSpec{}, err
429	490		}
430	491		if source.Y, err = checkedCoordinate(c.SourceY, c.HasSourceY); err != nil {
431		-	return nil, nil, err
	492	+	return graphics.PlacementSpec{}, err
432	493		}
433	494		if source.Width, err = checkedDimension(c.SourceWidth, c.HasSourceWidth, width); err != nil {
434		-	return nil, nil, err
	495	+	return graphics.PlacementSpec{}, err
435	496		}
436	497		if source.Height, err = checkedDimension(c.SourceHeight, c.HasSourceHeight, height); err != nil {
437		-	return nil, nil, err
	498	+	return graphics.PlacementSpec{}, err
438	499		}
439	500		x, err := checkedCoordinate(c.X, c.HasX)
440	501		if err != nil {
441		-	return nil, nil, err
	502	+	return graphics.PlacementSpec{}, err
442	503		}
443	504		y, err := checkedCoordinate(c.Y, c.HasY)
444	505		if err != nil {
445		-	return nil, nil, err
	506	+	return graphics.PlacementSpec{}, err
446	507		}
447	508		destination := graphics.PixelRect{X: x, Y: y, Width: source.Width, Height: source.Height}
448	509		if !source.Valid() \|\| !destination.Valid() {
449		-	return nil, nil, graphics.ErrInvalidRect
	510	+	return graphics.PlacementSpec{}, graphics.ErrInvalidRect
	511	+	}
	512	+	assetBounds := graphics.PixelRect{Width: width, Height: height}
	513	+	clipped, ok := assetBounds.Intersect(source)
	514	+	if !ok \|\| clipped != source {
	515	+	return graphics.PlacementSpec{}, graphics.ErrInvalidPlacement
450	516		}
451	517		var cells graphics.CellRect
452	518		if c.HasColumns \|\| c.HasRows {
453	519		if !c.HasColumns \|\| !c.HasRows \|\| c.Columns == 0 \|\| c.Rows == 0 \|\| c.Columns > math.MaxInt64 \|\| c.Rows > math.MaxInt64 {
454		-	return nil, nil, fmt.Errorf("%w: c/r", ErrInvalidCommand)
	520	+	return graphics.PlacementSpec{}, fmt.Errorf("%w: c/r", ErrInvalidCommand)
455	521		}
456	522		cells = graphics.CellRect{X: signedCoordinateInt(c.CellOffsetX, c.HasCellOffsetX), Y: signedCoordinateInt(c.CellOffsetY, c.HasCellOffsetY), Width: int64(c.Columns), Height: int64(c.Rows)}
457	523		if !cells.Valid() {
458		-	return nil, nil, graphics.ErrInvalidRect
	524	+	return graphics.PlacementSpec{}, graphics.ErrInvalidRect
459	525		}
460	526		}
461		-	spec := graphics.PlacementSpec{Asset: assetID, Source: source, Destination: destination, Cells: cells}
	527	+	spec := graphics.PlacementSpec{Asset: assetID, Source: source, Destination: destination, Cells: cells, HasCells: c.HasColumns \|\| c.HasRows}
462	528		if c.HasLayer {
463	529		spec.Layer = c.Layer
	530	+	spec.HasLayer = true
	531	+	}
	532	+	return spec, nil
	533	+	}
	534	+
	535	+	func (s Session) place(c Controls, imageID uint64, assetID graphics.AssetID) ([][]byte, Mutation, error) {
	536	+	if s.scene == nil {
	537	+	return nil, nil, ErrNoScene
	538	+	}
	539	+	asset, ok := s.scene.Snapshot().Asset(assetID)
	540	+	if !ok {
	541	+	return nil, nil, ErrImageNotFound
	542	+	}
	543	+	spec, err := placementSpec(c, assetID, asset.Width(), asset.Height())
	544	+	if err != nil {
	545	+	return nil, nil, err
464	546		}
465	547		placementID := c.PlacementID
466	548		if placementID == 0 {

 	}
 }
+// AbortPendingUpload discards an in-flight chunked upload without changing
+// committed images or placements. Screen-level full clears use it to ensure a
+// continuation arriving after the clear cannot publish stale graphics.
+func (s *Session) AbortPendingUpload() {
+	if s == nil {
+		return
+	}
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	s.upload = nil
+}
 func (s *Session) apply(command Command) ([][]byte, *Mutation, error) {
 	c := command.Controls
 	action := c.Action
 	if s.scene == nil {
 		return s.failure(value.controls, value.imageID, ErrNoScene)
 	}
-	if old, ok := s.images[value.imageID]; ok {
+	old, replacing := s.images[value.imageID]
-		// Remove adapter references while the old scene snapshot still
+	if !replacing && uint64(len(s.images)) >= s.limits.MaxImages {
-		// contains its placements, then remove the scene asset atomically.
+		return s.failure(value.controls, value.imageID, graphics.ErrTooManyAssets)
-		s.removeMappingsForAsset(old)
+	}
-		if err := s.scene.RemoveAssetCascade(old); err != nil {
+	if replacing && display {
+		// Validate the prospective placement before replacing the asset. A
+		// rejected T command must leave both the old asset and its placements
+		// available for subsequent use.
+		if err := s.validateReplacementPlacement(value.controls, old, width, height); err != nil {
 			return s.failure(value.controls, value.imageID, err)
 		}
 	}
-	if uint64(len(s.images)) >= s.limits.MaxImages {
+	var assetID graphics.AssetID
-		return s.failure(value.controls, value.imageID, graphics.ErrTooManyAssets)
+	if replacing {
+		// Scene replacement is copy-on-write: a rejected new asset leaves the
+		// old asset and every placement that references it untouched.
+		assetID, err = s.scene.ReplaceAsset(old, graphics.AssetBlob{
+			Encoded: decoded,
+			Width:   width,
+			Height:  height,
+		})
+	} else {
+		assetID, err = s.scene.AddAsset(graphics.AssetBlob{
+			Encoded: decoded,
+			Width:   width,
+			Height:  height,
+		})
 	}
-	assetID, err := s.scene.AddAsset(graphics.AssetBlob{
-		Encoded: decoded,
-		Width:   width,
-		Height:  height,
-	})
 	if err != nil {
 		return s.failure(value.controls, value.imageID, err)
 	}
+	if replacing {
+		s.removeMappingsForAsset(old)
+	}
 	s.images[value.imageID] = assetID
 	if value.controls.HasImageNumber {
 		s.imageNumbers[value.controls.ImageNumber] = value.imageID
 				delete(s.imageNumbers, value.controls.ImageNumber)
 			}
 			delete(s.children, childID)
-			return responses, nil, placeErr
+			failureResponses, _, failureErr := s.failure(value.controls, value.imageID, placeErr)
+			return failureResponses, nil, failureErr
 		}
 		return s.success(value.controls, value.imageID, responses...), mutation, nil
 	}
 	}
 	responses, mutation, err := s.place(c, imageID, assetID)
 	if err != nil {
-		return responses, mutation, err
+		failureResponses, _, failureErr := s.failure(c, imageID, err)
+		return failureResponses, nil, failureErr
 	}
 	return s.success(c, imageID, responses...), mutation, nil
 }
-func (s *Session) place(c Controls, imageID uint64, assetID graphics.AssetID) ([][]byte, *Mutation, error) {
+func (s *Session) validateReplacementPlacement(c Controls, old graphics.AssetID, width, height int64) error {
-	if s.scene == nil {
+	if _, err := placementSpec(c, old, width, height); err != nil {
-		return nil, nil, ErrNoScene
+		return err
 	}
-	asset, ok := s.scene.Snapshot().Asset(assetID)
+	snapshot := s.scene.Snapshot()
-	if !ok {
+	removedScene := uint64(0)
-		return nil, nil, ErrImageNotFound
+	for _, placement := range snapshot.Placements() {
+		if placement.AssetID() == old {
+			removedScene++
+		}
+	}
+	removedSession := uint64(0)
+	for _, mapped := range s.placements {
+		placement, ok := snapshot.Placement(mapped)
+		if !ok || placement.AssetID() == old {
+			removedSession++
+		}
+	}
+	projectedSession := uint64(len(s.placements)) - removedSession
+	projectedScene := snapshot.Usage().Placements - removedScene
+	needsSlot := true
+	if c.HasPlacementID {
+		if mapped, ok := s.placements[c.PlacementID]; ok {
+			if placement, ok := snapshot.Placement(mapped); ok && placement.AssetID() != old {
+				needsSlot = false
+			}
+		}
 	}
-	width, height := asset.Width(), asset.Height()
+	if needsSlot && projectedSession >= s.limits.MaxPlacements {
+		return graphics.ErrTooManyPlacements
+	}
+	if needsSlot && projectedScene >= s.scene.Limits().MaxPlacements {
+		return graphics.ErrTooManyPlacements
+	}
+	if s.scene.Generation() >= ^uint64(0)-1 {
+		return graphics.ErrGenerationOverflow
+	}
+	return nil
+}
+func placementSpec(c Controls, assetID graphics.AssetID, width, height int64) (graphics.PlacementSpec, error) {
 	source := graphics.PixelRect{Width: width, Height: height}
 	var err error
 	if source.X, err = checkedCoordinate(c.SourceX, c.HasSourceX); err != nil {
-		return nil, nil, err
+		return graphics.PlacementSpec{}, err
 	}
 	if source.Y, err = checkedCoordinate(c.SourceY, c.HasSourceY); err != nil {
-		return nil, nil, err
+		return graphics.PlacementSpec{}, err
 	}
 	if source.Width, err = checkedDimension(c.SourceWidth, c.HasSourceWidth, width); err != nil {
-		return nil, nil, err
+		return graphics.PlacementSpec{}, err
 	}
 	if source.Height, err = checkedDimension(c.SourceHeight, c.HasSourceHeight, height); err != nil {
-		return nil, nil, err
+		return graphics.PlacementSpec{}, err
 	}
 	x, err := checkedCoordinate(c.X, c.HasX)
 	if err != nil {
-		return nil, nil, err
+		return graphics.PlacementSpec{}, err
 	}
 	y, err := checkedCoordinate(c.Y, c.HasY)
 	if err != nil {
-		return nil, nil, err
+		return graphics.PlacementSpec{}, err
 	}
 	destination := graphics.PixelRect{X: x, Y: y, Width: source.Width, Height: source.Height}
 	if !source.Valid() || !destination.Valid() {
-		return nil, nil, graphics.ErrInvalidRect
+		return graphics.PlacementSpec{}, graphics.ErrInvalidRect
+	}
+	assetBounds := graphics.PixelRect{Width: width, Height: height}
+	clipped, ok := assetBounds.Intersect(source)
+	if !ok || clipped != source {
+		return graphics.PlacementSpec{}, graphics.ErrInvalidPlacement
 	}
 	var cells graphics.CellRect
 	if c.HasColumns || c.HasRows {
 		if !c.HasColumns || !c.HasRows || c.Columns == 0 || c.Rows == 0 || c.Columns > math.MaxInt64 || c.Rows > math.MaxInt64 {
-			return nil, nil, fmt.Errorf("%w: c/r", ErrInvalidCommand)
+			return graphics.PlacementSpec{}, fmt.Errorf("%w: c/r", ErrInvalidCommand)
 		}
 		cells = graphics.CellRect{X: signedCoordinateInt(c.CellOffsetX, c.HasCellOffsetX), Y: signedCoordinateInt(c.CellOffsetY, c.HasCellOffsetY), Width: int64(c.Columns), Height: int64(c.Rows)}
 		if !cells.Valid() {
-			return nil, nil, graphics.ErrInvalidRect
+			return graphics.PlacementSpec{}, graphics.ErrInvalidRect
 		}
 	}
-	spec := graphics.PlacementSpec{Asset: assetID, Source: source, Destination: destination, Cells: cells}
+	spec := graphics.PlacementSpec{Asset: assetID, Source: source, Destination: destination, Cells: cells, HasCells: c.HasColumns || c.HasRows}
 	if c.HasLayer {
 		spec.Layer = c.Layer
+		spec.HasLayer = true
+	}
+	return spec, nil
+}
+func (s *Session) place(c Controls, imageID uint64, assetID graphics.AssetID) ([][]byte, *Mutation, error) {
+	if s.scene == nil {
+		return nil, nil, ErrNoScene
+	}
+	asset, ok := s.scene.Snapshot().Asset(assetID)
+	if !ok {
+		return nil, nil, ErrImageNotFound
+	}
+	spec, err := placementSpec(c, assetID, asset.Width(), asset.Height())
+	if err != nil {
+		return nil, nil, err
 	}
 	placementID := c.PlacementID
 	if placementID == 0 {

M screen_graphics_test.go +29 -0


@@ -59,6 +59,35 @@ func TestScreenKittyGraphicsPrimaryAndAlternateState(t *testing.T) {
59	59		require.Equal(t, primary.Usage(), screen.GraphicsSnapshot().Usage())
60	60		}
61	61
	62	+	func TestScreenReenteringActiveAlternatePreservesGraphicsState(t *testing.T) {
	63	+	screen := NewScreen(8, 3)
	64	+	screen.Write([]byte("\x1b[?1049h"))
	65	+	screen.Write(screenKittyImageAPC("T"))
	66	+	before := screen.GraphicsSnapshot()
	67	+	require.NotNil(t, before)
	68	+	require.Equal(t, uint64(1), before.Usage().Placements)
	69	+
	70	+	screen.Write([]byte("\x1b[?1049h"))
	71	+	after := screen.GraphicsSnapshot()
	72	+	require.NotNil(t, after)
	73	+	require.Equal(t, before.Usage(), after.Usage())
	74	+	}
	75	+
	76	+	func TestScreenFullClearAbortsPendingUploadAndClearsPlacements(t *testing.T) {
	77	+	screen := NewScreen(8, 3)
	78	+	screen.Write(screenKittyImageAPC("T"))
	79	+	screen.Write([]byte("\x1b_Ga=T,i=2,f=32,s=1,v=1,m=1,C=1;AQ\x1b\\"))
	80	+	require.Equal(t, uint64(1), screen.GraphicsSnapshot().Usage().Placements)
	81	+
	82	+	screen.Write([]byte("\x1b[2J"))
	83	+	require.Equal(t, uint64(0), screen.GraphicsSnapshot().Usage().Placements)
	84	+	screen.Write([]byte("\x1b_Gm=0;IDBA\x1b\\"))
	85	+	graphicsSnapshot := screen.GraphicsSnapshot()
	86	+	require.NotNil(t, graphicsSnapshot)
	87	+	require.Equal(t, uint64(1), graphicsSnapshot.Usage().Assets)
	88	+	require.Equal(t, uint64(0), graphicsSnapshot.Usage().Placements)
	89	+	}
	90	+
62	91		func TestScreenKittyGraphicsOrdinaryResetClearScrollAndResize(t *testing.T) {
63	92		screen := NewScreen(3, 2)
64	93		screen.Write(screenKittyImageAPC("T"))


@@ -59,6 +59,35 @@ func TestScreenKittyGraphicsPrimaryAndAlternateState(t *testing.T) {
59	require.Equal(t, primary.Usage(), screen.GraphicsSnapshot().Usage())	59		require.Equal(t, primary.Usage(), screen.GraphicsSnapshot().Usage())
60	}	60		}
61		61
		62	+	func TestScreenReenteringActiveAlternatePreservesGraphicsState(t *testing.T) {
		63	+	screen := NewScreen(8, 3)
		64	+	screen.Write([]byte("\x1b[?1049h"))
		65	+	screen.Write(screenKittyImageAPC("T"))
		66	+	before := screen.GraphicsSnapshot()
		67	+	require.NotNil(t, before)
		68	+	require.Equal(t, uint64(1), before.Usage().Placements)
		69	+
		70	+	screen.Write([]byte("\x1b[?1049h"))
		71	+	after := screen.GraphicsSnapshot()
		72	+	require.NotNil(t, after)
		73	+	require.Equal(t, before.Usage(), after.Usage())
		74	+	}
		75	+
		76	+	func TestScreenFullClearAbortsPendingUploadAndClearsPlacements(t *testing.T) {
		77	+	screen := NewScreen(8, 3)
		78	+	screen.Write(screenKittyImageAPC("T"))
		79	+	screen.Write([]byte("\x1b_Ga=T,i=2,f=32,s=1,v=1,m=1,C=1;AQ\x1b\\"))
		80	+	require.Equal(t, uint64(1), screen.GraphicsSnapshot().Usage().Placements)
		81	+
		82	+	screen.Write([]byte("\x1b[2J"))
		83	+	require.Equal(t, uint64(0), screen.GraphicsSnapshot().Usage().Placements)
		84	+	screen.Write([]byte("\x1b_Gm=0;IDBA\x1b\\"))
		85	+	graphicsSnapshot := screen.GraphicsSnapshot()
		86	+	require.NotNil(t, graphicsSnapshot)
		87	+	require.Equal(t, uint64(1), graphicsSnapshot.Usage().Assets)
		88	+	require.Equal(t, uint64(0), graphicsSnapshot.Usage().Placements)
		89	+	}
		90	+
62	func TestScreenKittyGraphicsOrdinaryResetClearScrollAndResize(t *testing.T) {	91		func TestScreenKittyGraphicsOrdinaryResetClearScrollAndResize(t *testing.T) {
63	screen := NewScreen(3, 2)	92		screen := NewScreen(3, 2)
64	screen.Write(screenKittyImageAPC("T"))	93		screen.Write(screenKittyImageAPC("T"))

M screen_modes.go +15 -14


@@ -194,20 +194,21 @@ func (s *Screen) setMode(private bool, parts []int, enabled bool) {
194	194		}
195	195
196	196		func (s *Screen) enterAlternateScreen() {
197		-	if s.alternate == nil {
198		-	s.alternate = &screenState{
199		-	frame: cloneFrame(s.Frame),
200		-	buffer: s.buffer.clone(),
201		-	row: s.Row,
202		-	col: s.Col,
203		-	style: s.Style,
204		-	scrollTop: s.scrollTop,
205		-	scrollBottom: s.scrollBottom,
206		-	savedCursor: s.savedCursor,
207		-	graphics: s.graphics,
208		-	originMode: s.originMode,
209		-	insertMode: s.insertMode,
210		-	}
	197	+	if s.alternate != nil {
	198	+	return
	199	+	}
	200	+	s.alternate = &screenState{
	201	+	frame: cloneFrame(s.Frame),
	202	+	buffer: s.buffer.clone(),
	203	+	row: s.Row,
	204	+	col: s.Col,
	205	+	style: s.Style,
	206	+	scrollTop: s.scrollTop,
	207	+	scrollBottom: s.scrollBottom,
	208	+	savedCursor: s.savedCursor,
	209	+	graphics: s.graphics,
	210	+	originMode: s.originMode,
	211	+	insertMode: s.insertMode,
211	212		}
212	213		s.graphics = nil
213	214		s.buffer = s.newBuffer(s.Frame.Width, s.Frame.Height)


@@ -194,20 +194,21 @@ func (s *Screen) setMode(private bool, parts []int, enabled bool) {
194		}	194		}
195			195
196		func (s *Screen) enterAlternateScreen() {	196		func (s *Screen) enterAlternateScreen() {
197	-	if s.alternate == nil {	197	+	if s.alternate != nil {
198	-	s.alternate = &screenState{	198	+	return
199	-	frame: cloneFrame(s.Frame),	199	+	}
200	-	buffer: s.buffer.clone(),	200	+	s.alternate = &screenState{
201	-	row: s.Row,	201	+	frame: cloneFrame(s.Frame),
202	-	col: s.Col,	202	+	buffer: s.buffer.clone(),
203	-	style: s.Style,	203	+	row: s.Row,
204	-	scrollTop: s.scrollTop,	204	+	col: s.Col,
205	-	scrollBottom: s.scrollBottom,	205	+	style: s.Style,
206	-	savedCursor: s.savedCursor,	206	+	scrollTop: s.scrollTop,
207	-	graphics: s.graphics,	207	+	scrollBottom: s.scrollBottom,
208	-	originMode: s.originMode,	208	+	savedCursor: s.savedCursor,
209	-	insertMode: s.insertMode,	209	+	graphics: s.graphics,
210	-	}	210	+	originMode: s.originMode,
			211	+	insertMode: s.insertMode,
211		}	212		}
212		s.graphics = nil	213		s.graphics = nil
213		s.buffer = s.newBuffer(s.Frame.Width, s.Frame.Height)	214		s.buffer = s.newBuffer(s.Frame.Width, s.Frame.Height)