Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions web/src/engine/src/keyboard/variableStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,5 @@ export type VariableStore = { [name: string]: string; };
export interface VariableStoreSerializer {
loadStore(keyboardID: string, storeName: string): VariableStore;
saveStore(keyboardID: string, storeName: string, storeMap: VariableStore): void;
findStores(keyboardID: string): VariableStore[];
}
34 changes: 29 additions & 5 deletions web/src/engine/src/main/variableStoreCookieSerializer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,27 +9,51 @@ import { CookieSerializer } from "keyman/engine/dom-utils";
// dynamic property names; they'd have to be known at compile time to facilitate
// strict type checking.
Comment on lines 9 to 10
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment (lines 4-10) is an excuse for the implementation, but doesn't help describe the purpose of the class. It's missing background which doesn't help a reader understand (a) what is this 'transformation' for, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left that comment for now.

class VarStoreSerializer extends CookieSerializer<VariableStore> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So ... we have VarStoreSerializer, VariableStoreSerializer, VariableStoreCookieSerializer, and CookieSerializer<VariableStore>. Yuck! Why do we need 4? Can't we do this all with just one? What benefit are we getting with this complexity and confusion?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up in #15470. It reduces the number of types, although it's still more than one - see PR description.

constructor(keyboardID: string, storeName: string) {
public constructor(keyboardID: string, storeName: string) {
super(`KeymanWeb_${keyboardID}_Option_${storeName}`);
}

load() {
public load(): VariableStore {
return super.load(decodeURIComponent);
}

save(storeMap: VariableStore) {
public save(storeMap: VariableStore) {
super.save(storeMap, encodeURIComponent);
}

/**
* Find all variable stores associated with a given keyboard.
*
* @param {string} keyboardID The keyboard ID whose variable stores are to be found.
*
* @returns An array of VariableStore objects found for the keyboard.
*/
public static findStores(keyboardID: string): VariableStore[] {
const pattern = new RegExp(`^KeymanWeb_${keyboardID}_Option_`);
const matching = CookieSerializer.loadAllMatching<VariableStore>(pattern, decodeURIComponent);
return matching.map(m => m.value);
}
}

export class VariableStoreCookieSerializer implements VariableStoreSerializer {
loadStore(keyboardID: string, storeName: string): VariableStore {
public loadStore(keyboardID: string, storeName: string): VariableStore {
const storeCookieSerializer = new VarStoreSerializer(keyboardID, storeName);
return storeCookieSerializer.load();
}

saveStore(keyboardID: string, storeName: string, storeMap: VariableStore) {
public saveStore(keyboardID: string, storeName: string, storeMap: VariableStore) {
const storeCookieSerializer = new VarStoreSerializer(keyboardID, storeName);
storeCookieSerializer.save(storeMap);
}

/**
* Find all variable stores associated with a given keyboard.
*
* @param {string} keyboardID The keyboard ID whose variable stores are to be found.
*
* @returns An array of VariableStore objects found for the keyboard.
*/
public findStores(keyboardID: string): VariableStore[] {
return VarStoreSerializer.findStores(keyboardID);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
/*
* Keyman is copyright (C) SIL Global. MIT License.
*/
import { assert } from 'chai';
import { VariableStore } from 'keyman/engine/keyboard';
import { VariableStoreCookieSerializer } from 'keyman/engine/main';

describe('VariableStoreCookieSerializer', () => {
describe('saveStore and loadStore', () => {
const keyboardID = 'test-keyboard';
const storeName = 'testStore';
const cookieName = `KeymanWeb_${keyboardID}_Option_${storeName}`;

afterEach(async () => {
document.cookie = `${cookieName}=; max-age=0`;
});

it('should save and load a simple store with string values', () => {
// Arrange
const serializer = new VariableStoreCookieSerializer();
const storeData: VariableStore = {
option1: 'value1',
option2: 'value2',
option3: 'value3'
};

// Act
serializer.saveStore(keyboardID, storeName, storeData);
const loaded = serializer.loadStore(keyboardID, storeName);

// Assert
assert.deepEqual(loaded, storeData, 'loaded store should match saved store');
});

it('should save and load a store with empty values', () => {
// Arrange
const serializer = new VariableStoreCookieSerializer();
const storeData: VariableStore = {
option1: '',
option2: 'value',
option3: ''
};

// Act
serializer.saveStore(keyboardID, storeName, storeData);
const loaded = serializer.loadStore(keyboardID, storeName);

// Assert
assert.deepEqual(loaded, storeData, 'empty string values should be preserved');
});

it('should save and load a store with special characters', () => {
// Arrange
const serializer = new VariableStoreCookieSerializer();
const storeData: VariableStore = {
encoded: 'value:with;special&chars=',
emoji: '⭐'
};

// Act
serializer.saveStore(keyboardID, storeName, storeData);
const loaded = serializer.loadStore(keyboardID, storeName);

// Assert
assert.deepEqual(loaded, storeData, 'special characters should be properly encoded/decoded');
});

it('should handle multiple stores for the same keyboard independently', () => {
// Arrange
const serializer = new VariableStoreCookieSerializer();
const store1Data: VariableStore = { option: 'store1value' };
const store2Data: VariableStore = { option: 'store2value' };

// Act
serializer.saveStore(keyboardID, 'store1', store1Data);
serializer.saveStore(keyboardID, 'store2', store2Data);
const loaded1 = serializer.loadStore(keyboardID, 'store1');
const loaded2 = serializer.loadStore(keyboardID, 'store2');

// Assert
assert.deepEqual(loaded1, store1Data, 'first store should be independent');
assert.deepEqual(loaded2, store2Data, 'second store should be independent');

// Cleanup
document.cookie = `KeymanWeb_${keyboardID}_Option_store1=; max-age=0`;
document.cookie = `KeymanWeb_${keyboardID}_Option_store2=; max-age=0`;
});

it('should return empty object for non-existent store', () => {
// Arrange
const serializer = new VariableStoreCookieSerializer();

// Act
const loaded = serializer.loadStore(keyboardID, 'nonexistent');

// Assert
assert.deepEqual(loaded, {}, 'non-existent store should return empty object');
});
});

describe('findStores', () => {
it('should return an empty array when no stores exist for a keyboardID', () => {

// Arrange
const serializer = new VariableStoreCookieSerializer();
const keyboardID = 'test-keyboard';
const expected: VariableStore[] = [];

// Act
const result = serializer.findStores(keyboardID);

// Assert
assert.deepEqual(result, expected, 'result should be an empty array');
assert.isTrue(Array.isArray(result), 'result should be an array');
assert.strictEqual(result.length, 0, 'result array length should be 0');
});

it('should return store for keyboard', () => {
// Arrange
const serializer = new VariableStoreCookieSerializer();
const storeData1: VariableStore = {
option1: 'value1',
option2: 'value2',
option3: 'value3'
};
serializer.saveStore('test-keyboard', 'storeName', storeData1);
const storeData2: VariableStore = {
settingA: 'A',
settingB: 'B'
};
serializer.saveStore('another-keyboard', 'anotherStore', storeData2);

// Act
const stores = serializer.findStores('test-keyboard');

// Assert
assert.deepEqual(stores, [storeData1], 'stores should match saved store');

// Cleanup
document.cookie = `KeymanWeb_test-keyboard_Option_storeName=; max-age=0`;
});

});
});
7 changes: 6 additions & 1 deletion web/src/test/auto/dom/web-test-runner.config.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -84,11 +84,16 @@ export default {
// Relative, from the containing package.json
files: ['web/build/test/dom/cases/keyboard-storage/**/*.tests.mjs']
},
{
name: 'engine/main',
// Relative, from the containing package.json
files: ['web/build/test/dom/cases/main/**/*.tests.mjs']
},
{
name: 'engine/osk',
// Relative, from the containing package.json
files: ['web/build/test/dom/cases/osk/**/*.tests.mjs']
}
},
],
middleware: [
// Rewrites short-hand paths for test resources, making them fully relative to the repo root.
Expand Down