Remove recover from svg drawing

[Jacalz]

Description:

I ran all .svg files I could find on my system through a test application that I wrote and it did not crash once with this change (although there were some other printed errors from the image loading related to strconv.Atoi used on floating point values among other things). After that, I believe that the recover no longer is relevant. Also, we should fix the crash in the dependency instead of working around the issue there in any case. I anyone has a file which does crash, please provide one.

Checklist:

• Tests included.
• Lint and formatter run with no errors.
• Tests all pass.

[Jacalz]

Ha, that's funny. I piped all images outside the development container through my test program but then it turns out that an image in our own repository causes the crash to happen. Lol 😂

[Jacalz]

Hmm. Maybe not? Piping all svg files in the repo through my test app does not cause a crash. Something is strange with the test maybe?

[andydotxyz]

I'm not aware of any images in our repo that crash it - otherwise they would not display.

However I would be extremely cautious of just removing this because it's been there for 6 years now.
It was required because many icons that we had not encountered were causing the renderer to crash - this seemed better than allowing apps to crash.

Unfortunately the original change did not list files or issues that caused the crash but I know the library is prone to crashes on unexpected parse content 7c56bda.

[Jacalz]

I'm not aware of any images in our repo that crash it - otherwise they would not display.

Something in the repo is causing the tests to crash after this change (not before). It might just be a faulty thest though. I will investigate.

However I would be extremely cautious of just removing this because it's been there for 6 years now.
It was required because many icons that we had not encountered were causing the renderer to crash - this seemed better than allowing apps to crash.

Absolutely, agreed. That is why this is draft. My idea was to go through the oksvg code and patch up any cases where crashes could happen but I got a bit excited when I couldn't find any crashes. Will continue investigating and working on oksvg fixes as well.

[Jacalz]

I found out why that specific test was crashing. A crash in golang.org/x/image was apparently triggered by the test to attaching the Card widget to a window before doing some renderer lookup stuff.

[Jacalz]

That dialog error is even stranger. I think the removal of the recover() is making other bugs surface within our toolkit. Any help to diagnose this would be appreciated.

[redawl]

That dialog error is even stranger. I think the removal of the recover() is making other bugs surface within our toolkit. Any help to diagnose this would be appreciated.

@Jacalz I am able to fix the dialog test with the below diff:

diff --git a/widget/popup.go b/widget/popup.go
index ec102b82c..a17bc3e9a 100644
--- a/widget/popup.go
+++ b/widget/popup.go
@@ -258,7 +258,7 @@ func (r *modalPopUpRenderer) Layout(canvasSize fyne.Size) {

        requestedSize := innerSize.Subtract(padding)
        size := r.popUp.Content.MinSize().Max(requestedSize)
-       size = size.Min(canvasSize.Subtract(padding))
+       size = size.Min(fyne.NewSize(0, 0).Max(canvasSize.Subtract(padding)))
        pos := fyne.NewPos((canvasSize.Width-size.Width)/2, (canvasSize.Height-size.Height)/2)
        r.popUp.Content.Move(pos)
        r.popUp.Content.Resize(size)

You are right: Removing the recover is showing places where tests are giving the drawing code bad data. In this case, the panic is caused by an image with negative size being specified as the dest image. Negative size is coming from the canvas size of 0, 0 minus a padding of 8.

Not sure if the above diff is the correct place to fix it though. Also, there are other tests that are failing even after this change, but it is probably for the same reasons :)

Hope that helps