how to use annotation with create_widget #68

gatoniel · 2022-12-12T19:40:06Z

Description

Adds to the documentation how to use annotations with magicgui.widgets.create_widget in a plugin subclass of QtWidgets.QWidget.

Type of change

Fixes or improves existing content

References

closes napari/napari#5340

uses information from https://github.com/PartSeg/PartSeg_bioimageio/blob/2afcbd558e52d6af6a13f0b844e80d280cd6ba9d/src/PartSeg_bioimageio/_widget.py#L421

Final checklist:

My PR is the minimum possible work for the desired functionality
I have commented my code, particularly in hard-to-understand areas
I have added alt text to new images included in this PR

melissawm

Hi @gatoniel - welcome and thanks for your PR! I left one minor comment (to sctually create a link to the magicgui-and-type-annotations section) but will leave the actual example to be reviewed by one of the core maintainers. Cheers!

docs/guides/magicgui.md

Czaki · 2022-12-12T22:26:30Z

This is a much more generous mechanism. magicgui.widgets.create_widget allows you to get magicgui widget for any type registered in magicgui.

So getting layer selector should be mentioned as an example of usage, not primary usage.

Maybe it could also be nice to add an example using magicgui.widgets.Container for complex examples that do not require direct Qt usage.

melissawm · 2023-04-25T17:28:13Z

Hi folks - I'd love to see this one go in. I will link to it in zulip to maybe get more eyes on it 😄

Co-authored-by: Melissa Weber Mendonça <melissawm@gmail.com>

melissawm · 2023-06-23T13:46:46Z

Hi @gatoniel - do you need help with updating your branch, or moving this forward? Thanks!

psobolewskiPhD · 2024-01-20T21:28:45Z

@lucyleeow @DragaDoncila
This fell through the cracks -- how does this fit in with the current state of extending napari docs?

lucyleeow · 2024-01-22T03:35:10Z

This looks useful but I think it is missing some context. In what situations would you not be able to use the @magicgui decorator?

I think this would be a better example than what is used here: https://napari.org/dev/howtos/extending/magicgui.html#qtwidgets-qwidget and we can over-write it. Could we add some more detail about how create_widget gives you more control over the selection widget created (i.e. we can specify the widget type (e.g., slider, RangeEdit etc)). I would also make it more general and not specifically about layer selection, though we can use that as an example.

@gatoniel are you still interested in continuing this PR?

melissawm · 2024-03-25T20:06:25Z

Gentle ping, @gatoniel ! Let us know if you need help 😄

lucyleeow · 2024-03-25T23:42:00Z

I think this would be a better example than what is used here: https://napari.org/dev/howtos/extending/magicgui.html#qtwidgets-qwidget and we can over-write it.

Reading again, I think it would be a good addition to the existing example. We could explain why/how create_widget makes things easier?

gatoniel · 2024-03-26T08:46:38Z

Hi, sorry for the late reply. I was / am busy with other stuff. Maybe I will have time this weekend.

gatoniel · 2024-04-02T10:42:33Z

This looks useful but I think it is missing some context. In what situations would you not be able to use the @magicgui decorator?

I do not know whether this question requires an answer as long as the information is available at https://napari.org/dev/howtos/extending/magicgui.html#qtwidgets-qwidget. For now, I have added the example as a special case there as people using QtWidgets.QWidget might be interested in still using the type annotated widgets with automatic synchronization.

Could we add some more detail about how create_widget gives you more control over the selection widget created (i.e. we can specify the widget type (e.g., slider, RangeEdit etc)). I would also make it more general and not specifically about layer selection, though we can use that as an example.

Unfortunately, I do not know enough about the other possibilities of create_widget.

Maybe it could also be nice to add an example using magicgui.widgets.Container for complex examples that do not require direct Qt usage.

However, in the example under https://napari.org/dev/howtos/extending/magicgui.html#magicgui-widgets-container create_widget is already used. Hence, I would actualy propose to close this PR without merging as now it becomes clear how to use create_widget to get for example layer selections without use of @magicgui.

lucyleeow · 2024-04-03T05:00:03Z

I think there is very useful and important information here (e.g., regarding refreshing the widget) that is not documented anywhere else. Fitting it in with existing documentation is more tricky and probably more work than originally intended. How about I try and add this example, along with some more info, and you can review if you are interested @gatoniel ?

