-
Notifications
You must be signed in to change notification settings - Fork 227
Ensures highlighting of all nodes that are spoken #1349
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -421,6 +421,11 @@ export class SpeechExplorer | |
['dblclick', this.DblClick.bind(this)], | ||
]); | ||
|
||
/** | ||
* Semantic id to subtree map. | ||
*/ | ||
private subtrees: Map<string, Set<string>> = new Map(); | ||
|
||
/** | ||
* @override | ||
*/ | ||
|
@@ -1040,7 +1045,35 @@ export class SpeechExplorer | |
if (!id) { | ||
return [node]; | ||
} | ||
return Array.from(this.node.querySelectorAll(`[data-semantic-id="${id}"]`)); | ||
const parts = Array.from( | ||
this.node.querySelectorAll(`[data-semantic-id="${id}"]`) | ||
) as HTMLElement[]; | ||
const subtree = this.subtree(id, parts); | ||
return [...parts, ...subtree]; | ||
} | ||
|
||
/** | ||
* Retrieve the elements in the semantic subtree that are not in the DOM subtree. | ||
* | ||
* @param {string} id The semantic id of the root node. | ||
* @param {HTMLElement[]} nodes The list of nodes corresponding to that id | ||
* (could be multiple for linebroken ones). | ||
* @returns {HTMLElement[]} The list of nodes external to the DOM trees rooted | ||
* by any of the input nodes. | ||
*/ | ||
private subtree(id: string, nodes: HTMLElement[]): HTMLElement[] { | ||
const sub = this.subtrees.get(id); | ||
const children: Set<string> = new Set(); | ||
for (const node of nodes) { | ||
Array.from(node.querySelectorAll(`[data-semantic-id]`)).forEach((x) => | ||
children.add(x.getAttribute('data-semantic-id')) | ||
); | ||
} | ||
const rest = setdifference(sub, children); | ||
return [...rest].map((child) => { | ||
const node = this.node.querySelector(`[data-semantic-id="${child}"]`); | ||
return node as HTMLElement; | ||
}); | ||
} | ||
|
||
/** | ||
|
@@ -1496,6 +1529,7 @@ export class SpeechExplorer | |
public item: ExplorerMathItem | ||
) { | ||
super(document, pool, null, node); | ||
this.getSubtrees(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we put this off until the expression is focused? No need to do all this work and create all this data if the user never explores the expression. With pages having a lot of expressions, this could save a lot of time. |
||
} | ||
|
||
/** | ||
|
@@ -1730,4 +1764,97 @@ export class SpeechExplorer | |
} | ||
return focus.join(' '); | ||
} | ||
|
||
/** | ||
* Populates the subtrees map from the data-semantic-structure attribute. | ||
*/ | ||
private getSubtrees() { | ||
const node = this.node.querySelector('[data-semantic-structure]'); | ||
if (!node) return; | ||
const sexp = node.getAttribute('data-semantic-structure'); | ||
const tokens = tokenize(sexp); | ||
const tree = parse(tokens); | ||
buildMap(tree, this.subtrees); | ||
} | ||
} | ||
|
||
// Some Aux functions for parsing the semantic structure sexpression | ||
// | ||
type SexpTree = string | SexpTree[]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should types all be at the top? |
||
|
||
/** | ||
* Helper to tokenize input | ||
* | ||
* @param str The semantic structure. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we no longer need types in the Similarly, do we need |
||
*/ | ||
function tokenize(str: string): string[] { | ||
return str.replace(/\(/g, ' ( ').replace(/\)/g, ' ) ').trim().split(/\s+/); | ||
} | ||
|
||
/** | ||
* Recursive parser to convert tokens into a tree | ||
* | ||
* @param tokens The tokens from the semantic structure. | ||
*/ | ||
function parse(tokens: string[]): SexpTree { | ||
if (!tokens.length) return null; | ||
|
||
const token = tokens.shift(); | ||
|
||
if (token === '(') { | ||
const node = []; | ||
while (tokens[0] !== ')') { | ||
node.push(parse(tokens)); | ||
} | ||
tokens.shift(); // remove ')' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be more efficient to use an index into the tokens array rather than constantly changing the array? I'm not sure what the performance is of shifting arrays, but it seems unnecessary to modify the token list when you are basically just marching through it. |
||
return node; | ||
} else { | ||
return token; | ||
} | ||
} | ||
|
||
/** | ||
* Flattens the tree and builds the map. | ||
* | ||
* @param tree The sexpression tree. | ||
* @param map The map to populate. | ||
*/ | ||
function buildMap(tree: SexpTree, map = new Map()) { | ||
if (typeof tree === 'string') { | ||
if (!map.has(tree)) map.set(tree, new Set()); | ||
return new Set(); | ||
} | ||
|
||
const [root, ...children] = tree; | ||
const rootId = root; | ||
Comment on lines
+1828
to
+1829
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why the need for two variables |
||
const descendants = new Set(); | ||
|
||
for (const child of children) { | ||
const childRoot = typeof child === 'string' ? child : child[0]; | ||
if (!map.has(rootId)) map.set(rootId, new Set()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this line be moved to before line 1832, outside the loop? If it should only be set of there are children, can't we test for that once before the loop rather than testing this for every child? And doesn't line 1841 overwrite this anyway? |
||
|
||
const childDescendants = buildMap(child, map); | ||
descendants.add(childRoot); | ||
childDescendants.forEach((d) => descendants.add(d)); | ||
} | ||
|
||
map.set(rootId, descendants); | ||
return descendants; | ||
} | ||
|
||
// Can be replaced with ES2024 implementation of Set.prototyp.difference | ||
/** | ||
* Set difference between two sets A and B: A\B. | ||
* | ||
* @param a Initial set. | ||
* @param b Set to remove from A. | ||
*/ | ||
function setdifference(a: Set<string>, b: Set<string>): Set<string> { | ||
if (!a) { | ||
return new Set(); | ||
} | ||
if (!b) { | ||
return a; | ||
} | ||
return new Set([...a].filter((x) => !b.has(x))); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since
getSplitNodes()
is called every time you move to a new element in the explorer (twice -- once for the element you move from and once for the one you move to), and since the subtree computation has added more complexity, here, I'm wondering if it would be useful to cache the results (they won't change unless anmaction
element is toggled, and then the key explorer is recreated anyway), along with (or in place of?) the subtree ids?