Skip to content
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

Add context menu for nodes #4950

Merged
merged 22 commits into from
Jan 15, 2021
Merged

Add context menu for nodes #4950

merged 22 commits into from
Jan 15, 2021

Conversation

MichaelBuessemeyer
Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer commented Nov 23, 2020

This PR adds a context menu for nodes that offers various actions.

Currently, the node context menu is only supported on nodes.
I could also change it, so that it is possible to open it up anywhere, no matter whether there is a node or not.
The following operations are currently implemented and are available if the menu is opened on an existing node:

  • One of the following:
    • Create Edge To This Node (When the active node and the clicked node are in the same tree)
    • Create Edge & Merge With This Tree (When the active tree and the tree the clicked node is in are not the same)
  • Delete Edge To This Node (Only enabled, when the active node is directly connected to this node)
  • Measure Length To This Node (Only enabled, when the active node and the clicked node are in the same tree)
  • Measure Length Of This Tree

Additional useful feature suggestions are welcome 😸

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz

Steps to test:

  • Open a skeleton tracing and create some trees.
  • The context menu should open with shift + right-click and should automatically close when an option is clicked or the user clicks somewhere else.
    • Currently, firefox still opens up the usual context menu which is kinda bothersome. I'll try to work out a fix for this.
  • Each option in the context menu should do what is says and should only be enabled if the action makes sense and is possible.
  • Play around with the options and try out edge cases like deleting a node in a "branch" should split the current tree.

Issues:


@MichaelBuessemeyer MichaelBuessemeyer self-assigned this Nov 23, 2020
@MichaelBuessemeyer
Copy link
Contributor Author

MichaelBuessemeyer commented Nov 30, 2020

Suggested Features:

  • directly show the euclidian distance between the clicked and the active node in the menu.
  • Add hint about the move node shortcut in the menu
  • "Hide this tree"
  • Add a version of the menu that has no selected node and has options like:
    • create node here
    • create new tree here
    • and maybe even more

@MichaelBuessemeyer
Copy link
Contributor Author

I would stop here adding no more features as otherwise this PR only becomes bigger and therefore harder to review.

@philippotto Could you please review this PR?

And do you have an idea on how to handle that firefox opens a context menu on right click + shift? I could not fix this / catch this event in firefox.

@MichaelBuessemeyer MichaelBuessemeyer marked this pull request as ready for review November 30, 2020 19:28
Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

Awesome stuff! This will be a great usability improvement :)