gatoniel · 2024-04-03T07:39:16Z

Sounds good. Otherwise, I could also add some more description about the refreshing into the surrounding text.
Additionally, I guess it would be good to have a link from https://napari.org/dev/howtos/extending/magicgui.html#parameter-annotations to the availability of create_widget.

gatoniel · 2024-04-03T08:58:20Z

I have now added some more description about the refreshing and a note box in the #parameter-annotations section to direct the reader directly to this example if she is not concerned about @magicgui.

lucyleeow · 2024-04-03T10:24:05Z

Ah that is interesting. @DragaDoncila and I were talking about whether the reset_choices method in your example somehow overrides the parent CategoricalWidgets reset_choices method or if it is just called manually. If it is the latter, then technically it doesn't matter what this method is called right? Its just something we manually call in ShowEvent ?

lucyleeow · 2024-04-03T10:44:10Z

Now that I understand better, I think we could just have one QWidget example. The first example widget doesn't really do anything useful, but it does show how to connect a function to an event. I would maybe amend your example to add a function to execute when a layer is selected.

I think some more details about what is happening in the 2 methods would be useful too. And highlight that we need to update ourselves as magicgui is not doing this for us.

There are also several improvements I would like to make to that page, but this can be separate to this addition.

Overall, maybe this is too much extra work for you?

lucyleeow · 2024-04-03T10:45:31Z

docs/howtos/extending/magicgui.md

