-
Notifications
You must be signed in to change notification settings - Fork 616
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
[WIP] Remove border from central widget #2255
base: main
Are you sure you want to change the base?
Conversation
Thanks for making this after our discussion on gitter. This PR is making me realize you can do the same kind of thing as So, I'd say we either make a PR to change the overall default to |
It looks like That's why in napari/napari#3621, I overrode the property getter in our sub-class, which I think is probably sufficient for napari, and probably for other users. What do you think? |
Darn, you're right. Let's not do a setter. I think you're right that the SceneCanvas needs control of that. It does seem very odd to have that border of 1. Did you want to try setting the default border width for all widgets to 0 and see what tests fail? |
Sure. I pushed that change to the branch associated with this PR, so feel free to run the tests. |
Yep, looks like some of the tests are failing because of direct image comparison (maybe with some previously prepared golden/standard image?). For example
On napari, we have merged our workaround, so maybe it's better to convert this PR to an issue, and only take action if other people report the same problem. Happy to write up the issue, but I'm not sure what else to do with this. |
Appreciate your work on this @andy-sweet. If any of the other @vispy/core has opinions on this I'm curious to hear them. I don't see a point for the border width of 1, but it seems a lot of the tests depend on that extra pixel not being drawn on. I think in the long run, assuming no one can remember why it is set to 1, we should remove it. Updating the test images shouldn't be too bad as I think there is a procedure for that in |
Agreed. |
This makes the
border_width
ofSceneCanvas.central_widget
0. By default aWidget
's border width is 1, which probably makes sense for things likeGrid
andViewBox
, but maybe less sense forWidget.central_widget
(though feel free to correct my understanding here!). The other problem is thatcentral_widget
is a property, so there's no way to pass through aborder_width
like in the constructor ofWidget
or in something likeWidget.add_view
.The issue fixed by this PR originally came up in #3357 on napari, though it was easy enough to workaround by overriding
SceneCanvas.central_widget
in napari'sVispyCanvas
.I marked this as WIP mostly because I'm new to vispy and don't have much confidence in my changes yet.