I left some code comments and also noted some things during testing:

  • In a hybrid tracing, pressing the shift modifier will open a tooltip in the toolbar. however, that tooltip doesn't mention "rightclick to open contextmenu" (it only says that a node can be selected).
  • The "info"-stuff in the toolbar is cool! However, I'd move it to the bottom, so that one doesn't need to move the mouse across the info stuff to trigger an action.
  • The info part could also show the node id of the clicked node and the corresponding tree id. Also, the clicked (node's) position could be shown in another line (again, clicking it would copy that position).
  • Maybe rename "Navigate to subsequent Active Node" to "Activate next node" (same for previous)
  • It's cool that the direct distance to the active node is immediately visible in the context menu. However, copying that distance is hard since the contextmenu immediately vanishes when selecting something. In general, this seems fine to me, but maybe we can simply copy the distance to the clipboard when clicking the entry?
  • Why is the "Activate Next/Previous Node" action under "possible interactions" ? since it's a simple trigger (as opposed to the move action which needs additional interaction), we could simply list it in the top-level menu? however, it might be a bit weird, since it doesn't have to do anything with the clicked position 🤔 but that's a problem in general. Overall I'm a bit hesitant about the "possible interactions" UI. It's not very typical (afaik) to have such a help section in a context menu. What do you think about removing this for the first iteration?

I'll have a look regarding the context menu bug in firefox!

frontend/javascripts/libs/utils.js Outdated Show resolved Hide resolved
frontend/javascripts/oxalis/api/api_latest.js Outdated Show resolved Hide resolved
frontend/javascripts/oxalis/api/api_latest.js Outdated Show resolved Hide resolved
frontend/javascripts/oxalis/api/api_latest.js Outdated Show resolved Hide resolved
frontend/javascripts/oxalis/api/api_latest.js Outdated Show resolved Hide resolved
frontend/javascripts/oxalis/view/node_context_menu.js Outdated Show resolved Hide resolved
frontend/javascripts/oxalis/view/node_context_menu.js Outdated Show resolved Hide resolved
frontend/javascripts/oxalis/view/node_context_menu.js Outdated Show resolved Hide resolved
frontend/javascripts/oxalis/view/node_context_menu.js Outdated Show resolved Hide resolved
frontend/javascripts/oxalis/view/node_context_menu.js Outdated Show resolved Hide resolved
@philippotto
Copy link
Member

Currently, firefox still opens up the usual context menu which is kinda bothersome. I'll try to work out a fix for this.

This seems to be by design :( Then, I suggest to also allow Meta + Rightclick to open the wk context menu. It's a bit weird (which is why I wouldn't make it the default), but would be a workaround for firefox. When the user uses firefox and uses shift + rightclick, you could show a toast that they should use Win/Cmd + Right click instead, since they are using firefox.

Btw, I wouldn't use ctrl + rightclick, since mac users without a right mouse button would use ctrl + leftclick to get a rightclick. And alt + rightlick wouldn't work on default linux setups since that opens an OS context menu.

@MichaelBuessemeyer
Copy link
Contributor Author

MichaelBuessemeyer commented Dec 4, 2020

TODO List:

  • In a hybrid tracing, pressing the shift modifier will open a tooltip in the toolbar. however, that tooltip doesn't mention "rightclick to open contextmenu" (it only says that a node can be selected).
  • The "info"-stuff in the toolbar is cool! However, I'd move it to the bottom, so that one doesn't need to move the mouse across the info stuff to trigger an action.
  • The info part could also show the node id of the clicked node and the corresponding tree id. Also, the clicked (node's) position could be shown in another line (again, clicking it would copy that position).
  • Maybe rename "Navigate to subsequent Active Node" to "Activate next node" (same for previous)
  • It's cool that the direct distance to the active node is immediately visible in the context menu. However, copying that distance is hard since the contextmenu immediately vanishes when selecting something. In general, this seems fine to me, but maybe we can simply copy the distance to the clipboard when clicking the entry?
  • Why is the "Activate Next/Previous Node" action under "possible interactions" ? since it's a simple trigger (as opposed to the move action which needs additional interaction), we could simply list it in the top-level menu? however, it might be a bit weird, since it doesn't have to do anything with the clicked position 🤔 but that's a problem in general. Overall I'm a bit hesitant about the "possible interactions" UI. It's not very typical (afaik) to have such a help section in a context menu. What do you think about removing this for the first iteration?
  • [ ] Implement the firefox workaround We decided against that
  • Add this feature to the docs

@MichaelBuessemeyer
Copy link
Contributor Author

Why is the "Activate Next/Previous Node" action under "possible interactions" ? since it's a simple trigger (as opposed to the move action which needs additional interaction), we could simply list it in the top-level menu? however, it might be a bit weird, since it doesn't have to do anything with the clicked position 🤔 but that's a problem in general. Overall I'm a bit hesitant about the "possible interactions" UI. It's not very typical (afaik) to have such a help section in a context menu. What do you think about removing this for the first iteration?

I think removing it is fine.

@MichaelBuessemeyer
Copy link
Contributor Author

On hold because of planned keyboard shortcuts / UI toolbar rework in #4997

@MichaelBuessemeyer
Copy link
Contributor Author

@philippotto I applied all the feedback, added the context menu to the docs, and tested each feature a bit. Could you please check this PR again?

Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

Awesome stuff! Works very well for me and the code looks good, too :)

frontend/javascripts/libs/mjs.js Outdated Show resolved Hide resolved
@bulldozer-boy bulldozer-boy bot merged commit 0131db8 into master Jan 15, 2021
@bulldozer-boy bulldozer-boy bot deleted the add-context-menu-for-nodes branch January 15, 2021 12:15
@hotzenklotz hotzenklotz mentioned this pull request Oct 21, 2022
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Shortest path (length) between two nodes / More UI
2 participants