From eabfcd20d4cb1a7ea5258ce89ad836056a434273 Mon Sep 17 00:00:00 2001 From: Kostya Farber Date: Sat, 6 Jun 2026 16:22:51 +0300 Subject: [PATCH] Refine workspace layer edit boundary --- .../src/renderer/src/lib/model/Font.ts | 16 +- crates/shift-bridge/__test__/index.spec.mjs | 41 +- crates/shift-bridge/dts-header.d.ts | 1 + crates/shift-bridge/index.d.ts | 3 + crates/shift-bridge/src/bridge.rs | 543 ++++++++---------- crates/shift-bridge/src/input.rs | 8 +- crates/shift-font/src/changes.rs | 89 +++ crates/shift-font/src/layer_edit.rs | 48 +- crates/shift-workspace/src/lib.rs | 2 +- crates/shift-workspace/src/workspace.rs | 443 +++++++++++--- .../shift-workspace/tests/workspace_test.rs | 38 ++ packages/types/src/bridge/generated.ts | 4 + packages/types/src/ids.ts | 23 + packages/types/src/index.ts | 3 + scripts/generate-bridge-types.mjs | 2 + 15 files changed, 816 insertions(+), 448 deletions(-) diff --git a/apps/desktop/src/renderer/src/lib/model/Font.ts b/apps/desktop/src/renderer/src/lib/model/Font.ts index 83ce87da..e2255c80 100644 --- a/apps/desktop/src/renderer/src/lib/model/Font.ts +++ b/apps/desktop/src/renderer/src/lib/model/Font.ts @@ -445,9 +445,9 @@ export class Font { * * @remarks * If the requested name already exists, a numeric suffix is appended using - * {@link nextAvailableGlyphName}. The bridge receives an explicit glyph layer - * mutation so downstream save/export paths see a real committed glyph record, - * not a UI-only placeholder. + * {@link nextAvailableGlyphName}. Glyph identity and its default-source layer + * are created as explicit bridge operations before this model rehydrates from + * the native font state. * * @param name - Preferred glyph name. Blank input falls back to `newGlyph`. * @returns The handle for the glyph that was actually created. @@ -457,13 +457,9 @@ export class Font { const glyphName = this.nextAvailableGlyphName(name); const handle = this.glyphHandleForName(glyphName); - this.#bridge.setXAdvance( - { - glyphHandle: handle, - sourceId: this.defaultSource.id, - }, - 500, - ); + const unicodes = handle.unicode === undefined ? [] : [handle.unicode]; + const glyphId = this.#bridge.createGlyph(glyphName, unicodes); + this.#bridge.createGlyphLayer(glyphId, this.defaultSource.id); this.#glyphs.clear(); this.#glyphSources.clear(); diff --git a/crates/shift-bridge/__test__/index.spec.mjs b/crates/shift-bridge/__test__/index.spec.mjs index a97c8d55..cffaa8f9 100644 --- a/crates/shift-bridge/__test__/index.spec.mjs +++ b/crates/shift-bridge/__test__/index.spec.mjs @@ -25,13 +25,25 @@ describe("Bridge", () => { return bridge.getSources()[0].id; } - function defaultLayerRef(name = "A", unicode = 65) { + function defaultUnicode(name, unicode) { + return unicode ?? (name === "A" ? 65 : undefined); + } + + function defaultLayerRef(name = "A", unicode) { return { - glyphHandle: { name, unicode }, + glyphHandle: { name, unicode: defaultUnicode(name, unicode) }, sourceId: defaultSourceId(), }; } + function createDefaultLayer(name = "A", unicode) { + const resolvedUnicode = defaultUnicode(name, unicode); + const unicodes = resolvedUnicode === undefined ? [] : [resolvedUnicode]; + const glyphId = bridge.createGlyph(name, unicodes); + bridge.createGlyphLayer(glyphId, defaultSourceId()); + return defaultLayerRef(name, resolvedUnicode); + } + it("creates a workspace with default committed font metadata", () => { expect(bridge.getMetadata()).toMatchObject({ familyName: "Untitled Font", @@ -53,7 +65,7 @@ describe("Bridge", () => { }); it("creates a new glyph through an explicit layer edit", () => { - bridge.setXAdvance(defaultLayerRef(), 500); + bridge.setXAdvance(createDefaultLayer(), 500); expect(bridge.getGlyphs()).toEqual([ { name: "A", unicodes: [65], componentBaseGlyphNames: [] }, @@ -61,31 +73,32 @@ describe("Bridge", () => { }); it("saves direct glyph layer edits to a shift source package target", () => { - const glyphRef = defaultLayerRef(); + const glyphRef = createDefaultLayer(); const contourId = bridge.addContour(glyphRef).changed.contourIds[0]; bridge.addPoint(glyphRef, contourId, 10, 20, "onCurve", false); const outputPath = join(tempDir, "output.shift"); const savedVersion = bridge.saveWorkspaceAs(outputPath); - expect(savedVersion).toBe(2); - expect(bridge.getPersistedVersion()).toBe(2); + expect(savedVersion).toBe(4); + expect(bridge.getPersistedVersion()).toBe(4); expect(bridge.isDirty()).toBe(false); expect(existsSync(outputPath)).toBe(true); }); it("records the persisted version when saving the current workspace", () => { - bridge.addContour(defaultLayerRef()); + bridge.addContour(createDefaultLayer()); const savedVersion = bridge.saveWorkspace(); - expect(savedVersion).toBe(1); - expect(bridge.getPersistedVersion()).toBe(1); + expect(savedVersion).toBe(3); + expect(bridge.getPersistedVersion()).toBe(3); expect(bridge.isDirty()).toBe(false); }); it("exports the live workspace font through an explicit export path", async () => { - const glyphRef = defaultLayerRef(); + createDefaultLayer(".notdef", undefined); + const glyphRef = createDefaultLayer(); const contourId = bridge.addContour(glyphRef).changed.contourIds[0]; bridge.addPoint(glyphRef, contourId, 10, 20, "onCurve", false); @@ -97,7 +110,7 @@ describe("Bridge", () => { }); it("adds a point to a contour and returns structure, values, and changed ids", () => { - const glyphRef = defaultLayerRef(); + const glyphRef = createDefaultLayer(); const contourChange = bridge.addContour(glyphRef); const contourId = contourChange.changed.contourIds[0]; @@ -120,7 +133,7 @@ describe("Bridge", () => { }); it("applies point positions through the sparse typed-array hot path", () => { - const glyphRef = defaultLayerRef(); + const glyphRef = createDefaultLayer(); const contourId = bridge.addContour(glyphRef).changed.contourIds[0]; const pointId = bridge.addPoint(glyphRef, contourId, 10, 20, "onCurve", false).changed .pointIds[0]; @@ -141,7 +154,7 @@ describe("Bridge", () => { }); it("restores structure and values into a glyph layer", () => { - const glyphRef = defaultLayerRef(); + const glyphRef = createDefaultLayer(); const contourId = bridge.addContour(glyphRef).changed.contourIds[0]; const before = bridge.addPoint(glyphRef, contourId, 10, 20, "onCurve", false); const pointId = before.changed.pointIds[0]; @@ -161,7 +174,7 @@ describe("Bridge", () => { bridge.addContour({ glyphHandle: { name: "A", unicode: 65 }, sourceId: "not-a-source" }), ).toThrow(/source ID/i); - const glyphRef = defaultLayerRef(); + const glyphRef = createDefaultLayer(); expect(() => bridge.addPoint(glyphRef, "not-a-contour", 10, 20, "onCurve", false), ).toThrow(/contour ID/i); diff --git a/crates/shift-bridge/dts-header.d.ts b/crates/shift-bridge/dts-header.d.ts index 31f3c9e2..e6ee721b 100644 --- a/crates/shift-bridge/dts-header.d.ts +++ b/crates/shift-bridge/dts-header.d.ts @@ -4,6 +4,7 @@ import type { AnchorId, ComponentId, GuidelineId, + GlyphId, GlyphName, LayerId, SourceId, diff --git a/crates/shift-bridge/index.d.ts b/crates/shift-bridge/index.d.ts index 4ecbe54c..57919d48 100644 --- a/crates/shift-bridge/index.d.ts +++ b/crates/shift-bridge/index.d.ts @@ -4,6 +4,7 @@ import type { AnchorId, ComponentId, GuidelineId, + GlyphId, GlyphName, LayerId, SourceId, @@ -29,6 +30,8 @@ export declare class Bridge { getSources(): Array getPersistedVersion(): number isDirty(): boolean + createGlyph(name: GlyphName, unicodes: Array): GlyphId + createGlyphLayer(glyphId: GlyphId, sourceId: SourceId): LayerId setXAdvance(glyphRef: GlyphLayerRef, width: number): NapiGlyphValueChange translateLayer(glyphRef: GlyphLayerRef, dx: number, dy: number): NapiGlyphValueChange addPoint(glyphRef: GlyphLayerRef, contourId: ContourId, x: number, y: number, pointType: NapiPointType, smooth: boolean): NapiGlyphStructureChange diff --git a/crates/shift-bridge/src/bridge.rs b/crates/shift-bridge/src/bridge.rs index 48e0481a..58e4da33 100644 --- a/crates/shift-bridge/src/bridge.rs +++ b/crates/shift-bridge/src/bridge.rs @@ -6,8 +6,8 @@ use napi_derive::napi; use serde::{Deserialize, Serialize}; use shift_backends::{ExportFormat, FontExportRequest, FontExportResult, FontExporter, FontView}; use shift_font::{ - BooleanOp, BulkNodePositionUpdates, ContourId, Font, Glyph, GlyphLayer, LayerId, PointId, - SourceId, + BooleanOp, BulkNodePositionUpdates, ContourId, Font, Glyph, GlyphId, GlyphLayer, LayerId, + PointId, SourceId, }; use shift_wire::{ bridges::napi::{ @@ -19,7 +19,7 @@ use shift_wire::{ Axis, FontMetadata, FontMetrics, GlyphChangedEntities, GlyphRecord, GlyphState, GlyphStructure, GlyphStructureChange, GlyphValueChange, Source, }; -use shift_workspace::{FontWorkspace, GlyphLayerTarget, NewWorkspace}; +use shift_workspace::{FontWorkspace, NewWorkspace}; use std::sync::{ atomic::{AtomicU64, Ordering}, Arc, @@ -475,68 +475,22 @@ impl Bridge { Ok(self.font()?.glyph_by_name(glyph_name).cloned()) } - fn glyph_layer_target(&self, glyph_ref: GlyphLayerRef) -> BridgeResult { + fn existing_layer_id(&self, glyph_ref: GlyphLayerRef) -> BridgeResult { let source_id = parse::(&glyph_ref.source_id)?; - if !self - .font()? - .sources() - .iter() - .any(|source| source.id() == source_id) - { - return Err(BridgeError::InvalidInput { - kind: "source ID", - value: source_id.to_string(), - }); - } - let layer_id = self .font()? .glyph_by_name(&glyph_ref.glyph_handle.name) .and_then(|glyph| glyph.layer_for_source(source_id)) .map(GlyphLayer::id) - .unwrap_or_else(LayerId::new); - - Ok(GlyphLayerTarget { - glyph_name: glyph_ref.glyph_handle.name, - unicode: glyph_ref.glyph_handle.unicode, - source_id, - layer_id, - }) - } - - fn edit_glyph_layer( - &mut self, - glyph_ref: GlyphLayerRef, - edit: impl FnOnce(&mut GlyphLayer) -> std::result::Result, - changes: impl FnOnce( - &shift_font::GlyphLayerChangeTarget, - &GlyphLayer, - &R, - ) -> shift_font::FontChangeSet, - ) -> BridgeResult { - let target = self.glyph_layer_target(glyph_ref)?; - Ok( - self - .workspace_mut()? - .edit_glyph_layer(target, edit, changes)?, - ) - } - - fn one_change(change: shift_font::FontChange) -> shift_font::FontChangeSet { - shift_font::FontChangeSet::new(vec![change]) - } + .ok_or_else(|| BridgeError::InvalidInput { + kind: "glyph layer", + value: format!( + "{} at source {}", + glyph_ref.glyph_handle.name, glyph_ref.source_id + ), + })?; - fn layer_replaced_change( - target: &shift_font::GlyphLayerChangeTarget, - layer: &GlyphLayer, - _result: &impl Sized, - ) -> shift_font::FontChangeSet { - Self::one_change(shift_font::FontChange::LayerGeometryReplaced( - shift_font::LayerGeometryReplaced { - target: target.clone(), - layer: shift_font::GlyphLayerValue::from(layer), - }, - )) + Ok(layer_id) } fn variation_build_for_glyph( @@ -693,24 +647,43 @@ impl Bridge { self.persisted_version() < self.live_version() } + #[napi(ts_return_type = "GlyphId")] + pub fn create_glyph( + &mut self, + #[napi(ts_arg_type = "GlyphName")] name: String, + #[napi(ts_arg_type = "Array")] unicodes: Vec, + ) -> errors::Result { + let glyph = self.workspace_mut()?.create_glyph(name, unicodes)?; + + self.mark_font_changed(); + Ok(glyph.id().to_string()) + } + + #[napi(ts_return_type = "LayerId")] + pub fn create_glyph_layer( + &mut self, + #[napi(ts_arg_type = "GlyphId")] glyph_id: String, + #[napi(ts_arg_type = "SourceId")] source_id: String, + ) -> errors::Result { + let glyph_id = parse::(&glyph_id)?; + let source_id = parse::(&source_id)?; + let layer = self + .workspace_mut()? + .create_glyph_layer(glyph_id, source_id)?; + + self.mark_font_changed(); + Ok(layer.id().to_string()) + } + #[napi] pub fn set_x_advance( &mut self, glyph_ref: GlyphLayerRef, width: f64, ) -> errors::Result { - let change = self.edit_glyph_layer( - glyph_ref, - |layer| { - layer.set_x_advance(width); - Ok(GlyphValueChange::from_layer(layer, Default::default())) - }, - |target, layer, _change| { - Self::one_change(shift_font::FontChange::LayerMetricsChanged( - shift_font::LayerMetricsChanged::from_layer(target.clone(), layer), - )) - }, - )?; + let layer_id = self.existing_layer_id(glyph_ref)?; + let layer = self.workspace_mut()?.set_x_advance(layer_id, width)?; + let change = GlyphValueChange::from_layer(&layer, Default::default()); self.mark_font_changed(); Ok(change.into()) @@ -723,14 +696,9 @@ impl Bridge { dx: f64, dy: f64, ) -> errors::Result { - let change = self.edit_glyph_layer( - glyph_ref, - |layer| { - layer.translate_layer(dx, dy); - Ok(GlyphValueChange::from_layer(layer, Default::default())) - }, - Self::layer_replaced_change, - )?; + let layer_id = self.existing_layer_id(glyph_ref)?; + let layer = self.workspace_mut()?.translate_layer(layer_id, dx, dy)?; + let change = GlyphValueChange::from_layer(&layer, Default::default()); self.mark_font_changed(); Ok(change.into()) @@ -749,33 +717,15 @@ impl Bridge { let contour_id = parse::(&contour_id)?; let point_type = point_type.into(); - let change = self.edit_glyph_layer( - glyph_ref, - |layer| { - let point_id = layer.add_point_to_contour(contour_id, x, y, point_type, smooth)?; - let changed = GlyphChangedEntities { - point_ids: vec![point_id], - ..Default::default() - }; - Ok(GlyphStructureChange::from_layer(layer, changed)) - }, - move |target, layer, change| { - let contour = layer - .contour(contour_id) - .map(shift_font::ContourValue::from); - contour - .map(|contour| { - Self::one_change(shift_font::FontChange::PointsAdded( - shift_font::PointsAdded { - target: target.clone(), - contour, - point_ids: change.changed.point_ids.clone(), - }, - )) - }) - .unwrap_or_default() - }, - )?; + let layer_id = self.existing_layer_id(glyph_ref)?; + let (layer, point_id) = self + .workspace_mut()? + .add_point(layer_id, contour_id, x, y, point_type, smooth)?; + let changed = GlyphChangedEntities { + point_ids: vec![point_id], + ..Default::default() + }; + let change = GlyphStructureChange::from_layer(&layer, changed); self.mark_font_changed(); Ok(change.into()) @@ -794,68 +744,37 @@ impl Bridge { let before_point_id = parse::(&before_point_id)?; let point_type = point_type.into(); - let change = self.edit_glyph_layer( - glyph_ref, - |layer| { - let point_id = layer.insert_point_before(before_point_id, x, y, point_type, smooth)?; - let changed = GlyphChangedEntities { - point_ids: vec![point_id], - ..Default::default() - }; - Ok(GlyphStructureChange::from_layer(layer, changed)) - }, - move |target, layer, change| { - let Some(contour_id) = layer.find_point_contour(before_point_id) else { - return Self::layer_replaced_change(target, layer, change); - }; - let Some(contour) = layer - .contour(contour_id) - .map(shift_font::ContourValue::from) - else { - return Self::layer_replaced_change(target, layer, change); - }; - Self::one_change(shift_font::FontChange::PointsAdded( - shift_font::PointsAdded { - target: target.clone(), - contour, - point_ids: change.changed.point_ids.clone(), - }, - )) - }, + let layer_id = self.existing_layer_id(glyph_ref)?; + let (layer, point_id) = self.workspace_mut()?.insert_point_before( + layer_id, + before_point_id, + x, + y, + point_type, + smooth, )?; + let changed = GlyphChangedEntities { + point_ids: vec![point_id], + ..Default::default() + }; + let change = GlyphStructureChange::from_layer(&layer, changed); self.mark_font_changed(); Ok(change.into()) } #[napi] - pub fn add_contour(&mut self, glyph_ref: GlyphLayerRef) -> Result { - let change = self.edit_glyph_layer( - glyph_ref, - |layer| { - let contour_id = layer.add_empty_contour(); - let changed = GlyphChangedEntities { - contour_ids: vec![contour_id], - ..Default::default() - }; - Ok(GlyphStructureChange::from_layer(layer, changed)) - }, - |target, layer, change| { - let Some(contour) = layer - .contours_iter() - .last() - .map(shift_font::ContourValue::from) - else { - return Self::layer_replaced_change(target, layer, change); - }; - Self::one_change(shift_font::FontChange::ContourAdded( - shift_font::ContourAdded { - target: target.clone(), - contour, - }, - )) - }, - )?; + pub fn add_contour( + &mut self, + glyph_ref: GlyphLayerRef, + ) -> errors::Result { + let layer_id = self.existing_layer_id(glyph_ref)?; + let (layer, contour_id) = self.workspace_mut()?.add_contour(layer_id)?; + let changed = GlyphChangedEntities { + contour_ids: vec![contour_id], + ..Default::default() + }; + let change = GlyphStructureChange::from_layer(&layer, changed); self.mark_font_changed(); Ok(change.into()) @@ -868,26 +787,13 @@ impl Bridge { #[napi(ts_arg_type = "ContourId")] contour_id: String, ) -> errors::Result { let contour_id = parse::(&contour_id)?; - let change = self.edit_glyph_layer( - glyph_ref, - |layer| { - layer.open_contour(contour_id)?; - let changed = GlyphChangedEntities { - contour_ids: vec![contour_id], - ..Default::default() - }; - Ok(GlyphStructureChange::from_layer(layer, changed)) - }, - move |target, _layer, _change| { - Self::one_change(shift_font::FontChange::ContourOpenClosedChanged( - shift_font::ContourOpenClosedChanged { - target: target.clone(), - contour_id, - closed: false, - }, - )) - }, - )?; + let layer_id = self.existing_layer_id(glyph_ref)?; + let layer = self.workspace_mut()?.open_contour(layer_id, contour_id)?; + let changed = GlyphChangedEntities { + contour_ids: vec![contour_id], + ..Default::default() + }; + let change = GlyphStructureChange::from_layer(&layer, changed); self.mark_font_changed(); Ok(change.into()) @@ -900,26 +806,13 @@ impl Bridge { #[napi(ts_arg_type = "ContourId")] contour_id: String, ) -> errors::Result { let contour_id = parse::(&contour_id)?; - let change = self.edit_glyph_layer( - glyph_ref, - |layer| { - layer.close_contour(contour_id)?; - let changed = GlyphChangedEntities { - contour_ids: vec![contour_id], - ..Default::default() - }; - Ok(GlyphStructureChange::from_layer(layer, changed)) - }, - move |target, _layer, _change| { - Self::one_change(shift_font::FontChange::ContourOpenClosedChanged( - shift_font::ContourOpenClosedChanged { - target: target.clone(), - contour_id, - closed: true, - }, - )) - }, - )?; + let layer_id = self.existing_layer_id(glyph_ref)?; + let layer = self.workspace_mut()?.close_contour(layer_id, contour_id)?; + let changed = GlyphChangedEntities { + contour_ids: vec![contour_id], + ..Default::default() + }; + let change = GlyphStructureChange::from_layer(&layer, changed); self.mark_font_changed(); Ok(change.into()) @@ -932,18 +825,15 @@ impl Bridge { #[napi(ts_arg_type = "ContourId")] contour_id: String, ) -> errors::Result { let contour_id = parse::(&contour_id)?; - let change = self.edit_glyph_layer( - glyph_ref, - |layer| { - layer.reverse_contour(contour_id)?; - let changed = GlyphChangedEntities { - contour_ids: vec![contour_id], - ..Default::default() - }; - Ok(GlyphStructureChange::from_layer(layer, changed)) - }, - Self::layer_replaced_change, - )?; + let layer_id = self.existing_layer_id(glyph_ref)?; + let layer = self + .workspace_mut()? + .reverse_contour(layer_id, contour_id)?; + let changed = GlyphChangedEntities { + contour_ids: vec![contour_id], + ..Default::default() + }; + let change = GlyphStructureChange::from_layer(&layer, changed); self.mark_font_changed(); Ok(change.into()) @@ -973,18 +863,15 @@ impl Bridge { } }; - let change = self.edit_glyph_layer( - glyph_ref, - |layer| { - let created_ids = layer.apply_boolean_op(cid_a, cid_b, op)?; - let changed = GlyphChangedEntities { - contour_ids: created_ids, - ..Default::default() - }; - Ok(GlyphStructureChange::from_layer(layer, changed)) - }, - Self::layer_replaced_change, - )?; + let layer_id = self.existing_layer_id(glyph_ref)?; + let (layer, contour_ids) = self + .workspace_mut()? + .apply_boolean_op(layer_id, cid_a, cid_b, op)?; + let changed = GlyphChangedEntities { + contour_ids, + ..Default::default() + }; + let change = GlyphStructureChange::from_layer(&layer, changed); self.mark_font_changed(); Ok(change.into()) @@ -999,15 +886,11 @@ impl Bridge { let point_ids: BridgeResult> = point_ids.iter().map(|id| parse::(id)).collect(); let point_ids = point_ids?; - let change = self.edit_glyph_layer( - glyph_ref, - |layer| { - layer.remove_points(&point_ids)?; - let changed = GlyphChangedEntities::points(point_ids); - Ok(GlyphStructureChange::from_layer(layer, changed)) - }, - Self::layer_replaced_change, - )?; + let layer_id = self.existing_layer_id(glyph_ref)?; + let layer = self + .workspace_mut()? + .remove_points(layer_id, point_ids.clone())?; + let change = GlyphStructureChange::from_layer(&layer, GlyphChangedEntities::points(point_ids)); self.mark_font_changed(); Ok(change.into()) @@ -1020,34 +903,13 @@ impl Bridge { #[napi(ts_arg_type = "PointId")] point_id: String, ) -> errors::Result { let parsed_id = parse::(&point_id)?; - let change = self.edit_glyph_layer( - glyph_ref, - |layer| { - layer.toggle_smooth(parsed_id)?; - let changed = GlyphChangedEntities { - point_ids: vec![parsed_id], - ..Default::default() - }; - Ok(GlyphStructureChange::from_layer(layer, changed)) - }, - move |target, layer, change| { - let smooth = layer - .contours_iter() - .find_map(|contour| contour.get_point(parsed_id)) - .map(|point| point.is_smooth()); - smooth - .map(|smooth| { - Self::one_change(shift_font::FontChange::PointSmoothChanged( - shift_font::PointSmoothChanged { - target: target.clone(), - point_id: parsed_id, - smooth, - }, - )) - }) - .unwrap_or_else(|| Self::layer_replaced_change(target, layer, change)) - }, - )?; + let layer_id = self.existing_layer_id(glyph_ref)?; + let layer = self.workspace_mut()?.toggle_smooth(layer_id, parsed_id)?; + let changed = GlyphChangedEntities { + point_ids: vec![parsed_id], + ..Default::default() + }; + let change = GlyphStructureChange::from_layer(&layer, changed); self.mark_font_changed(); Ok(change.into()) @@ -1069,42 +931,34 @@ impl Bridge { || anchor_coords .as_ref() .is_some_and(|coords| !coords.is_empty()); + let layer_id = self.existing_layer_id(glyph_ref)?; + let point_id_slice = point_ids.as_ref().map(|ids| { + let ids: &[u64] = ids; + ids + }); + let point_coord_slice = point_coords.as_ref().map(|coords| { + let coords: &[f64] = coords; + coords + }); + let anchor_id_slice = anchor_ids.as_ref().map(|ids| { + let ids: &[u64] = ids; + ids + }); + let anchor_coord_slice = anchor_coords.as_ref().map(|coords| { + let coords: &[f64] = coords; + coords + }); - self.edit_glyph_layer( - glyph_ref, - |layer| { - layer.apply_bulk_node_positions(BulkNodePositionUpdates { - point_ids: point_ids.as_ref().map(|ids| { - let ids: &[u64] = ids; - ids - }), - point_coords: point_coords.as_ref().map(|coords| { - let coords: &[f64] = coords; - coords - }), - anchor_ids: anchor_ids.as_ref().map(|ids| { - let ids: &[u64] = ids; - ids - }), - anchor_coords: anchor_coords.as_ref().map(|coords| { - let coords: &[f64] = coords; - coords - }), - })?; - Ok(()) - }, - move |target, layer, result| { - if has_anchor_updates { - return Self::layer_replaced_change(target, layer, result); - } - - Self::one_change(shift_font::FontChange::PointPositionsChanged( - shift_font::PointPositionsChanged { - target: target.clone(), - points: point_position_changes, - }, - )) + self.workspace_mut()?.apply_position_patch( + layer_id, + BulkNodePositionUpdates { + point_ids: point_id_slice, + point_coords: point_coord_slice, + anchor_ids: anchor_id_slice, + anchor_coords: anchor_coord_slice, }, + point_position_changes, + has_anchor_updates, )?; self.mark_font_changed(); @@ -1120,15 +974,15 @@ impl Bridge { ) -> errors::Result { let structure = GlyphStructure::from(structure); let values: &[f64] = &values; - - let change = self.edit_glyph_layer( - glyph_ref, - |layer| { - apply_state_to_layer(layer, &structure, values)?; - Ok(GlyphStructureChange::from_layer(layer, Default::default())) - }, - Self::layer_replaced_change, - )?; + let layer_id = self.existing_layer_id(glyph_ref)?; + let mut layer = self + .font()? + .layer(layer_id) + .ok_or(shift_font::error::CoreError::LayerNotFound(layer_id))? + .clone(); + apply_state_to_layer(&mut layer, &structure, values)?; + let layer = self.workspace_mut()?.replace_layer(layer_id, layer)?; + let change = GlyphStructureChange::from_layer(&layer, Default::default()); self.mark_font_changed(); Ok(change.into()) @@ -1220,6 +1074,21 @@ mod tests { } } + fn create_default_glyph_layer( + bridge: &mut Bridge, + name: &str, + unicode: Option, + ) -> GlyphLayerRef { + let glyph_ref = default_layer_ref(bridge, name, unicode); + let unicodes = unicode.into_iter().collect(); + let glyph_id = bridge.create_glyph(name.to_string(), unicodes).unwrap(); + bridge + .create_glyph_layer(glyph_id, glyph_ref.source_id.clone()) + .unwrap(); + + glyph_ref + } + #[test] fn new_bridge_requires_workspace_before_font_reads() { let bridge = Bridge::new(); @@ -1253,9 +1122,8 @@ mod tests { #[test] fn create_workspace_resets_to_fresh_font_state() { let mut bridge = bridge_with_workspace(); - bridge - .set_x_advance(default_layer_ref(&bridge, "A", Some(65)), 500.0) - .unwrap(); + let glyph_ref = create_default_glyph_layer(&mut bridge, "A", Some(65)); + bridge.add_contour(glyph_ref).unwrap(); let (source_path, store_path) = test_paths("reset"); bridge @@ -1271,7 +1139,7 @@ mod tests { #[test] fn add_contour_returns_structure_change() { let mut bridge = bridge_with_workspace(); - let glyph_ref = default_layer_ref(&bridge, "A", Some(65)); + let glyph_ref = create_default_glyph_layer(&mut bridge, "A", Some(65)); let change = bridge.add_contour(glyph_ref).unwrap(); @@ -1285,9 +1153,41 @@ mod tests { } #[test] - fn save_snapshot_includes_direct_glyph_layer_edit() { + fn set_x_advance_requires_existing_glyph_layer() { let mut bridge = bridge_with_workspace(); let glyph_ref = default_layer_ref(&bridge, "A", Some(65)); + + let error = match bridge.set_x_advance(glyph_ref, 640.0) { + Ok(_) => panic!("set_x_advance should require an existing glyph layer"), + Err(error) => error, + }; + + assert!(matches!( + error, + BridgeError::InvalidInput { + kind: "glyph layer", + .. + } + )); + assert_eq!(bridge.get_glyph_count().unwrap(), 0); + } + + #[test] + fn set_x_advance_updates_existing_glyph_layer() { + let mut bridge = bridge_with_workspace(); + let glyph_ref = create_default_glyph_layer(&mut bridge, "A", Some(65)); + + let change = bridge.set_x_advance(glyph_ref, 640.0).unwrap(); + + assert_eq!(&change.values[..], &[640.0]); + assert!(change.changed.contour_ids.is_empty()); + assert!(change.changed.point_ids.is_empty()); + } + + #[test] + fn save_snapshot_includes_direct_glyph_layer_edit() { + let mut bridge = bridge_with_workspace(); + let glyph_ref = create_default_glyph_layer(&mut bridge, "A", Some(65)); let contour_id = bridge .add_contour(glyph_ref.clone()) .unwrap() @@ -1346,7 +1246,7 @@ mod tests { #[test] fn persisted_older_snapshot_keeps_document_dirty_after_new_edit() { let mut bridge = bridge_with_workspace(); - let glyph_ref = default_layer_ref(&bridge, "A", Some(65)); + let glyph_ref = create_default_glyph_layer(&mut bridge, "A", Some(65)); let contour_id = bridge .add_contour(glyph_ref.clone()) .unwrap() @@ -1367,16 +1267,19 @@ mod tests { .unwrap(); record_persisted_version(&bridge.persisted_version, snapshot_version); - assert_eq!(snapshot_version.as_u32(), 1); - assert_eq!(bridge.live_version().as_u32(), 2); - assert_eq!(bridge.get_persisted_version(), 1); + assert!(snapshot_version.as_u32() > 0); + assert_eq!( + bridge.live_version().as_u32(), + snapshot_version.as_u32() + 1 + ); + assert_eq!(bridge.get_persisted_version(), snapshot_version.as_u32()); assert!(bridge.is_dirty()); } #[test] fn opening_workspace_resets_persisted_version_handle_for_old_saves() { let mut bridge = bridge_with_workspace(); - let glyph_ref = default_layer_ref(&bridge, "A", Some(65)); + let glyph_ref = create_default_glyph_layer(&mut bridge, "A", Some(65)); bridge.add_contour(glyph_ref).unwrap(); let old_persisted_version = bridge.persisted_version.clone(); @@ -1393,7 +1296,7 @@ mod tests { #[test] fn add_point_returns_structure_and_changed_point() { let mut bridge = bridge_with_workspace(); - let glyph_ref = default_layer_ref(&bridge, "A", Some(65)); + let glyph_ref = create_default_glyph_layer(&mut bridge, "A", Some(65)); let contour_id = bridge .add_contour(glyph_ref.clone()) .unwrap() @@ -1423,7 +1326,7 @@ mod tests { #[test] fn get_glyph_state_reads_direct_glyph_layer_edit() { let mut bridge = bridge_with_workspace(); - let glyph_ref = default_layer_ref(&bridge, "A", Some(65)); + let glyph_ref = create_default_glyph_layer(&mut bridge, "A", Some(65)); let contour_id = bridge .add_contour(glyph_ref.clone()) .unwrap() @@ -1471,6 +1374,12 @@ mod tests { source_id: "not-a-source-id".to_string(), }); - assert!(result.err().unwrap().reason.contains("invalid source ID")); + assert!(matches!( + result.err().unwrap(), + BridgeError::InvalidInput { + kind: "source ID", + .. + } + )); } } diff --git a/crates/shift-bridge/src/input.rs b/crates/shift-bridge/src/input.rs index 2f622efa..53312882 100644 --- a/crates/shift-bridge/src/input.rs +++ b/crates/shift-bridge/src/input.rs @@ -1,6 +1,8 @@ use std::str::FromStr; -use shift_font::{AnchorId, ComponentId, ContourId, GuidelineId, LayerId, PointId, SourceId}; +use shift_font::{ + AnchorId, ComponentId, ContourId, GlyphId, GuidelineId, LayerId, PointId, SourceId, +}; use crate::errors::{BridgeError, BridgeResult}; @@ -23,6 +25,10 @@ impl BridgeParse for ContourId { const KIND: &'static str = "contour ID"; } +impl BridgeParse for GlyphId { + const KIND: &'static str = "glyph ID"; +} + impl BridgeParse for PointId { const KIND: &'static str = "point ID"; } diff --git a/crates/shift-font/src/changes.rs b/crates/shift-font/src/changes.rs index 413bab7e..2884f615 100644 --- a/crates/shift-font/src/changes.rs +++ b/crates/shift-font/src/changes.rs @@ -22,6 +22,12 @@ impl FontChangeSet { } } +impl From for FontChangeSet { + fn from(change: FontChange) -> Self { + Self::new(vec![change]) + } +} + #[derive(Clone, Debug)] pub enum FontChange { GlyphCreated(GlyphCreated), @@ -36,6 +42,89 @@ pub enum FontChange { LayerGeometryReplaced(LayerGeometryReplaced), } +impl FontChange { + pub fn glyph_created(glyph: &Glyph) -> Self { + Self::GlyphCreated(GlyphCreated::from(glyph)) + } + + pub fn glyph_identity_changed( + glyph_id: GlyphId, + from_name: GlyphName, + to_name: GlyphName, + from_unicodes: Vec, + to_unicodes: Vec, + ) -> Self { + Self::GlyphIdentityChanged(GlyphIdentityChanged { + glyph_id, + from_name, + to_name, + from_unicodes, + to_unicodes, + }) + } + + pub fn layer_metrics_changed(target: GlyphLayerChangeTarget, layer: &GlyphLayer) -> Self { + Self::LayerMetricsChanged(LayerMetricsChanged::from_layer(target, layer)) + } + + pub fn contour_added(target: GlyphLayerChangeTarget, contour: &Contour) -> Self { + Self::ContourAdded(ContourAdded { + target, + contour: ContourValue::from(contour), + }) + } + + pub fn contour_open_closed_changed( + target: GlyphLayerChangeTarget, + contour_id: ContourId, + closed: bool, + ) -> Self { + Self::ContourOpenClosedChanged(ContourOpenClosedChanged { + target, + contour_id, + closed, + }) + } + + pub fn points_added( + target: GlyphLayerChangeTarget, + contour: &Contour, + point_ids: Vec, + ) -> Self { + Self::PointsAdded(PointsAdded { + target, + contour: ContourValue::from(contour), + point_ids, + }) + } + + pub fn point_smooth_changed( + target: GlyphLayerChangeTarget, + point_id: PointId, + smooth: bool, + ) -> Self { + Self::PointSmoothChanged(PointSmoothChanged { + target, + point_id, + smooth, + }) + } + + pub fn point_positions_changed( + target: GlyphLayerChangeTarget, + points: Vec, + ) -> Self { + Self::PointPositionsChanged(PointPositionsChanged { target, points }) + } + + pub fn layer_geometry_replaced(target: GlyphLayerChangeTarget, layer: &GlyphLayer) -> Self { + Self::LayerGeometryReplaced(LayerGeometryReplaced { + target, + layer: GlyphLayerValue::from(layer), + }) + } +} + #[derive(Clone, Debug)] pub struct GlyphCreated { pub glyph_id: GlyphId, diff --git a/crates/shift-font/src/layer_edit.rs b/crates/shift-font/src/layer_edit.rs index 8fad9439..021ccaf1 100644 --- a/crates/shift-font/src/layer_edit.rs +++ b/crates/shift-font/src/layer_edit.rs @@ -16,6 +16,12 @@ pub struct ChangedEntities { pub component_ids: Vec, } +#[derive(Clone, Debug)] +pub struct AddedPoint { + pub point_id: PointId, + pub contour: Contour, +} + impl ChangedEntities { pub fn points(point_ids: Vec) -> Self { Self { @@ -398,11 +404,12 @@ impl GlyphLayer { } impl GlyphLayer { - pub fn add_empty_contour(&mut self) -> ContourId { + pub fn add_empty_contour(&mut self) -> Contour { let contour = Contour::new(); - let contour_id = contour.id(); + let created_contour = contour.clone(); self.add_contour(contour); - contour_id + + created_contour } pub fn remove_contour_checked(&mut self, contour_id: ContourId) -> CoreResult { @@ -451,8 +458,8 @@ impl GlyphLayer { let mut created_ids = Vec::new(); for contour in result.0 { - let id = self.add_contour(contour); - created_ids.push(id); + let contour_id = self.add_contour(contour); + created_ids.push(contour_id); } Ok(created_ids) @@ -476,11 +483,14 @@ impl GlyphLayer { y: f64, point_type: PointType, is_smooth: bool, - ) -> CoreResult { + ) -> CoreResult { let contour = self.contour_mut_or_err(contour_id)?; let point_id = contour.add_point(x, y, point_type, is_smooth); - Ok(point_id) + Ok(AddedPoint { + point_id, + contour: contour.clone(), + }) } pub fn insert_point_before( @@ -490,7 +500,7 @@ impl GlyphLayer { y: f64, point_type: PointType, is_smooth: bool, - ) -> CoreResult { + ) -> CoreResult { let contour_id = self.point_contour_or_err(before_id)?; let contour = self.contour_mut_or_err(contour_id)?; @@ -498,7 +508,10 @@ impl GlyphLayer { .insert_point_before(before_id, x, y, point_type, is_smooth) .ok_or(CoreError::PointNotFound(before_id))?; - Ok(point_id) + Ok(AddedPoint { + point_id, + contour: contour.clone(), + }) } pub fn remove_point(&mut self, point_id: PointId) -> CoreResult<()> { @@ -540,13 +553,13 @@ impl GlyphLayer { Ok(()) } - pub fn toggle_smooth(&mut self, point_id: PointId) -> CoreResult<()> { + pub fn toggle_smooth(&mut self, point_id: PointId) -> CoreResult { let contour_id = self.point_contour_or_err(point_id)?; let point = self.point_mut_or_err(contour_id, point_id)?; point.toggle_smooth(); - Ok(()) + Ok(point.is_smooth()) } pub fn remove_points(&mut self, point_ids: &[PointId]) -> CoreResult<()> { @@ -728,7 +741,7 @@ mod tests { fn session_with_contour() -> (GlyphLayer, ContourId) { let mut session = create_session(); - let contour_id = session.add_empty_contour(); + let contour_id = session.add_empty_contour().id(); (session, contour_id) } @@ -736,6 +749,7 @@ mod tests { session .add_point_to_contour(contour_id, x, y, PointType::OnCurve, false) .unwrap() + .point_id } fn add_anchor(session: &mut GlyphLayer, x: f64, y: f64) -> AnchorId { @@ -901,15 +915,17 @@ mod tests { #[test] fn move_points_across_contours() { let mut session = create_session(); - let c1_id = session.add_empty_contour(); + let c1_id = session.add_empty_contour().id(); let p1 = session .add_point_to_contour(c1_id, 0.0, 0.0, PointType::OnCurve, false) .unwrap(); + let p1 = p1.point_id; - let c2_id = session.add_empty_contour(); + let c2_id = session.add_empty_contour().id(); let p2 = session .add_point_to_contour(c2_id, 50.0, 50.0, PointType::OnCurve, false) .unwrap(); + let p2 = p2.point_id; session.move_points(&[p1, p2], 5.0, 5.0).unwrap(); @@ -1000,10 +1016,11 @@ mod tests { fn translate_layer_moves_points_and_anchors_without_changing_width() { let mut session = create_session(); let original_width = session.width(); - let contour_id = session.add_empty_contour(); + let contour_id = session.add_empty_contour().id(); let point_id = session .add_point_to_contour(contour_id, 10.0, 20.0, PointType::OnCurve, false) .unwrap(); + let point_id = point_id.point_id; let anchor_id = session.add_anchor(Anchor::new(Some("top".to_string()), 30.0, 40.0)); session.translate_layer(5.0, -3.0); @@ -1072,6 +1089,7 @@ mod tests { let control = session .insert_point_before(anchor2, 50.0, 75.0, PointType::OffCurve, false) .unwrap(); + let control = control.point_id; let contour = session.contour(contour_id).unwrap(); let points: Vec<_> = contour.points().iter().collect(); diff --git a/crates/shift-workspace/src/lib.rs b/crates/shift-workspace/src/lib.rs index 53e8d1d5..91ff39cf 100644 --- a/crates/shift-workspace/src/lib.rs +++ b/crates/shift-workspace/src/lib.rs @@ -2,4 +2,4 @@ mod new_workspace; mod workspace; pub use new_workspace::NewWorkspace; -pub use workspace::{FontWorkspace, GlyphLayerTarget, WorkspaceError, WorkspaceSource}; +pub use workspace::{FontWorkspace, WorkspaceError, WorkspaceSource}; diff --git a/crates/shift-workspace/src/workspace.rs b/crates/shift-workspace/src/workspace.rs index c5bb4b90..6b49750a 100644 --- a/crates/shift-workspace/src/workspace.rs +++ b/crates/shift-workspace/src/workspace.rs @@ -2,7 +2,8 @@ use std::path::{Path, PathBuf}; use shift_backends::{FontExportRequest, FontExportResult, FontExporter, font_loader::FontLoader}; use shift_font::{ - FontChange, FontChangeSet, Glyph, GlyphCreated, GlyphLayer, GlyphLayerChangeTarget, LayerId, + BooleanOp, BulkNodePositionUpdates, ContourId, FontChange, FontChangeSet, Glyph, GlyphId, + GlyphLayer, GlyphLayerChangeTarget, GlyphName, LayerId, PointId, PointPosition, PointType, SourceId, error::CoreError, }; use shift_source::ShiftSourcePackage; @@ -43,14 +44,6 @@ pub enum WorkspaceSource { Imported { original_path: PathBuf }, } -#[derive(Clone, Debug, Eq, PartialEq)] -pub struct GlyphLayerTarget { - pub glyph_name: String, - pub unicode: Option, - pub source_id: SourceId, - pub layer_id: LayerId, -} - pub struct FontWorkspace { font: shift_font::Font, source: WorkspaceSource, @@ -180,102 +173,278 @@ impl FontWorkspace { self.commit_font( next_font, - FontChangeSet::new(vec![FontChange::GlyphIdentityChanged( - shift_font::GlyphIdentityChanged { - glyph_id, - from_name, - to_name, - from_unicodes, - to_unicodes, - }, - )]), + FontChange::glyph_identity_changed( + glyph_id, + from_name, + to_name, + from_unicodes, + to_unicodes, + ) + .into(), )?; Ok(()) } - pub fn edit_glyph_layer( + pub fn create_glyph( &mut self, - target: GlyphLayerTarget, - edit: impl FnOnce(&mut GlyphLayer) -> Result, - changes: impl FnOnce(&GlyphLayerChangeTarget, &GlyphLayer, &R) -> FontChangeSet, - ) -> Result { - let mut next_font = self.font.clone(); - let mut change_set = FontChangeSet::default(); - - if next_font.glyph_id_by_name(&target.glyph_name).is_none() { - let mut glyph = Glyph::new(target.glyph_name.clone()); - if let Some(unicode) = target.unicode { - glyph.add_unicode(unicode); + name: String, + unicodes: Vec, + ) -> Result { + self.commit_edit(|font| { + let name = name.trim(); + if name.is_empty() { + return Err(WorkspaceError::InvalidInput { + kind: "glyph name", + value: name.to_string(), + }); + } + if font.glyph_id_by_name(name).is_some() { + return Err(WorkspaceError::InvalidInput { + kind: "glyph name", + value: format!("{name} already exists"), + }); } - glyph.set_layer(GlyphLayer::with_width( - target.layer_id, - target.source_id, - 500.0, - )); - change_set.push(FontChange::GlyphCreated(GlyphCreated::from(&glyph))); - next_font.insert_glyph(glyph)?; - } - if !next_font - .sources() - .iter() - .any(|source| source.id() == target.source_id) - { - return Err(WorkspaceError::InvalidInput { - kind: "source ID", - value: target.source_id.to_string(), - }); - } + let glyph_name = + GlyphName::new(name.to_string()).map_err(|_| WorkspaceError::InvalidInput { + kind: "glyph name", + value: name.to_string(), + })?; + let mut glyph = Glyph::new(glyph_name); + glyph.set_unicodes(unicodes); + let created_glyph = glyph.clone(); + let change_set = FontChange::glyph_created(&glyph).into(); - let glyph_id = next_font - .glyph_id_by_name(&target.glyph_name) - .ok_or_else(|| WorkspaceError::InvalidInput { - kind: "glyph name", - value: target.glyph_name.clone(), - })?; + font.insert_glyph(glyph)?; - if next_font.layer(target.layer_id).is_none() { - next_font.insert_glyph_layer( + Ok((created_glyph, change_set)) + }) + } + + pub fn create_glyph_layer( + &mut self, + glyph_id: GlyphId, + source_id: SourceId, + ) -> Result { + self.commit_edit(|font| { + let glyph = font + .glyph(glyph_id) + .ok_or(CoreError::GlyphNotFound(glyph_id))?; + let glyph_name = glyph.glyph_name().clone(); + let layer = GlyphLayer::with_width(LayerId::new(), source_id, 500.0); + let layer_id = layer.id(); + + font.insert_glyph_layer(glyph_id, layer)?; + let layer = font + .layer(layer_id) + .ok_or(CoreError::LayerNotFound(layer_id))? + .clone(); + let target = GlyphLayerChangeTarget { glyph_id, - GlyphLayer::with_width(target.layer_id, target.source_id, 500.0), - )?; - } + glyph_name, + source_id, + layer_id, + }; + let change_set = FontChange::layer_metrics_changed(target, &layer).into(); - let glyph = next_font - .glyph(glyph_id) - .ok_or_else(|| WorkspaceError::InvalidInput { - kind: "glyph name", - value: target.glyph_name.clone(), - })?; - let change_target = GlyphLayerChangeTarget { - glyph_id: glyph.id(), - glyph_name: glyph.glyph_name().clone(), - source_id: target.source_id, - layer_id: target.layer_id, - }; + Ok((layer, change_set)) + }) + } - if let Some(unicode) = target.unicode - && !glyph.unicodes().contains(&unicode) - { - let mut unicodes = glyph.unicodes().to_vec(); - unicodes.push(unicode); - next_font.set_glyph_unicodes(glyph_id, unicodes)?; - } + pub fn set_x_advance( + &mut self, + layer_id: LayerId, + width: f64, + ) -> Result { + self.commit_edit(|font| { + let target = Self::layer_change_target(font, layer_id)?; + let layer = font + .layer_mut(layer_id) + .ok_or(CoreError::LayerNotFound(layer_id))?; + + layer.set_x_advance(width); + let edited_layer = layer.clone(); + let change_set = FontChange::layer_metrics_changed(target, layer).into(); + + Ok((edited_layer, change_set)) + }) + } - let layer = - next_font - .layer_mut(target.layer_id) - .ok_or_else(|| WorkspaceError::InvalidInput { - kind: "layer ID", - value: target.layer_id.to_string(), - })?; - let result = edit(layer)?; - let layer_changes = changes(&change_target, layer, &result); - change_set.changes.extend(layer_changes.changes); + pub fn translate_layer( + &mut self, + layer_id: LayerId, + dx: f64, + dy: f64, + ) -> Result { + self.replace_layer_geometry(layer_id, |layer| { + layer.translate_layer(dx, dy); + Ok(()) + }) + } - self.commit_font(next_font, change_set)?; + pub fn add_point( + &mut self, + layer_id: LayerId, + contour_id: ContourId, + x: f64, + y: f64, + point_type: PointType, + smooth: bool, + ) -> Result<(GlyphLayer, PointId), WorkspaceError> { + self.commit_edit(|font| { + let target = Self::layer_change_target(font, layer_id)?; + let layer = font + .layer_mut(layer_id) + .ok_or(CoreError::LayerNotFound(layer_id))?; + let added = layer.add_point_to_contour(contour_id, x, y, point_type, smooth)?; + let edited_layer = layer.clone(); + let change_set = + FontChange::points_added(target, &added.contour, vec![added.point_id]).into(); + + Ok(((edited_layer, added.point_id), change_set)) + }) + } - Ok(result) + pub fn insert_point_before( + &mut self, + layer_id: LayerId, + before_point_id: PointId, + x: f64, + y: f64, + point_type: PointType, + smooth: bool, + ) -> Result<(GlyphLayer, PointId), WorkspaceError> { + self.commit_edit(|font| { + let target = Self::layer_change_target(font, layer_id)?; + let layer = font + .layer_mut(layer_id) + .ok_or(CoreError::LayerNotFound(layer_id))?; + let added = layer.insert_point_before(before_point_id, x, y, point_type, smooth)?; + let edited_layer = layer.clone(); + let change_set = + FontChange::points_added(target, &added.contour, vec![added.point_id]).into(); + + Ok(((edited_layer, added.point_id), change_set)) + }) + } + + pub fn add_contour( + &mut self, + layer_id: LayerId, + ) -> Result<(GlyphLayer, ContourId), WorkspaceError> { + self.commit_edit(|font| { + let target = Self::layer_change_target(font, layer_id)?; + let layer = font + .layer_mut(layer_id) + .ok_or(CoreError::LayerNotFound(layer_id))?; + let contour = layer.add_empty_contour(); + let edited_layer = layer.clone(); + let change_set = FontChange::contour_added(target, &contour).into(); + + Ok(((edited_layer, contour.id()), change_set)) + }) + } + + pub fn open_contour( + &mut self, + layer_id: LayerId, + contour_id: ContourId, + ) -> Result { + self.set_contour_closed(layer_id, contour_id, false) + } + + pub fn close_contour( + &mut self, + layer_id: LayerId, + contour_id: ContourId, + ) -> Result { + self.set_contour_closed(layer_id, contour_id, true) + } + + pub fn reverse_contour( + &mut self, + layer_id: LayerId, + contour_id: ContourId, + ) -> Result { + self.replace_layer_geometry(layer_id, |layer| { + layer.reverse_contour(contour_id)?; + Ok(()) + }) + } + + pub fn apply_boolean_op( + &mut self, + layer_id: LayerId, + contour_id_a: ContourId, + contour_id_b: ContourId, + operation: BooleanOp, + ) -> Result<(GlyphLayer, Vec), WorkspaceError> { + self.replace_layer_geometry_with_result(layer_id, |layer| { + layer.apply_boolean_op(contour_id_a, contour_id_b, operation) + }) + } + + pub fn remove_points( + &mut self, + layer_id: LayerId, + point_ids: Vec, + ) -> Result { + self.replace_layer_geometry(layer_id, |layer| { + layer.remove_points(&point_ids)?; + Ok(()) + }) + } + + pub fn toggle_smooth( + &mut self, + layer_id: LayerId, + point_id: PointId, + ) -> Result { + self.commit_edit(|font| { + let target = Self::layer_change_target(font, layer_id)?; + let layer = font + .layer_mut(layer_id) + .ok_or(CoreError::LayerNotFound(layer_id))?; + let smooth = layer.toggle_smooth(point_id)?; + let edited_layer = layer.clone(); + let change_set = FontChange::point_smooth_changed(target, point_id, smooth).into(); + + Ok((edited_layer, change_set)) + }) + } + + pub fn apply_position_patch( + &mut self, + layer_id: LayerId, + updates: BulkNodePositionUpdates<'_>, + point_position_changes: Vec, + replace_geometry: bool, + ) -> Result<(), WorkspaceError> { + self.commit_edit(|font| { + let target = Self::layer_change_target(font, layer_id)?; + let layer = font + .layer_mut(layer_id) + .ok_or(CoreError::LayerNotFound(layer_id))?; + layer.apply_bulk_node_positions(updates)?; + let change_set = if replace_geometry { + Self::layer_replaced_change_set(target, layer) + } else { + FontChange::point_positions_changed(target, point_position_changes).into() + }; + + Ok(((), change_set)) + }) + } + + pub fn replace_layer( + &mut self, + layer_id: LayerId, + replacement: GlyphLayer, + ) -> Result { + self.replace_layer_geometry(layer_id, |layer| { + *layer = replacement; + Ok(()) + }) } fn commit_font( @@ -288,6 +457,100 @@ impl FontWorkspace { Ok(()) } + fn commit_edit(&mut self, edit: F) -> Result + where + F: FnOnce(&mut shift_font::Font) -> Result<(R, FontChangeSet), WorkspaceError>, + { + let mut next_font = self.font.clone(); + let (result, change_set) = edit(&mut next_font)?; + self.commit_font(next_font, change_set)?; + + Ok(result) + } + + fn layer_change_target( + font: &shift_font::Font, + layer_id: LayerId, + ) -> Result { + let glyph_id = font + .glyph_id_by_layer(layer_id) + .ok_or(CoreError::LayerNotFound(layer_id))?; + let glyph = font + .glyph(glyph_id) + .ok_or(CoreError::GlyphNotFound(glyph_id))?; + let layer = glyph + .layer(layer_id) + .ok_or(CoreError::LayerNotFound(layer_id))?; + + Ok(GlyphLayerChangeTarget { + glyph_id, + glyph_name: glyph.glyph_name().clone(), + source_id: layer.source_id(), + layer_id, + }) + } + + fn set_contour_closed( + &mut self, + layer_id: LayerId, + contour_id: ContourId, + closed: bool, + ) -> Result { + self.commit_edit(|font| { + let target = Self::layer_change_target(font, layer_id)?; + let layer = font + .layer_mut(layer_id) + .ok_or(CoreError::LayerNotFound(layer_id))?; + if closed { + layer.close_contour(contour_id)?; + } else { + layer.open_contour(contour_id)?; + } + let edited_layer = layer.clone(); + let change_set = + FontChange::contour_open_closed_changed(target, contour_id, closed).into(); + + Ok((edited_layer, change_set)) + }) + } + + fn replace_layer_geometry( + &mut self, + layer_id: LayerId, + edit: impl FnOnce(&mut GlyphLayer) -> Result<(), CoreError>, + ) -> Result { + self.replace_layer_geometry_with_result(layer_id, |layer| { + edit(layer)?; + Ok(()) + }) + .map(|(layer, ())| layer) + } + + fn replace_layer_geometry_with_result( + &mut self, + layer_id: LayerId, + edit: impl FnOnce(&mut GlyphLayer) -> Result, + ) -> Result<(GlyphLayer, R), WorkspaceError> { + self.commit_edit(|font| { + let target = Self::layer_change_target(font, layer_id)?; + let layer = font + .layer_mut(layer_id) + .ok_or(CoreError::LayerNotFound(layer_id))?; + let result = edit(layer)?; + let edited_layer = layer.clone(); + let change_set = Self::layer_replaced_change_set(target, layer); + + Ok(((edited_layer, result), change_set)) + }) + } + + fn layer_replaced_change_set( + target: GlyphLayerChangeTarget, + layer: &GlyphLayer, + ) -> FontChangeSet { + FontChange::layer_geometry_replaced(target, layer).into() + } + fn open_package( source_path: impl AsRef, store_path: impl AsRef, diff --git a/crates/shift-workspace/tests/workspace_test.rs b/crates/shift-workspace/tests/workspace_test.rs index 9c02c093..48d6fd67 100644 --- a/crates/shift-workspace/tests/workspace_test.rs +++ b/crates/shift-workspace/tests/workspace_test.rs @@ -1,5 +1,6 @@ use std::path::PathBuf; +use shift_font::{LayerId, error::CoreError}; use shift_workspace::{FontWorkspace, NewWorkspace, WorkspaceError, WorkspaceSource}; #[test] @@ -109,6 +110,43 @@ fn save_as_assigns_a_shift_package_save_target() { workspace.save().unwrap(); } +#[test] +fn set_x_advance_updates_existing_layer() { + let temp = tempfile::tempdir().unwrap(); + let source_path = temp.path().join("TestFont.shift"); + let store_path = temp.path().join("working.sqlite"); + let mut workspace = + FontWorkspace::create(&source_path, &store_path, NewWorkspace::new()).unwrap(); + let source_id = workspace.font().default_source_id().unwrap(); + let glyph = workspace.create_glyph("A".to_string(), vec![65]).unwrap(); + let layer = workspace.create_glyph_layer(glyph.id(), source_id).unwrap(); + let layer_id = layer.id(); + + let edited_layer = workspace.set_x_advance(layer_id, 640.0).unwrap(); + + assert_eq!(edited_layer.width(), 640.0); + assert_eq!(workspace.font().layer(layer_id).unwrap().width(), 640.0); +} + +#[test] +fn set_x_advance_rejects_missing_layer() { + let temp = tempfile::tempdir().unwrap(); + let source_path = temp.path().join("TestFont.shift"); + let store_path = temp.path().join("working.sqlite"); + let mut workspace = + FontWorkspace::create(&source_path, &store_path, NewWorkspace::new()).unwrap(); + let layer_id = LayerId::new(); + + let error = workspace.set_x_advance(layer_id, 640.0).unwrap_err(); + + assert!(matches!( + error, + WorkspaceError::Font(CoreError::LayerNotFound(missing_layer_id)) + if missing_layer_id == layer_id + )); + assert_eq!(workspace.font().glyph_count(), 0); +} + fn fixture(path: &str) -> PathBuf { PathBuf::from(env!("CARGO_MANIFEST_DIR")) .join("../..") diff --git a/packages/types/src/bridge/generated.ts b/packages/types/src/bridge/generated.ts index 1e865097..9ec8c6ea 100644 --- a/packages/types/src/bridge/generated.ts +++ b/packages/types/src/bridge/generated.ts @@ -4,6 +4,8 @@ import type { AnchorId, ComponentId, GuidelineId, + GlyphId, + LayerId, SourceId, } from "../ids"; @@ -33,6 +35,8 @@ export interface BridgeApi { getSources(): Array getPersistedVersion(): number isDirty(): boolean + createGlyph(name: GlyphName, unicodes: Array): GlyphId + createGlyphLayer(glyphId: GlyphId, sourceId: SourceId): LayerId setXAdvance(glyphRef: GlyphLayerRef, width: number): GlyphValueChange translateLayer(glyphRef: GlyphLayerRef, dx: number, dy: number): GlyphValueChange addPoint(glyphRef: GlyphLayerRef, contourId: ContourId, x: number, y: number, pointType: PointType, smooth: boolean): GlyphStructureChange diff --git a/packages/types/src/ids.ts b/packages/types/src/ids.ts index f3e15403..c1b65612 100644 --- a/packages/types/src/ids.ts +++ b/packages/types/src/ids.ts @@ -11,6 +11,7 @@ declare const ContourIdBrand: unique symbol; declare const AnchorIdBrand: unique symbol; declare const ComponentIdBrand: unique symbol; declare const GuidelineIdBrand: unique symbol; +declare const GlyphIdBrand: unique symbol; declare const LayerIdBrand: unique symbol; declare const SourceIdBrand: unique symbol; @@ -46,6 +47,12 @@ export type ComponentId = string & { readonly [ComponentIdBrand]: typeof Compone */ export type GuidelineId = string & { readonly [GuidelineIdBrand]: typeof GuidelineIdBrand }; +/** + * A glyph identifier from Rust. + * Branded string type - can't be confused with names or other IDs. + */ +export type GlyphId = string & { readonly [GlyphIdBrand]: typeof GlyphIdBrand }; + /** * A layer identifier from Rust. * Branded string type - can't be confused with other IDs or plain strings. @@ -98,6 +105,14 @@ export function asGuidelineId(id: string): GuidelineId { return id as GuidelineId; } +/** + * Convert a string ID from Rust to a typed GlyphId. + * Use this when receiving IDs from Rust snapshots. + */ +export function asGlyphId(id: string): GlyphId { + return id as GlyphId; +} + /** * Convert a string ID from Rust to a typed LayerId. * Use this when receiving IDs from Rust snapshots. @@ -154,6 +169,14 @@ export function isValidGuidelineId(id: unknown): id is GuidelineId { return typeof id === "string" && id.length > 0; } +/** + * Type guard to check if a value is a valid GlyphId. + * Useful for runtime validation in debug builds. + */ +export function isValidGlyphId(id: unknown): id is GlyphId { + return typeof id === "string" && id.length > 0; +} + /** * Type guard to check if a value is a valid LayerId. * Useful for runtime validation in debug builds. diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index 7c1c7c52..f1105926 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -9,6 +9,7 @@ export type { AnchorId, ComponentId, GuidelineId, + GlyphId, LayerId, SourceId, } from "./ids"; @@ -18,6 +19,7 @@ export { asAnchorId, asComponentId, asGuidelineId, + asGlyphId, asLayerId, asSourceId, isValidPointId, @@ -25,6 +27,7 @@ export { isValidAnchorId, isValidComponentId, isValidGuidelineId, + isValidGlyphId, isValidLayerId, isValidSourceId, } from "./ids"; diff --git a/scripts/generate-bridge-types.mjs b/scripts/generate-bridge-types.mjs index 04f9e723..d6b2c850 100644 --- a/scripts/generate-bridge-types.mjs +++ b/scripts/generate-bridge-types.mjs @@ -16,6 +16,8 @@ const headerImports = idImportMatch : []; const idTypeNames = new Set([ + "GlyphId", + "LayerId", "PointId", "ContourId", "AnchorId",