Skip to content

Commit

Permalink
Use jsTree's state plugin to persist object tree state
Browse files Browse the repository at this point in the history 
... 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
     #61 - this
     is an issue which reported this exact problem, but was closed
     because nobody knew how to reproduce it. The remark in
     #61 (comment)
     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).
  • Loading branch information
@generalmimon
generalmimon committed Feb 15, 2024
1 parent d05ff20 commit dcfdca6
Show file tree
Hide file tree
Showing 3 changed files with 113 additions and 124 deletions.
5 changes: 3 additions & 2 deletions lib/ts-types/jstree.d.ts
View file
Original file line number Diff line number Diff line change
@@ -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 <https://github.com/adaskothebeast>
// Definitions: https://github.com/DefinitelyTyped/DefinitelyTyped
Expand Down Expand Up @@ -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<string, any>;
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)
Expand Down
34 changes: 17 additions & 17 deletions src/v1/app.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 = <IParsedTreeNode>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 = <IParsedTreeNode>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);
Expand Down
198 changes: 93 additions & 105 deletions src/v1/parsedToTree.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, any>) {
const openNodes: string[] = state.core.open;
const nodesToLoad = new Set<string>();
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[]) => (<any>this.jstree).element.on(...args);
this.jstree.off = (...args: any[]) => (<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[]) => (<any>this.jstree).get_container().on(...args);
this.jstree.off = (...args: any[]) => (<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<IParsedTreeInterval>();
}

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 = <IParsedTreeNode>te.node;
this.parsedTreeOpenedNodes[this.getNodeId(node)] = true;
this.saveOpenedNodes();
}).on("close_node.jstree", (e, te) => {
var node = <IParsedTreeNode>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 {
Expand Down Expand Up @@ -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<boolean> {
return new Promise((resolve, reject) => {
this.saveOpenedNodesDisabled = true;
var origAnim = (<any>this.jstree).settings.core.animation;
(<any>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);
(<any>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<void> {
const pathParts = typeof path === "string" ? path.split("/") : path;

activatePath(path: string|string[]): Promise<boolean> {
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();
});
});
}
}

0 comments on commit dcfdca6

Please sign in to comment.