From d05ff2089ca2e5e3b62710b95ac4a9cd1275e476 Mon Sep 17 00:00:00 2001 From: Petr Pucil Date: Thu, 23 Nov 2023 21:59:50 +0100 Subject: [PATCH 1/2] Bump jstree from 3.3.14 to 3.3.16 --- package-lock.json | 10 +++++----- package.json | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/package-lock.json b/package-lock.json index b3c2bf52..927ab698 100644 --- a/package-lock.json +++ b/package-lock.json @@ -18,7 +18,7 @@ "golden-layout": "^1.5.9", "jquery": "^3.5.0", "js-yaml": "^4.1.0", - "jstree": "^3.3.4", + "jstree": "^3.3.16", "kaitai-struct": "next", "kaitai-struct-compiler": "next", "localforage": "^1.5.0", @@ -755,11 +755,11 @@ "integrity": "sha512-8+9WqebbFzpX9OR+Wa6O29asIogeRMzcGtAINdpMHHyAg10f05aSFVBbcEqGf/PXw1EjAZ+q2/bEBg3DvurK3Q==" }, "node_modules/jstree": { - "version": "3.3.14", - "resolved": "https://registry.npmjs.org/jstree/-/jstree-3.3.14.tgz", - "integrity": "sha512-W8t+nFOKENXNIulvu+DW4gPcnpOXY0FswiTiOn1Fnhs6poRe6eA/Kf6fS1/GJJ8C8KEy0q3ttF6tbGRDmHIM/g==", + "version": "3.3.16", + "resolved": "https://registry.npmjs.org/jstree/-/jstree-3.3.16.tgz", + "integrity": "sha512-yeeIJffi2WAqyMeHufXj/Ozy7GqgKdDkxfN8L8lwbG0h1cw/TgDafWmyhroH4AKgDSk9yW1W6jiJZu4zXAqzXw==", "dependencies": { - "jquery": "^3.6.0" + "jquery": "^3.5.0" } }, "node_modules/kaitai-struct": { diff --git a/package.json b/package.json index 677d671a..74974386 100644 --- a/package.json +++ b/package.json @@ -15,7 +15,7 @@ "golden-layout": "^1.5.9", "jquery": "^3.5.0", "js-yaml": "^4.1.0", - "jstree": "^3.3.4", + "jstree": "^3.3.16", "kaitai-struct": "next", "kaitai-struct-compiler": "next", "localforage": "^1.5.0", From dcfdca6f6a21532253e4f3898e966018de322713 Mon Sep 17 00:00:00 2001 From: Petr Pucil Date: Wed, 6 Dec 2023 21:11:15 +0100 Subject: [PATCH 2/2] Use jsTree's state plugin to persist object tree state ... instead of a poor custom reimplementation of the same. This change fixes several bugs of the existing implementation and improves the overall user experience when interacting with the object tree: 1. It fixes one annoying bug I've encountered many times: if you had expanded a certain node in the tree and then changed the .ksy spec so that a node under the same path still existed in the tree but was no longer openable (in our case, only non-empty arrays and user type objects are openable, so either it has become empty or its type has changed to a primitive type, for example), then our naive code would still try to open it and asynchronously wait for it to be opened, which wouldn't happen. In that case, the code was stuck on an unfulfilled promise at every reparsing (because the promise fulfillment happened only after all required nodes were opened), which in practice had several implications: - selecting a node in the object tree didn't highlight the corresponding interval in the hex dump, - opening or closing nodes in the tree was abrupt (without a smooth transition as usual), - changes to the set of opened nodes weren't persisted to the local storage, - an open "Errors" pane didn't disappear, even if the error has been already fixed. So this change also fixes https://github.com/kaitai-io/kaitai_struct_webide/issues/61 - this is an issue which reported this exact problem, but was closed because nobody knew how to reproduce it. The remark in https://github.com/kaitai-io/kaitai_struct_webide/issues/61#issuecomment-365416726 by @DarkShadow44 confirms my suspicions that it is the same thing: > After I cleared the local storage data, I can't reproduce it > either. Basically the only way I know to resolve the promise was to open a few random nodes in the tree (or pick one and repeatedly close and open it again). The opening code would (mis)interpret these events as if the nodes it requested to open were finally opened, and then resolve the promise. But you had to do this after every reparsing, which was annoying. 2. Thanks to the default behavior of the jsTree's state plugin, the scroll position and the selected node are now persisted as well. Previously, only the set of opened nodes and the selection range in the hex viewer was preserved (the saved hex viewer selection sometimes resulted to selecting the right node and scrolling to it, but this indirect approach is not as good as actually saving the scroll offset and the selected node). Currently, the events that trigger the saving of the tree state are left at default `changed.jstree open_node.jstree close_node.jstree` (see https://www.jstree.com/api/#/?f=$.jstree.defaults.state.events), meaning that the state is only saved when opening, closing or selecting a node. This means that if you only scroll in the jsTree and don't interact with the tree after that, the new scroll position won't be persisted - but this is just a minor inconvenience. We can further improve this in the future. 3. A tree node is now selected as soon as it's focused. This in practice means that a node is selected upon "mousedown" (i.e. when the button is pressed, but not necessarily released yet), not only after a full cycle of pressing and releasing the mouse button. Likewise, when navigating in the tree using the keyboard (for example using the arrow keys), a new node is selected already on "keydown", not "keyup". 4. Both the first time after page load (after restoring the selected node from local storage) and after clicking on an interval in the hex dump, the jsTree's stored "active node" is updated to the selected node. This ensures that when the object tree pane regains focus (and no node has been moused over since the event that changed the selected node), jsTree will focus the selected node. A notable difference in state persistence is that the old version kept paths of all nodes ever opened in the local storage, whereas the jsTree's state plugin always rewrites the local storage with the *current* state. This means that if you have some nodes open and switch to a different spec, the information about the open nodes from the original spec is lost unless the open nodes' paths also exist in the new spec. In the previous version, all open nodes would survive switching between specs like that. This might not be ideal, but it can be fixed in the future simply by storing states for different specs under different keys (see https://www.jstree.com/api/#/?f=$.jstree.defaults.state.key). --- lib/ts-types/jstree.d.ts | 5 +- src/v1/app.ts | 34 +++---- src/v1/parsedToTree.ts | 198 ++++++++++++++++++--------------------- 3 files changed, 113 insertions(+), 124 deletions(-) diff --git a/lib/ts-types/jstree.d.ts b/lib/ts-types/jstree.d.ts index 2c9ed96e..6ee17c99 100644 --- a/lib/ts-types/jstree.d.ts +++ b/lib/ts-types/jstree.d.ts @@ -1,4 +1,4 @@ -// Type definitions for jsTree v3.3.2 +// Type definitions for jsTree v3.3.2 // Project: http://www.jstree.com/ // Definitions by: Adam Pluciński // Definitions: https://github.com/DefinitelyTyped/DefinitelyTyped @@ -814,7 +814,8 @@ interface JSTree extends JQuery { * @param {Boolean} as_dom * @return {Object|jQuery} */ - get_node: (obj: any, as_dom?: boolean) => any; + get_node(obj: any, as_dom?: false): Record; + get_node(obj: any, as_dom: true): JQuery; /** * get the path to a node, either consisting of node texts, or of node IDs, optionally glued together (otherwise an array) diff --git a/src/v1/app.ts b/src/v1/app.ts index a24122e7..d55cf531 100644 --- a/src/v1/app.ts +++ b/src/v1/app.ts @@ -123,23 +123,23 @@ class AppController { //console.log("reparse exportedRoot", exportedRoot); this.ui.parsedDataTreeHandler = new ParsedTreeHandler(this.ui.parsedDataTreeCont.getElement(), exportedRoot, this.compilerService.ksyTypes); - await this.ui.parsedDataTreeHandler.initNodeReopenHandling(); - this.ui.hexViewer.onSelectionChanged(); - - this.ui.parsedDataTreeHandler.jstree.on("select_node.jstree", (e, selectNodeArgs) => { - var node = selectNodeArgs.node; - //console.log("node", node); - var exp = this.ui.parsedDataTreeHandler.getNodeData(node).exported; - - if (exp && exp.path) - $("#parsedPath").text(exp.path.join("/")); - - if (!this.blockRecursive && exp && exp.start < exp.end) { - this.selectedInTree = true; - //console.log("setSelection", exp.ioOffset, exp.start); - this.ui.hexViewer.setSelection(exp.ioOffset + exp.start, exp.ioOffset + exp.end - 1); - this.selectedInTree = false; - } + + this.ui.parsedDataTreeHandler.jstree.on("state_ready.jstree", () => { + this.ui.parsedDataTreeHandler.jstree.on("select_node.jstree", (e, selectNodeArgs) => { + var node = selectNodeArgs.node; + //console.log("node", node); + var exp = this.ui.parsedDataTreeHandler.getNodeData(node).exported; + + if (exp && exp.path) + $("#parsedPath").text(exp.path.join("/")); + + if (!this.blockRecursive && exp && exp.start < exp.end) { + this.selectedInTree = true; + //console.log("setSelection", exp.ioOffset, exp.start); + this.ui.hexViewer.setSelection(exp.ioOffset + exp.start, exp.ioOffset + exp.end - 1); + this.selectedInTree = false; + } + }); }); this.errors.handle(null); diff --git a/src/v1/parsedToTree.ts b/src/v1/parsedToTree.ts index bb82887d..b61f3d79 100644 --- a/src/v1/parsedToTree.ts +++ b/src/v1/parsedToTree.ts @@ -34,45 +34,80 @@ export class ParsedTreeHandler { this.jstree = jsTreeElement.jstree({ core: { data: (node: IParsedTreeNode, cb: any) => - this.getNode(node).then(x => cb(x), e => app.errors.handle(e)), themes: { icons: false }, multiple: false, force_text: false - } + this.getNode(node).then(x => cb(x), e => app.errors.handle(e)), + themes: { icons: false }, + multiple: false, + force_text: false, + allow_reselect: true, + loaded_state: true, + }, + plugins: [ "state" ], + state: { + preserve_loaded: true, + filter: function (state: Record) { + const openNodes: string[] = state.core.open; + const nodesToLoad = new Set(); + for (const path of openNodes) { + const pathParts = path.split("-"); + if (pathParts[0] !== "inputField") { + continue; + } + let subPath = pathParts.shift(); + for (const part of pathParts) { + subPath += "-" + part; + if (this.is_loaded(subPath)) { + continue; + } + nodesToLoad.add(subPath); + } + } + // If we want to preserve the open state of nodes with closed parents, + // we must at least load their parents so that such nodes appear + // in the internal list of nodes that jsTree knows and the jsTree + // 'state' plugin can mark them as open. + state.core.loaded = Array.from(nodesToLoad); + return state; + }, + }, }).jstree(true); - this.jstree.on = (...args: any[]) => (this.jstree).element.on(...args); - this.jstree.off = (...args: any[]) => (this.jstree).element.off(...args); - this.jstree.on("keyup.jstree", e => this.jstree.activate_node(e.target.id, null)); + this.jstree.on = (...args: any[]) => (this.jstree).get_container().on(...args); + this.jstree.off = (...args: any[]) => (this.jstree).get_container().off(...args); + this.jstree.on("state_ready.jstree", () => { + // These settings have been set to `true` only temporarily so that our + // approach of populating the `state.core.loaded` property in the + // `state.filter` function takes effect. + this.jstree.settings.state.preserve_loaded = false; + this.jstree.settings.core.loaded_state = false; + + this.updateActiveJstreeNode(); + }); + this.jstree.on("focus.jstree", ".jstree-anchor", e => { + const focusedNode = e.currentTarget; + if (!this.jstree.is_selected(focusedNode)) { + this.jstree.deselect_all(true); + this.jstree.select_node(focusedNode); + } + }); this.intervalHandler = new IntervalHandler(); } - private parsedTreeOpenedNodes: { [id: string]: boolean } = {}; - private saveOpenedNodesDisabled = false; - - private saveOpenedNodes() { - if (this.saveOpenedNodesDisabled) return; - localStorage.setItem("parsedTreeOpenedNodes", Object.keys(this.parsedTreeOpenedNodes).join(",")); - } - - public initNodeReopenHandling() { - var parsedTreeOpenedNodesStr = localStorage.getItem("parsedTreeOpenedNodes"); - if (parsedTreeOpenedNodesStr) - parsedTreeOpenedNodesStr.split(",").forEach(x => this.parsedTreeOpenedNodes[x] = true); - - return new Promise((resolve, reject) => { - this.jstree.on("ready.jstree", _ => { - this.openNodes(Object.keys(this.parsedTreeOpenedNodes)).then(() => { - this.jstree.on("open_node.jstree", (e, te) => { - var node = te.node; - this.parsedTreeOpenedNodes[this.getNodeId(node)] = true; - this.saveOpenedNodes(); - }).on("close_node.jstree", (e, te) => { - var node = te.node; - delete this.parsedTreeOpenedNodes[this.getNodeId(node)]; - this.saveOpenedNodes(); - }); - - resolve(); - }, err => reject(err)); - }); - }); + updateActiveJstreeNode(): void { + const selectedNode = this.jstree.get_selected()[0]; + if (!selectedNode) { + return; + } + // This ensures that next time the jsTree is focused (even when clicking + // somewhere in the empty space of the jsTree pane without clicking or + // hovering over any particular node first), the selected node (if any) + // will be focused. If we don't do this, jsTree will instead focus the + // most recently node that the user directly interacted with or (upon + // page load) the very first node of the entire tree, which is not + // ideal. + // + // As of jsTree 3.3.16, jsTree uses the `aria-activedescendant` + // attribute as the only means of persisting the active node, so we + // don't have much choice how to implement this. + this.jstree.get_container().attr('aria-activedescendant', selectedNode); } primitiveToText(exported: IExportedValue, detailed: boolean = true): string { @@ -335,83 +370,36 @@ export class ParsedTreeHandler { var path = nodeData.exported ? nodeData.exported.path : nodeData.instance.path; if (nodeData.arrayStart || nodeData.arrayEnd) path = path.concat([`${nodeData.arrayStart || 0}`, `${nodeData.arrayEnd || 0}`]); - return "inputField_" + path.join("_"); + return ["inputField", ...path].join("-"); } - openNodes(nodesToOpen: string[]): Promise { - return new Promise((resolve, reject) => { - this.saveOpenedNodesDisabled = true; - var origAnim = (this.jstree).settings.core.animation; - (this.jstree).settings.core.animation = 0; - //console.log("saveOpenedNodesDisabled = true"); - - var openCallCounter = 1; - var openRound = (e: any) => { - openCallCounter--; - //console.log("openRound", openCallCounter, nodesToOpen); - - var newNodesToOpen: string[] = []; - var existingNodes: string[] = []; - nodesToOpen.forEach(nodeId => { - var node = this.jstree.get_node(nodeId); - if (node) { - if (!node.state.opened) - existingNodes.push(node); - } else - newNodesToOpen.push(nodeId); - }); - nodesToOpen = newNodesToOpen; - - //console.log("existingNodes", existingNodes, "openCallCounter", openCallCounter); - - if (existingNodes.length > 0) - existingNodes.forEach(node => { - openCallCounter++; - //console.log(`open_node called on ${node.id}`) - this.jstree.open_node(node); - }); - else if (openCallCounter === 0) { - //console.log("saveOpenedNodesDisabled = false"); - this.saveOpenedNodesDisabled = false; - if (e) - this.jstree.off(e); - (this.jstree).settings.core.animation = origAnim; - this.saveOpenedNodes(); - - resolve(nodesToOpen.length === 0); - } - }; - - this.jstree.on("open_node.jstree", e => openRound(e)); - openRound(null); - }); - } + activatePath(path: string|string[]): Promise { + const pathParts = typeof path === "string" ? path.split("/") : path; - activatePath(path: string|string[]): Promise { - var pathParts = typeof path === "string" ? path.split("/") : path; - - var expandNodes = []; - var pathStr = "inputField"; - for (var i = 0; i < pathParts.length; i++) { - pathStr += "_" + pathParts[i]; - expandNodes.push(pathStr); + const nodesToLoad: string[] = []; + let pathStr = "inputField"; + for (let i = 0; i < pathParts.length; i++) { + pathStr += "-" + pathParts[i]; + nodesToLoad.push(pathStr); } - var activateId = expandNodes.pop(); - - return this.openNodes(expandNodes).then(foundAll => { - //console.log("activatePath", foundAll, activateId); - this.jstree.activate_node(activateId, null); + const nodeToSelect = nodesToLoad.pop(); - if (foundAll) { - var element = $(`#${activateId}`).get(0); - if (element) + return new Promise((resolve, reject) => { + this.jstree.load_node(nodesToLoad, () => { + // First select the node - the `select_node` method also recursively opens + // all parents of the selected node by default. + this.jstree.deselect_all(true); + this.jstree.select_node(nodeToSelect); + + const element = this.jstree.get_node(nodeToSelect, true)[0]; + if (element) { element.scrollIntoView(); - else { - console.log("element not found", activateId); + } else { + console.warn("element not found", nodeToSelect); } - } - - return foundAll; + this.updateActiveJstreeNode(); + resolve(); + }); }); } }