From ef9bf13057f7e12d5501c84aa766f582edfc6d76 Mon Sep 17 00:00:00 2001 From: Just van Rossum Date: Tue, 12 Nov 2024 17:57:46 +0100 Subject: [PATCH 1/8] Prepare to make sourcesByLocationString also contain sparse sources --- src/fontra/client/core/font-sources-instancer.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/fontra/client/core/font-sources-instancer.js b/src/fontra/client/core/font-sources-instancer.js index 6cd78db9f..273b5a57c 100644 --- a/src/fontra/client/core/font-sources-instancer.js +++ b/src/fontra/client/core/font-sources-instancer.js @@ -64,11 +64,12 @@ export class FontSourcesInstancer { sourceLocation = { ...this.defaultLocation, ...sourceLocation }; const locationString = locationToString(sourceLocation); - if (locationString in this.sourcesByLocationString) { + let sourceInstance = this.sourcesByLocationString[locationString]; + if (sourceInstance && !sourceInstance.isSparse) { return this.sourcesByLocationString[locationString]; } - let sourceInstance = this._instanceCache.get(locationString); + sourceInstance = this._instanceCache.get(locationString); if (!sourceInstance) { const deltas = this.deltas; From ea3285423fafaca21e09a95a50f3a411d76e869a Mon Sep 17 00:00:00 2001 From: Just van Rossum Date: Tue, 12 Nov 2024 17:59:44 +0100 Subject: [PATCH 2/8] Make some properties private --- .../client/core/font-sources-instancer.js | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/fontra/client/core/font-sources-instancer.js b/src/fontra/client/core/font-sources-instancer.js index 273b5a57c..052d2579f 100644 --- a/src/fontra/client/core/font-sources-instancer.js +++ b/src/fontra/client/core/font-sources-instancer.js @@ -15,15 +15,15 @@ export class FontSourcesInstancer { } _setup() { - this.fontSourcesList = Object.values(this.fontSources).filter( + this._fontSourcesList = Object.values(this.fontSources).filter( (source) => !source.isSparse ); this.fontAxesSourceSpace = mapAxesFromUserSpaceToSourceSpace(this.fontAxes); this.defaultLocation = Object.fromEntries( this.fontAxesSourceSpace.map((axis) => [axis.name, axis.defaultValue]) ); - this.sourcesByLocationString = Object.fromEntries( - this.fontSourcesList.map((source) => [ + this._sourcesByLocationString = Object.fromEntries( + this._fontSourcesList.map((source) => [ locationToString({ ...this.defaultLocation, ...source.location }), source, ]) @@ -33,17 +33,17 @@ export class FontSourcesInstancer { get model() { if (!this._model) { - const locations = this.fontSourcesList.map((source) => source.location); + const locations = this._fontSourcesList.map((source) => source.location); this._model = new DiscreteVariationModel(locations, this.fontAxesSourceSpace); } return this._model; } get deltas() { - const guidelinesAreCompatible = areGuidelinesCompatible(this.fontSourcesList); - const customDatasAreCompatible = areCustomDatasCompatible(this.fontSourcesList); + const guidelinesAreCompatible = areGuidelinesCompatible(this._fontSourcesList); + const customDatasAreCompatible = areCustomDatasCompatible(this._fontSourcesList); - const fixedSourceValues = this.fontSourcesList.map((source) => { + const fixedSourceValues = this._fontSourcesList.map((source) => { return { ...source, location: null, @@ -58,15 +58,15 @@ export class FontSourcesInstancer { } instantiate(sourceLocation) { - if (!this.fontSourcesList.length) { + if (!this._fontSourcesList.length) { return undefined; } sourceLocation = { ...this.defaultLocation, ...sourceLocation }; const locationString = locationToString(sourceLocation); - let sourceInstance = this.sourcesByLocationString[locationString]; + let sourceInstance = this._sourcesByLocationString[locationString]; if (sourceInstance && !sourceInstance.isSparse) { - return this.sourcesByLocationString[locationString]; + return this._sourcesByLocationString[locationString]; } sourceInstance = this._instanceCache.get(locationString); From 5ef43f961413bf36454f525939cd5c8c97131921 Mon Sep 17 00:00:00 2001 From: Just van Rossum Date: Tue, 12 Nov 2024 18:10:39 +0100 Subject: [PATCH 3/8] Map to source idenitifier instead of source --- src/fontra/client/core/font-sources-instancer.js | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/fontra/client/core/font-sources-instancer.js b/src/fontra/client/core/font-sources-instancer.js index 052d2579f..a925c4a06 100644 --- a/src/fontra/client/core/font-sources-instancer.js +++ b/src/fontra/client/core/font-sources-instancer.js @@ -22,10 +22,10 @@ export class FontSourcesInstancer { this.defaultLocation = Object.fromEntries( this.fontAxesSourceSpace.map((axis) => [axis.name, axis.defaultValue]) ); - this._sourcesByLocationString = Object.fromEntries( - this._fontSourcesList.map((source) => [ + this._sourceIdentifiersByLocationString = Object.fromEntries( + Object.entries(this.fontSources).map(([sourceIdentifier, source]) => [ locationToString({ ...this.defaultLocation, ...source.location }), - source, + sourceIdentifier, ]) ); this._instanceCache = new LRUCache(50); @@ -64,9 +64,13 @@ export class FontSourcesInstancer { sourceLocation = { ...this.defaultLocation, ...sourceLocation }; const locationString = locationToString(sourceLocation); - let sourceInstance = this._sourcesByLocationString[locationString]; + const sourceIdentifier = this._sourceIdentifiersByLocationString[locationString]; + let sourceInstance = sourceIdentifier + ? this.fontSources[sourceIdentifier] + : undefined; + if (sourceInstance && !sourceInstance.isSparse) { - return this._sourcesByLocationString[locationString]; + return sourceInstance; } sourceInstance = this._instanceCache.get(locationString); From 6f6e1678a105e772ebebbc3b6d53c82fcb4dbbcd Mon Sep 17 00:00:00 2001 From: Just van Rossum Date: Tue, 12 Nov 2024 18:26:43 +0100 Subject: [PATCH 4/8] Add defaultSourceIdentifier property --- src/fontra/client/core/font-sources-instancer.js | 3 +++ test-js/test-font-sources-instancer.js | 8 ++++++++ 2 files changed, 11 insertions(+) diff --git a/src/fontra/client/core/font-sources-instancer.js b/src/fontra/client/core/font-sources-instancer.js index a925c4a06..e16293a2f 100644 --- a/src/fontra/client/core/font-sources-instancer.js +++ b/src/fontra/client/core/font-sources-instancer.js @@ -28,6 +28,9 @@ export class FontSourcesInstancer { sourceIdentifier, ]) ); + this.defaultSourceIdentifier = + this._sourceIdentifiersByLocationString[locationToString(this.defaultLocation)]; + this._instanceCache = new LRUCache(50); } diff --git a/test-js/test-font-sources-instancer.js b/test-js/test-font-sources-instancer.js index d14c492a8..7530c00c8 100644 --- a/test-js/test-font-sources-instancer.js +++ b/test-js/test-font-sources-instancer.js @@ -109,9 +109,17 @@ describe("FontSourcesInstancer Tests", () => { expect(sourceInstance).to.deep.equal(testItem.expectedSource); }); + it("Default location identifier", () => { + const fsi = new FontSourcesInstancer(testAxes, testSources); + expect(fsi.defaultSourceIdentifier).to.equal("source1"); + expect(fsi.defaultLocation).to.deep.equal({ Weight: 400, Width: 50 }); + }); + it("Empty sources list", () => { const fsi = new FontSourcesInstancer([], {}); const sourceInstance = fsi.instantiate({}); expect(sourceInstance).to.deep.equal(undefined); + expect(fsi.defaultSourceIdentifier).to.equal(undefined); + expect(fsi.defaultLocation).to.deep.equal({}); }); }); From 3e44d7f25094de33cc98528f035e9874f8f967f4 Mon Sep 17 00:00:00 2001 From: Just van Rossum Date: Tue, 12 Nov 2024 19:44:53 +0100 Subject: [PATCH 5/8] Add getLocationIdentifierForLocation() method --- .../client/core/font-sources-instancer.js | 5 +++++ test-js/test-font-sources-instancer.js | 22 +++++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/src/fontra/client/core/font-sources-instancer.js b/src/fontra/client/core/font-sources-instancer.js index e16293a2f..16f6896fc 100644 --- a/src/fontra/client/core/font-sources-instancer.js +++ b/src/fontra/client/core/font-sources-instancer.js @@ -34,6 +34,11 @@ export class FontSourcesInstancer { this._instanceCache = new LRUCache(50); } + getLocationIdentifierForLocation(location) { + location = { ...this.defaultLocation, ...location }; + return this._sourceIdentifiersByLocationString[locationToString(location)]; + } + get model() { if (!this._model) { const locations = this._fontSourcesList.map((source) => source.location); diff --git a/test-js/test-font-sources-instancer.js b/test-js/test-font-sources-instancer.js index 7530c00c8..ab49d3ab0 100644 --- a/test-js/test-font-sources-instancer.js +++ b/test-js/test-font-sources-instancer.js @@ -115,11 +115,33 @@ describe("FontSourcesInstancer Tests", () => { expect(fsi.defaultLocation).to.deep.equal({ Weight: 400, Width: 50 }); }); + parametrize( + "FontSourcesInstancer.getLocationIdentifierForLocation", + [ + { location: {}, locationIdentifier: "source1" }, + { location: { Weight: 400 }, locationIdentifier: "source1" }, + { location: { Width: 50 }, locationIdentifier: "source1" }, + { location: { Weight: 400, Width: 50 }, locationIdentifier: "source1" }, + { location: { Weight: 900 }, locationIdentifier: "source2" }, + { location: { Weight: 900, Width: 50 }, locationIdentifier: "source2" }, + { location: { Width: 100 }, locationIdentifier: "source3" }, + { location: { Weight: 900, Width: 100 }, locationIdentifier: "source4" }, + { location: { Weight: 800, Width: 100 }, locationIdentifier: undefined }, + ], + (testItem) => { + const fsi = new FontSourcesInstancer(testAxes, testSources); + expect(fsi.getLocationIdentifierForLocation(testItem.location)).to.equal( + testItem.locationIdentifier + ); + } + ); + it("Empty sources list", () => { const fsi = new FontSourcesInstancer([], {}); const sourceInstance = fsi.instantiate({}); expect(sourceInstance).to.deep.equal(undefined); expect(fsi.defaultSourceIdentifier).to.equal(undefined); expect(fsi.defaultLocation).to.deep.equal({}); + expect(fsi.getLocationIdentifierForLocation({})).to.equal(undefined); }); }); From b4bace8a23f6feeca7dd11bf7fb8851092845a36 Mon Sep 17 00:00:00 2001 From: Just van Rossum Date: Tue, 12 Nov 2024 19:46:04 +0100 Subject: [PATCH 6/8] Add test case for unknown axis --- test-js/test-font-sources-instancer.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test-js/test-font-sources-instancer.js b/test-js/test-font-sources-instancer.js index ab49d3ab0..407017972 100644 --- a/test-js/test-font-sources-instancer.js +++ b/test-js/test-font-sources-instancer.js @@ -127,6 +127,10 @@ describe("FontSourcesInstancer Tests", () => { { location: { Width: 100 }, locationIdentifier: "source3" }, { location: { Weight: 900, Width: 100 }, locationIdentifier: "source4" }, { location: { Weight: 800, Width: 100 }, locationIdentifier: undefined }, + { + location: { Weight: 900, Width: 100, UnknownAxis: 120 }, + locationIdentifier: undefined, + }, ], (testItem) => { const fsi = new FontSourcesInstancer(testAxes, testSources); From 9fe7d5f760981d9f3d7095392da8d27c34f1a1ac Mon Sep 17 00:00:00 2001 From: Just van Rossum Date: Tue, 12 Nov 2024 19:49:02 +0100 Subject: [PATCH 7/8] Shorten internal property --- src/fontra/client/core/font-sources-instancer.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/fontra/client/core/font-sources-instancer.js b/src/fontra/client/core/font-sources-instancer.js index 16f6896fc..e6a07236c 100644 --- a/src/fontra/client/core/font-sources-instancer.js +++ b/src/fontra/client/core/font-sources-instancer.js @@ -22,21 +22,21 @@ export class FontSourcesInstancer { this.defaultLocation = Object.fromEntries( this.fontAxesSourceSpace.map((axis) => [axis.name, axis.defaultValue]) ); - this._sourceIdentifiersByLocationString = Object.fromEntries( + this._sourceIdsByLocationString = Object.fromEntries( Object.entries(this.fontSources).map(([sourceIdentifier, source]) => [ locationToString({ ...this.defaultLocation, ...source.location }), sourceIdentifier, ]) ); this.defaultSourceIdentifier = - this._sourceIdentifiersByLocationString[locationToString(this.defaultLocation)]; + this._sourceIdsByLocationString[locationToString(this.defaultLocation)]; this._instanceCache = new LRUCache(50); } getLocationIdentifierForLocation(location) { location = { ...this.defaultLocation, ...location }; - return this._sourceIdentifiersByLocationString[locationToString(location)]; + return this._sourceIdsByLocationString[locationToString(location)]; } get model() { @@ -72,7 +72,7 @@ export class FontSourcesInstancer { sourceLocation = { ...this.defaultLocation, ...sourceLocation }; const locationString = locationToString(sourceLocation); - const sourceIdentifier = this._sourceIdentifiersByLocationString[locationString]; + const sourceIdentifier = this._sourceIdsByLocationString[locationString]; let sourceInstance = sourceIdentifier ? this.fontSources[sourceIdentifier] : undefined; From 6dd8e032ffc40ac8767dece8532a49124482d5bf Mon Sep 17 00:00:00 2001 From: Just van Rossum Date: Tue, 12 Nov 2024 19:58:17 +0100 Subject: [PATCH 8/8] Rename property to be more explicit --- src/fontra/client/core/font-sources-instancer.js | 10 +++++----- test-js/test-font-sources-instancer.js | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/fontra/client/core/font-sources-instancer.js b/src/fontra/client/core/font-sources-instancer.js index e6a07236c..70607d521 100644 --- a/src/fontra/client/core/font-sources-instancer.js +++ b/src/fontra/client/core/font-sources-instancer.js @@ -19,23 +19,23 @@ export class FontSourcesInstancer { (source) => !source.isSparse ); this.fontAxesSourceSpace = mapAxesFromUserSpaceToSourceSpace(this.fontAxes); - this.defaultLocation = Object.fromEntries( + this.defaultSourceLocation = Object.fromEntries( this.fontAxesSourceSpace.map((axis) => [axis.name, axis.defaultValue]) ); this._sourceIdsByLocationString = Object.fromEntries( Object.entries(this.fontSources).map(([sourceIdentifier, source]) => [ - locationToString({ ...this.defaultLocation, ...source.location }), + locationToString({ ...this.defaultSourceLocation, ...source.location }), sourceIdentifier, ]) ); this.defaultSourceIdentifier = - this._sourceIdsByLocationString[locationToString(this.defaultLocation)]; + this._sourceIdsByLocationString[locationToString(this.defaultSourceLocation)]; this._instanceCache = new LRUCache(50); } getLocationIdentifierForLocation(location) { - location = { ...this.defaultLocation, ...location }; + location = { ...this.defaultSourceLocation, ...location }; return this._sourceIdsByLocationString[locationToString(location)]; } @@ -69,7 +69,7 @@ export class FontSourcesInstancer { if (!this._fontSourcesList.length) { return undefined; } - sourceLocation = { ...this.defaultLocation, ...sourceLocation }; + sourceLocation = { ...this.defaultSourceLocation, ...sourceLocation }; const locationString = locationToString(sourceLocation); const sourceIdentifier = this._sourceIdsByLocationString[locationString]; diff --git a/test-js/test-font-sources-instancer.js b/test-js/test-font-sources-instancer.js index 407017972..6e22214a8 100644 --- a/test-js/test-font-sources-instancer.js +++ b/test-js/test-font-sources-instancer.js @@ -112,7 +112,7 @@ describe("FontSourcesInstancer Tests", () => { it("Default location identifier", () => { const fsi = new FontSourcesInstancer(testAxes, testSources); expect(fsi.defaultSourceIdentifier).to.equal("source1"); - expect(fsi.defaultLocation).to.deep.equal({ Weight: 400, Width: 50 }); + expect(fsi.defaultSourceLocation).to.deep.equal({ Weight: 400, Width: 50 }); }); parametrize( @@ -145,7 +145,7 @@ describe("FontSourcesInstancer Tests", () => { const sourceInstance = fsi.instantiate({}); expect(sourceInstance).to.deep.equal(undefined); expect(fsi.defaultSourceIdentifier).to.equal(undefined); - expect(fsi.defaultLocation).to.deep.equal({}); + expect(fsi.defaultSourceLocation).to.deep.equal({}); expect(fsi.getLocationIdentifierForLocation({})).to.equal(undefined); }); });