Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
how to use annotation with create_widget #68
how to use annotation with create_widget #68
Changes from 9 commits
4e73155
8f1682f
5fd71e1
687b338
4d4b7f2
674a2c1
2bd13b8
52076db
033e55f
e3d9df9
a8b68e6
303d811
3c4e9e4
ab19235
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#68 (comment)
@Czaki
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, we can explain that part of napari magic, too, and show both possibilities with a marker that one of them is potentially deprecated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are working on nested Layers (Layer Groups). It may change a list of events. So then people will need to update their plugins if they do not depend on napari calling
reset_choices
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Czaki I'm not happy to enshrine this behaviour in our docs right now (see original comment here). It's very brittle and in my opinion, quite an overreach that could lead to surprising effects on widgets totally unrelated to the layerlist. I would strongly prefer to keep the example as it currently is, because it involves no magic and no reliance on
napari
internals. If we one day change thelayerlist
events then yes, people may have to update their plugins, but I have no issue with that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we explain the shortcut via
reset_choices
in the docs and also mention that there are plans that bothviewer.layers.events
andreset_choices
might change in future, so that developers should check the corresponding source code once there plugins stop working as expected.reset_choices
https://github.com/napari/napari/blob/50b314f7ad2e7cd4ef21b5705b3c6b363c0857ed/napari/_qt/qt_main_window.py#L1093 shows all the relevantlayers.events
that would need to be connected individually.viewer.layers.events
currently is a instance subclassing https://github.com/napari/napari/blob/50b314f7ad2e7cd4ef21b5705b3c6b363c0857ed/napari/utils/events/containers/_selectable_list.py#L12There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the offer @gatoniel —if we decide to take you up on it or parts of it, I think expanding on the reset_choices magic should come in a separate PR. For now I think we are still internally deciding whether we want it in the docs, as you can see 😅, so I think this should go in as-is.
Regarding the future of the event connections: in my opinion, if we start worrying in the docs about all the things we are planning to deprecate, we will never document anything. 😂 I think in general the docs should be written with regard to the current public API, unless we really want to discourage use of a specific API. Which is not the case here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.