Position the selection menu normally, even for nodes with duplicate names (when clicked on) #240
Conversation
also started working on improving canvas events docs
Still buggy (changing selection btwn clicking and searching seems off, but might have been already broken tbf), and code needs some polish. Also would be good to remove "Select a metadata column to summarize" from the menu if ambiguous -- TODO, wrap that stuff in a div and hide/unhide as needed
...based on if the smTable is shown or not
There was a problem hiding this comment.
Thanks @fedarko! I have a few minor comments. Also, can you checkout the docs folder on the biocore master branch so that the .qzv files are the same. That way once this branch is merged, it wont cause conflicts on the other branches.
| if (keyList.length > 1) { | ||
| // Multiple nodes have this name, so just place the menu at | ||
| // the root of the tree | ||
| node = this.empress._treeData[this.empress._tree.size - 1]; |
There was a problem hiding this comment.
Currently, empress uses a nodes postorder position in the tree starting at 1. So, I believe the root node should be this.empress._tree.size.
On a separate note, this was brought up during the code review that using 1-based index can lead to confusion since 0-base index is typical. I used 1-based index because the paper the bp-tree was based on used 1-based index. Do you think it would be worthwhile to convert empress to use 0-based index. This would be outside the scope of this PR.
There was a problem hiding this comment.
Thanks for catching this! Just altered this code to use the root's position for this stuff. Although now that I'm looking at it, what do you think about just positioning the menu/camera at the first (say, the first in the BP structure, or whatever) matching node? I think this might make more sense to the user, since the menu's "arrow" is pointing at a node that matched their search (and since they'll see the warning in the menu).
Addressing your other point: yeah, I think using 0-based indices would be a good idea in the future (although once we have testing in I guess it isn't a big deal). I opened #223 recently for this; would be nice for us to to get to that sometime.
| var isDup = false; | ||
| if (this.nodeKeys.length > 1) { | ||
| var isUnambiguous = true; | ||
| var keysOfNodesWithThisName = this.empress._nameToKeys[name]; |
There was a problem hiding this comment.
I though this.nodeKeys was this?
There was a problem hiding this comment.
Sorry I was unclear -- long story short, the way this works now is that if a user clicks on a node, then nodeKeys will just have a single entry of that clicked-on-node's key (even if the node's name is ambiguous). If the user searched for a node with a duplicate name (e.g. 0.000 in the moving pictures data), then we have no way of knowing which node they meant, so nodeKeys will contain all of the 0.000-named nodes.
So, nodeKeys and keysOfNodesWithThisName should always be the same unless this menu is for a node with a duplicate name that the user clicked on.
I reworked some of the code and added more thorough documentation, so hopefully this makes things less confusing.
Addresses @kwcantrell comment
Co-authored-by: kwcantrell <[email protected]>
... that is, if the selected node is ambiguous, the menu is positioned at the first node in nodeKeys, rather than at the root of the tree. Considering we show a warning and omit sample info in this case, I think this is fine. The code is still a bit spaghetti-ish -- should be tested more.
|
@kwcantrell comments have been addressed, as described in my responses. Let me know what you think! |
…y-schmancy-menu
to avoid conflicts, per @kwcantrell's request
|
@fedarko thanks this looks good! |
Closes #169. (More specifically, this addresses the second and currently-only-unchecked point in #169.)
When a node is clicked on,
CanvasEventsalready finds its key (an umambiguous identifier) inempress._treeData. Now, this key is used to center the camera on the node that was clicked on, as well as only highlight this node in green. Additionally, I believe the sample presence info should now be correct (since no iteration is done over all of the nodes with this name).If a node is selected by searching and its name is ambiguous, then the old behavior is used (the camera is centered on the root, and all nodes with this name are highlighted). However, now there's some extra work done in the selection menu code that hides the sample presence info for this case.