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

Support default keys for makeToolButton #347

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kaikue
Copy link
Contributor

@kaikue kaikue commented Oct 25, 2021

This is part of the seung-lab merge project (see #340).

It would be convenient to have default keybinds for operations like merge and split, so that the user doesn't have to set them manually. This PR adds an optional defaultKey parameter to the ToolBindingWidget constructor and makeToolButton function, which will fill in the tool button with that key the first time Neuroglancer is loaded, but respect the user's choice and use a different key if one has been set manually.

Caveat: If the user unsets the tool (by double-clicking) then reloads Neuroglancer, it will be set back to the default. The LayerToolBinder.jsonToKey isn't structured to support tool -> undefined mappings, so I'm not sure if there's a good way around this, but it's a minor annoyance.

@google-cla
Copy link

google-cla bot commented Oct 25, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@kaikue
Copy link
Contributor Author

kaikue commented Oct 25, 2021

@googlebot I signed it!

@jbms
Copy link
Collaborator

jbms commented Oct 26, 2021

To summarize the discussion I had with @sdorkenw last week:

  1. Having to press and hold shift+key is less convenient than just using a single key. The reason these "tool bindings" use shift+key is to ensure user-customizable bindings never conflict with the built-in key bindings in Neuroglancer. This issue seemed to be lower priority, though.
  2. It is inconvenient for users to have to define keys for split and merge actions manually. It would be better if users could just add a graphene datasource and immediately split and merge with a default key binding.
  3. In the case that there are multiple layers with graphene data sources, the default key bindings would naturally conflict. Currently user-defined key bindings are not allowed to conflict. Instead, it could be allowed for user-defined key bindings to conflict, with the selected layer taking priority. The selected layer is shown with green border in layer bar, and also the side panel for the selected layer is shown with a green title bar. Note: With the introduction of user-customization of key bindings and the ability to show multiple side panels at once, I had intended to reduce the importance of the selected layer.
  4. Alternatively, in addition to the user-customizable key bindings for split and merge, there could be user-non-customizable key bindings for split and merge (not using shift), that only apply to the current selected layer. This would basically amount to supporting the seung lab's existing way of performing split/merge actions in addition to supporting the user-customizable bindings. Having two different ways of performing split/merge actions may be somewhat confusing, though, and would add some unfortunate additional complexity.

This pull request seems to be intended to address point 2 alone. Does the seung lab believe that will be sufficient to address this issue, or is it desired that additional changes also be made?

@jbms
Copy link
Collaborator

jbms commented Oct 26, 2021

As far as the implementation, although I can see that makeToolBinding provides a convenient place to add the logic, it is not really the right place, since that will only be called when the UI control is created, i.e. when the side panel is shown.

Instead, the assignment of a default key binding should occur when the layer is created, or when the graph source is first added, I think. But that is indeed a bit tricky.

Also we do really need to solve the issue of being able to unbind the key and not have it come right back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants