-
-
Notifications
You must be signed in to change notification settings - Fork 419
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
Update event loop documentation for gui_qt deprecation #2639
Conversation
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.
Thanks for working on this. Couple of typos and an optional suggestion to capitalize typos.
🥇
Thanks for the review! Agreed on all accounts |
Co-authored-by: Gonzalo Peña-Castellanos <goanpeca@gmail.com>
Co-authored-by: Gonzalo Peña-Castellanos <goanpeca@gmail.com>
…pari into update-evloop-docs
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.
Thanks for this update. I had a brief look at its all looking good to me, but maybe @DragaDoncila can do a thorough read and review. Thanks!!
[Edit: Also @GenevieveBuckley I know you've thought at lot about this too and how to make this as accessible to new comers as possible, maybe you can give it a read over too!!]
docs/guides/event_loop.md
Outdated
|
||
----------- | ||
|
||
|
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.
Can we shift the section on "Hooking up your own events" (which includes sub-sections on keybindings, mouse events, et) up above the nitty gritty about "The Qt Event Loop".
I actually think it might also be good to move the in-depth Qt Event Loop stuff into a separate markdown file, and just link to it from here for anyone who wants more information. That would make it much clearer that these parts are "explainer" documentation on how things work under the hood. Mixing it in here with the other practical info, like how to make your own keybindings, is very likely to confuse people.
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.
I kinda feel like if anything should go in another file, it's the hooking up of custom events (that always felt out of place to me here). The details about the qt event loop are the direct explanation of why you need napari.run sometimes and not other times (which I think you had asked for elsewhere?)
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.
Yeah, these things should get split up.
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.
I think that's a good idea, and it's also a good idea to clearly distinguish (a) which bits of the docs people need to use/type, vs (b) the bits we are explaining so you understand the stuff under the hood better (but we don't generally want new users to try and sprinkle QApplication
stuff through their code)
docs/guides/event_loop.md
Outdated
napari will detect if you are running an an IPython or Jupyter shell, and will | ||
automatically use the [IPython GUI event | ||
loop](https://ipython.readthedocs.io/en/stable/config/eventloops.html#integrating-with-gui-event-loops). | ||
As of [version 0.4.7](https://github.com/napari/napari/releases/tag/v0.4.7) is |
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.
I'd prefer the docs focus on what we do now, and de-emphasize what we used to do.
It's important to give people the info that we no longer need to call %gui qt
, but this is probably better distributed in the release notes, on zulip, and twitter, etc.
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.
I did try to deemphasize it with the offset, but given the 2 year usage... I kinda thought a lot of people would be asking "what about gui_qt??" So it'd be nice to send them somewhere to answer 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.
(Also not needing %gui qt
magic is a different point than this one, which is about napari.gui_qt
)
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.
I think this is ok for a transition period, but maybe in 3-6 months we can revisit and update the language a bit
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.
yeah, definitely doesn't have to be there forever
Thanks for doing this @tlambert03 |
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.
Thanks for the review @GenevieveBuckley. I really like the new connecting events section, I think a lot of people will just want to look there!
I think this is pretty close now. Maybe @DragaDoncila can take one more pass and then it will be good to merge
docs/guides/event_loop.md
Outdated
napari will detect if you are running an an IPython or Jupyter shell, and will | ||
automatically use the [IPython GUI event | ||
loop](https://ipython.readthedocs.io/en/stable/config/eventloops.html#integrating-with-gui-event-loops). | ||
As of [version 0.4.7](https://github.com/napari/napari/releases/tag/v0.4.7) is |
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.
I think this is ok for a transition period, but maybe in 3-6 months we can revisit and update the language a bit
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.
This looks great to me, and very readable. Just left a few minor proofreading tweaks.
Co-authored-by: Draga Doncila Pop <17995243+DragaDoncila@users.noreply.github.com>
Co-authored-by: Draga Doncila Pop <17995243+DragaDoncila@users.noreply.github.com>
Co-authored-by: Draga Doncila Pop <17995243+DragaDoncila@users.noreply.github.com>
Co-authored-by: Draga Doncila Pop <17995243+DragaDoncila@users.noreply.github.com>
Co-authored-by: Draga Doncila Pop <17995243+DragaDoncila@users.noreply.github.com>
Co-authored-by: Draga Doncila Pop <17995243+DragaDoncila@users.noreply.github.com>
Co-authored-by: Draga Doncila Pop <17995243+DragaDoncila@users.noreply.github.com>
Co-authored-by: Draga Doncila Pop <17995243+DragaDoncila@users.noreply.github.com>
Co-authored-by: Draga Doncila Pop <17995243+DragaDoncila@users.noreply.github.com>
thank you for the close read! |
Codecov Report
@@ Coverage Diff @@
## master #2639 +/- ##
=======================================
Coverage 81.87% 81.88%
=======================================
Files 475 476 +1
Lines 39475 39492 +17
=======================================
+ Hits 32322 32339 +17
Misses 7153 7153
Continue to review full report at Codecov.
|
Thanks for all the reviews here, will merge now |
Description
This updates the event loop doc to reflect the deprecation of
gui_qt
Type of change