[WIP] Remove border from central widget

[andy-sweet]

This makes the border_width of SceneCanvas.central_widget 0. By default a Widget's border width is 1, which probably makes sense for things like Grid and ViewBox, but maybe less sense for Widget.central_widget (though feel free to correct my understanding here!). The other problem is that central_widget is a property, so there's no way to pass through a border_width like in the constructor of Widget or in something like Widget.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's VispyCanvas.

I marked this as WIP mostly because I'm new to vispy and don't have much confidence in my changes yet.

[djhoese]

Thanks for making this after our discussion on gitter. This PR is making me realize you can do the same kind of thing as add_view or add_widget where you make your own Widget/ViewBox/other widget instance and do canvas.central_widget = my_widget_instance. As long as you do that before accessing canvas.central_widget as a getter property then it should assign your custom widget.

So, I'd say we either make a PR to change the overall default to 0 or we leave this to users to customize their widgets. Thoughts?

[andy-sweet]

This PR is making me realize you can do the same kind of thing as add_view or add_widget where you make your own Widget/ViewBox/other widget instance and do canvas.central_widget = my_widget_instance

It looks like SceneCanvas.central_widget is a read-only property. I guess we could add a setter, but it doesn't immediately feel right to me because I had the impression it's the root visual of a scene canvas. It's also described as taking up the entire area of the canvas (i.e. it gets its size from its canvas), so it also seems odd for any widget (that could have a different size than the canvas) to be assigned to it.

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?

[djhoese]

It looks like SceneCanvas.central_widget is a read-only property.

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?

[andy-sweet]

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.

[andy-sweet]

Yep, looks like some of the tests are failing because of direct image comparison (maybe with some previously prepared golden/standard image?). For example

=================================== FAILURES ===================================
___________________________ test_perspective_render ____________________________
vispy/scene/cameras/tests/test_perspective.py:53: in test_perspective_render
    max_px_diff=200)
vispy/testing/image_tester.py:138: in assert_image_approved
    assert_image_match(image, std_image, **kwargs)
vispy/testing/image_tester.py:201: in assert_image_match
    assert mask.sum() <= px_count
E   AssertionError
----------------------------- Captured stdout call -----------------------------
Attempting to create git clone of test data repo in /home/runner/.vispy/test_data..
git --git-dir=/home/runner/.vispy/test_data/.git --work-tree=/home/runner/.vispy/test_data init
Initialized empty Git repository in /home/runner/.vispy/test_data/.git/
git --git-dir=/home/runner/.vispy/test_data/.git --work-tree=/home/runner/.vispy/test_data remote add origin http://github.com/vispy/test-data
git --git-dir=/home/runner/.vispy/test_data/.git --work-tree=/home/runner/.vispy/test_data fetch --tags origin test-data-10 --depth=1
git --git-dir=/home/runner/.vispy/test_data/.git --work-tree=/home/runner/.vispy/test_data checkout -b main FETCH_HEAD

Image comparison failed. Test result: (200, 120, 4) uint8   Expected result: (200, 120, 4) uint8
Uploaded to: 
http://data.vispy.org/data/scene/cameras/dc853980a742db4a2ac1f5a5a758ed007a6c017c/perspective_test.png

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.

[djhoese]

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 make.py but the other individual tests may get a little annoying.

[almarklein]

I don't see a point for the border width of 1,

Agreed.