+
+You might want to add a layer selection as [shown above](#parameter-annotations) into your highly customizable `QtWidgets.QWidget` that is always synchronized with the available {attr}`napari.types.ImageData` layers in your viewer. For this you can use the {func}`create_widget <magicgui.widgets.create_widget>` function as described in the following example.
+
+To synchronize the information between the napari viewer (i.e. the available layers) the function `reset_choices` of the dropdown widget needs to be manually called whenever the main widget is shown. In the following example, this is done by defining two class methods `showEvent` and `reset_choices`.


I would have thought it needs to be called when the input selector widget is shown? If the main widget is open. then layers are changed, you'd expect the input selector widget to update?

I tested it. Napari somehow internally calls reset_choices of the main widget. So showEvent and reset_choices need to be named exactly like that. I updated it in the PR

gatoniel · 2024-04-03T11:17:37Z

Ah that is interesting. @DragaDoncila and I were talking about whether the reset_choices method in your example somehow overrides the parent CategoricalWidgets #68 (comment) method or if it is just called manually. If it is the latter, then technically it doesn't matter what this method is called right? Its just something we manually call in ShowEvent?

Yes, technically it doesn't matter what this method is called. One could also call the self.layer_select.reset_choices(event) in the showEvent method.

Overall, maybe this is too much extra work for you?

Depends. If I get rather precise orders, I can try :D

gatoniel · 2024-04-03T12:03:14Z

Yes, technically it doesn't matter what this method is called. One could also call the self.layer_select.reset_choices(event) in the showEvent method.

This was wrong. I tested the available options. It works only smoothly, when the function is exactly called reset_choices. It seems that napari calls this function of the docked widgets whenever something changed within napari. But I do not know where in the source code this call comes from. I've updated the PR accordingly. showEvent should just simply also call reset_choices as the choices should be reset whenever the widget is being shown.

DragaDoncila · 2024-04-04T02:02:55Z

It works only smoothly, when the function is exactly called reset_choices

Yeah I found this also but I couldn't for the life of me figure out where the connection is made.

I can see that for magicgui widgets we have a signal native_parent_changed, that calls the CategoricalWidget's reset_choices method - so I guess this would call layer_select.reset_choices when the containing widget, ExampleLayerListWidget changes.

What I don't understand is how having a method named reset_choices on a parent widget of a magicgui widget (in this case ExampleLayerListWidget) ensures that this method gets connected to layerlist events. There's no inheritance going on since ExampleLayerListWidget just inherits from QWidget, so magicgui must be reaching up from the layer_select widget to the parent widget somehow.

Pinging @tlambert03 who can hopefully unravel some of the magic in magicgui.

tlambert03 · 2024-04-04T02:24:06Z

What I don't understand is how having a method named reset_choices on a parent widget of a magicgui widget (in this case ExampleLayerListWidget) ensures that this method gets connected to layerlist events.

a grep of napari for reset_choices suggests its probably this:

https://github.com/napari/napari/blob/3cb47b5fc393427a09f3b98d641b3086ff93405e/napari/_qt/qt_main_window.py#L1093-L1100

(i.e. it's not magicgui magic, it's napari magic... i'm not sure if you want to "codify" that as a first class API here, but that's why it's working 😄 )

DragaDoncila · 2024-04-04T04:38:39Z

@gatoniel @lucyleeow I would say to move this PR forward while we think about how/if we want to maintain this reset_choices behavior, we should update the example to explicitly connect the layer_select.reset_choices method to viewer.layers events. It's a little more boilerplate, but a lot less magic, and I would prefer to have that documented rather than this brittle kinda weird thing we're doing in napari specifically for CategoricalWidgets.

lucyleeow · 2024-04-04T04:42:05Z

@gatoniel thanks for uncovering all of this, sorry this PR has gotten a bit derailed.

gatoniel · 2024-04-04T09:10:07Z

I changed the example. Now the reset_choices is directly connected to the three events in viewer.layers.events. I agree, this makes the example more straightforward and less magic :D Thanks @tlambert03 and @DragaDoncila for finding the mechanism...

lucyleeow · 2024-04-05T02:51:40Z

Thanks @gatoniel.

I still think that we only need one QWidget example. I would connect your widget to a function that does something vaguely useful with an input layer. Also mention that we need to connect to the layer events because magicgui is not doing this automatically for us, as with the above examples.

Could you also split your lines so max line length is 79 characters (this is what we've set for napari (see: here) but the docs repo has no linting). Thank you

lucyleeow · 2024-04-05T03:43:30Z

docs/howtos/extending/magicgui.md

+```{note}
+If you're interested in getting synchronized information from the napari viewer into widgets without `@magicgui`, see the [`create_widget` example](#magicgui-widgets-create_widget) below.
+```


I don't think this is necessary. I think we automatically update for all magicgui registered napari types; layer, layer subclasses and layer type data (e.g., napari.types.ImageData).
I am not sure if there would be any cases where you would want an input that is not in the above list.

If there is, we could mention that for non registered types you need to manually synchronize, see QWidget example for more...

cc @DragaDoncila

I have made the note more precise. I added it, because I was aware that one can easily create these synchronized dropdown lists of the available layers with @magicgui. But then, they were part of a bigger widget, and I was not able to figure out to create only these widgets which is possible with create_widget.

gatoniel · 2024-04-05T07:56:16Z

Could you also split your lines so max line length is 79 characters (this is what we've set for napari (see: here) but the docs repo has no linting). Thank you

Done. Is it ok now?

I still think that we only need one QWidget example. I would connect your widget to a function that does something vaguely useful with an input layer. Also mention that we need to connect to the layer events because magicgui is not doing this automatically for us, as with the above examples.

It is only one widget now. Clicking on the button prints the shape of the currently selected Image Layer. I mentioned that it needs manual connection to the layers events.

lucyleeow

Looks good, just some nits, thanks!

docs/howtos/extending/magicgui.md

Co-authored-by: Lucy Liu <jliu176@gmail.com>

DragaDoncila

Thanks for your work and patience on this @gatoniel! I suggested a more explicit comment about why we have to manually connect to reset_choices now, but am approving otherwise. Let's get this in!

docs/howtos/extending/magicgui.md

Co-authored-by: Draga Doncila Pop <17995243+DragaDoncila@users.noreply.github.com>

Czaki

If a widget has reset_choices method, then it will be called on all required events.

Fell free to reformat my text.

Czaki · 2024-04-10T08:54:49Z

docs/howtos/extending/magicgui.md

+manually connect the `reset_choices` of the created widget with the
+`viewer.layers.events` so that the available choices are synchronized
+with the current layers of the viewer:


Suggested change

manually connect the `reset_choices` of the created widget with the

`viewer.layers.events` so that the available choices are synchronized

with the current layers of the viewer:

define `reset_choices` method that will need to call `reset_choices`

of magicgui widgets.

Czaki · 2024-04-10T08:55:36Z

docs/howtos/extending/magicgui.md

+            "Selected layer has shape: ",
+            self.layer_select.value.shape,
+        )
+


Suggested change

def reset_choices(self):

self.layer_select.reset_choices()

Czaki · 2024-04-10T08:55:58Z

docs/howtos/extending/magicgui.md

+        layers_events = self.viewer.layers.events
+        layers_events.inserted.connect(self.layer_select.reset_choices)
+        layers_events.removed.connect(self.layer_select.reset_choices)
+        layers_events.reordered.connect(self.layer_select.reset_choices)


Suggested change

layers_events = self.viewer.layers.events

layers_events.inserted.connect(self.layer_select.reset_choices)

layers_events.removed.connect(self.layer_select.reset_choices)

layers_events.reordered.connect(self.layer_select.reset_choices)

#68 (comment)

@Czaki

Maybe, we can explain that part of napari magic, too, and show both possibilities with a marker that one of them is potentially deprecated?

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.

So then people will need to update their plugins if they do not depend on napari calling reset_choices.

@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 the layerlist events then yes, people may have to update their plugins, but I have no issue with that.

Could we explain the shortcut via reset_choices in the docs and also mention that there are plans that both viewer.layers.events and reset_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 relevant layers.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#L12

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.

jni · 2024-04-11T11:33:09Z

Hi folks,

I'm with Draga and Lucy here — I find the napari magic um, concerning, 😂, insomuch as it introduces a way-too-tight coupling between napari and magicgui, so I would rather we use the explicit connections in the docs. Yes @Czaki, the events might change, but they are part of the public API so in that case we would use a deprecation warningemitter cycle and so on and we would be able to update. (Of course, keeping the docs in sync is still a challenge, but that is the case either way — we might also remove the totally-private reset_choices magic and then the example would stop working — with no warning.)

@gatoniel thanks for offering to document both options — that is appreciated, but even if we decide to go that route, in my opinion the example as it is now is good and should go in, and we can expand on the magic in a subsequent PR.

I'll mark this as ready-to-merge and if @Czaki agrees with my rationale above, he can merge. 🙏 🚀

Thanks all for the discussion! I had actually not seen this page @lucyleeow, it is friggin GOLD. 🥇

Czaki · 2024-04-11T11:38:07Z

I still do not understand why you call this magic.
This is only the interface.

jni · 2024-04-11T11:40:11Z

it is magical to connect events behind the scenes based on a method name. (Not the signature, nor what the method does.) This is especially true if the interface is not annotated anywhere, specifically in the function that is adding the widget.

jni · 2024-04-11T11:43:54Z

At any rate @Czaki I don't think you can argue that layer events are public and therefore can be considered at least as stable as this internal connection, whether or not you call it magic? And that the docs are improved by this PR. I'd be happy to move this discussion to a new PR that discusses both options and their tradeoffs, as proposed by @gatoniel.

# References and relevant issues Came about after #68 Details `magicgui` type registration, in particular the `reset_choices` connection to layers we do for all widgets that have that method.

# References and relevant issues Follow on from #68 # Description * adds more info on when you would want to choose each type of widget, clarifying that widgets with access to the native QWidget are equally extensible. * clarify you can use `create_widget` in any widget class * Move the "Avoid imports with forward references" info to plugin best practices, where we already have a section about avoiding unnecessary imports * Move '`magicgui` function widgets as plugin contributions' to be next to the intro to magicgui function widgets

gatoniel added 2 commits December 12, 2022 20:31

how to use annotation with create_widget

4e73155

one more comment

8f1682f

github-actions bot added the documentation Improvements or additions to documentation label Dec 12, 2022

github-actions bot assigned gatoniel Dec 12, 2022

melissawm reviewed Dec 12, 2022

View reviewed changes

docs/guides/magicgui.md Outdated Show resolved Hide resolved

psobolewskiPhD requested a review from Czaki December 12, 2022 21:40

Update docs/guides/magicgui.md

5fd71e1

Co-authored-by: Melissa Weber Mendonça <melissawm@gmail.com>

merge with upstream

687b338

more info about reset_choices and info box from Parameter annotations

4d4b7f2

lucyleeow reviewed Apr 3, 2024

View reviewed changes

gatoniel added 2 commits April 3, 2024 13:55

call to setLayout was missing in __init__

674a2c1

reset_choices needs to be named reset_choices

2bd13b8

connect direct to viewer.layers.events

52076db

lucyleeow reviewed Apr 5, 2024

View reviewed changes

only one QWidget example, updated note

033e55f

psobolewskiPhD added this to the 0.5.0 milestone Apr 5, 2024

lucyleeow mentioned this pull request Apr 9, 2024

Add page on magicgui type registration #399

Merged

lucyleeow reviewed Apr 9, 2024

View reviewed changes

docs/howtos/extending/magicgui.md Outdated Show resolved Hide resolved

docs/howtos/extending/magicgui.md Outdated Show resolved Hide resolved

docs/howtos/extending/magicgui.md Outdated Show resolved Hide resolved

gatoniel and others added 2 commits April 9, 2024 15:18

Update docs/howtos/extending/magicgui.md

e3d9df9

Co-authored-by: Lucy Liu <jliu176@gmail.com>

Update docs/howtos/extending/magicgui.md

a8b68e6

Co-authored-by: Lucy Liu <jliu176@gmail.com>

DragaDoncila approved these changes Apr 10, 2024

View reviewed changes

docs/howtos/extending/magicgui.md Outdated Show resolved Hide resolved

lucyleeow reviewed Apr 10, 2024

View reviewed changes

docs/howtos/extending/magicgui.md Outdated Show resolved Hide resolved

gatoniel and others added 3 commits April 10, 2024 10:35

Update docs/howtos/extending/magicgui.md

303d811

Co-authored-by: Draga Doncila Pop <17995243+DragaDoncila@users.noreply.github.com>

Update docs/howtos/extending/magicgui.md

3c4e9e4

Co-authored-by: Draga Doncila Pop <17995243+DragaDoncila@users.noreply.github.com>

list -> created widget

ab19235

Czaki reviewed Apr 10, 2024

View reviewed changes

jni added the ready to merge label Apr 11, 2024

jni approved these changes Apr 11, 2024

View reviewed changes

psobolewskiPhD merged commit 79485ff into napari:main Apr 15, 2024
7 checks passed

psobolewskiPhD removed the ready to merge label Apr 15, 2024

lucyleeow mentioned this pull request Apr 16, 2024

Improve our 'Creating widgets' section #407

Merged


		You might want to add a layer selection as [shown above](#parameter-annotations) into your highly customizable `QtWidgets.QWidget` that is always synchronized with the available {attr}`napari.types.ImageData` layers in your viewer. For this you can use the {func}`create_widget <magicgui.widgets.create_widget>` function as described in the following example.

		To synchronize the information between the napari viewer (i.e. the available layers) the function `reset_choices` of the dropdown widget needs to be manually called whenever the main widget is shown. In the following example, this is done by defining two class methods `showEvent` and `reset_choices`.

how to use annotation with create_widget #68

how to use annotation with create_widget #68

Conversation

gatoniel commented Dec 12, 2022 • edited Loading

Description

Type of change

References

Final checklist:

melissawm left a comment

Choose a reason for hiding this comment

Czaki commented Dec 12, 2022 • edited Loading

melissawm commented Apr 25, 2023

melissawm commented Jun 23, 2023

psobolewskiPhD commented Jan 20, 2024

lucyleeow commented Jan 22, 2024

melissawm commented Mar 25, 2024

lucyleeow commented Mar 25, 2024

gatoniel commented Mar 26, 2024

gatoniel commented Apr 2, 2024

lucyleeow commented Apr 3, 2024

gatoniel commented Apr 3, 2024

gatoniel commented Apr 3, 2024

lucyleeow commented Apr 3, 2024

lucyleeow commented Apr 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gatoniel commented Apr 3, 2024

gatoniel commented Apr 3, 2024

DragaDoncila commented Apr 4, 2024

tlambert03 commented Apr 4, 2024 • edited Loading

DragaDoncila commented Apr 4, 2024

lucyleeow commented Apr 4, 2024

gatoniel commented Apr 4, 2024

lucyleeow commented Apr 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gatoniel commented Apr 5, 2024

lucyleeow left a comment

Choose a reason for hiding this comment

DragaDoncila left a comment

Choose a reason for hiding this comment

Czaki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DragaDoncila Apr 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jni commented Apr 11, 2024

Czaki commented Apr 11, 2024

jni commented Apr 11, 2024

jni commented Apr 11, 2024

gatoniel commented Dec 12, 2022 •

edited

Loading

Czaki commented Dec 12, 2022 •

edited

Loading

tlambert03 commented Apr 4, 2024 •

edited

Loading

DragaDoncila Apr 11, 2024 •

edited

Loading