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

Expose QScrollArea as native widget #429

Merged
merged 1 commit into from
Jul 19, 2022

Conversation

dstansby
Copy link
Contributor

Fixes https://github.com/napari/magicgui/issues/431 - this allows an external user to access the QScrollArea by returning it as the native widget. I think this is a sensible thing to do anyway, and it fixes a bug with the scroll bars not showing up when a widget is consumed by napari.

@codecov
Copy link

codecov bot commented Jun 20, 2022

Codecov Report

Merging #429 (ebab119) into main (919acb2) will decrease coverage by 0.00%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##             main     #429      +/-   ##
==========================================
- Coverage   88.99%   88.99%   -0.01%     
==========================================
  Files          30       30              
  Lines        3935     3944       +9     
==========================================
+ Hits         3502     3510       +8     
- Misses        433      434       +1     
Impacted Files Coverage Δ
magicgui/backends/_qtpy/widgets.py 86.95% <75.00%> (-0.06%) ⬇️
magicgui/widgets/_bases/widget.py 87.91% <100.00%> (+0.20%) ⬆️
magicgui/widgets/_protocols.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 919acb2...ebab119. Read the comment docs.

@tlambert03
Copy link
Member

thank you @dstansby!

I am slightly concerned about this (but I'm not sure, and could be convinced otherwise). The main reason is that there is sort of an implicit API that when you do widget.native on a container, you get the widget that has the layout... and I know a few people are accessing that to do things like widget.native.layout().addStretch()or something... I _think_ this would break that... and would require everyone consumingwidget.nativeto add anif isinstance()` check to see if the widget is a QScrollArea or not if they wanted to avoid potential errors. I know this fixes the scrollbars in napari, but I think it might create errors elsewhere. thoughts?

@dstansby
Copy link
Contributor Author

That all makes sense, and I think you're repeating yourself from the original PR, so sorry about that. So, it seems we should keep .native as the widget that has the layout (and document that if it isn't already, I'll check that), and expose another attribute that 3rd party libraries use for embedding. Since the widget with the layout is a child of the scroll area, there's no way around needing to expose the scroll area for embedding for scrolling to work.

@dstansby dstansby marked this pull request as draft June 27, 2022 15:27
@dstansby
Copy link
Contributor Author

I've added a new property, root_native_widget, which is designed to be whathever the top widget is and is designed to be embedded by third parties. Not sure if this a good approach, and the API might need bikeshedding, but thought I'd push some code for discussion 😃

@tlambert03
Copy link
Member

I think i like it! will have a play shortly... but this seems like a very good solution

@dstansby dstansby marked this pull request as ready for review July 7, 2022 18:53
Copy link
Member

@tlambert03 tlambert03 left a comment

Choose a reason for hiding this comment

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

looks excellent, thank you @dstansby!

@tlambert03 tlambert03 merged commit 88a0caa into pyapp-kit:main Jul 19, 2022
@dstansby dstansby deleted the scroll-native branch July 20, 2022 07:40
@tlambert03 tlambert03 added the bug Something isn't working label Oct 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

napari plugin scrollbar not appearing
2 participants