Update SkiaSharp to 3.119, Svg.Skia to 3.0.0, HarfBuzzSharp to 8.3.1.1, Uno to 6.0.130 and CommunityToolkit.Mvvm to 8.4.0 #3031
Conversation
| } | ||
|
|
||
| // The Canvas needs to be first set before calling the Shared Constructor or else it crashes in the InvalidateCanvas | ||
| SharedConstructor(); |
There was a problem hiding this comment.
I had to move it here or it crashed on my computer with _canvas not initilized in InitCanvas(); I think this could be a problem on other platforms too. But I think this should be then a seperate pull request.
There was a problem hiding this comment.
Yes it is. I was just looking into this because of another bug report I got.
pauldendulk
left a comment
pauldendulk
left a comment
There was a problem hiding this comment.
Great!
I just have some questions.
| --> | ||
| <UnoFeatures> | ||
| </UnoFeatures> | ||
| SkiaRenderer; |
There was a problem hiding this comment.
What does the UnoFeatures SkiaRenderer do? It uses SkiaRendering instead of something else? Which platforms does it affect?
There was a problem hiding this comment.
SkiaRenderer is now the default on Uno 6.0. I'll investigate if this is needed in the Control. My guess is that this setting only works on an WinUI Application, because only in the WinUI Sample the references changed when I enabled this. But nothing changed in the MapControl.
There was a problem hiding this comment.
This is needed in the Control too, It adds some more dependencies (But only in the Wasm Targets)
There was a problem hiding this comment.
I don't think you should be using SkiaRenderer here - it will make it stop working on native renderers. The correct thing to do is leave it off unless you only want to support skia.
There was a problem hiding this comment.
Note that #if ANDROID blocks won't have any effect when a non-skia library is used on skia renderer app though.
LiveCharts2 renders using skia and it works on both native and skia renderers, might be worth having a look at what they did?
There was a problem hiding this comment.
@mikernet There #if ANDROID was removed, so that is a good thing?
There was a problem hiding this comment.
I don't think you should be using SkiaRenderer here - it will make it stop working on native renderers. The correct thing to do is leave it off unless you only want to support skia.
To 'support skia' means that the entire interface is rendered to a skia canvas, right? And from your url I understand that this is the default for Android and iOS fro Uno 6, but apparently it is just an option. So, the question is what Mapsui should support, or what it needs to do to support both.
There was a problem hiding this comment.
@pauldendulk Sorry, that was slightly confusing phrasing from me. For a library to support both uno native and uno skia renderers (where the whole app is rendered to a skia canvas), the library itself should not use the SkiaRenderer feature. The controls in the library itself can render to a skia surface I believe, as apparently evidenced by LiveCharts2 working fine on both, but not sure what caveats that entails.
| GC.SuppressFinalize(this); | ||
| } | ||
| #else | ||
| #if __ANDROID__ |
There was a problem hiding this comment.
Ghd #if ANDROID is not relevant anymore. Why?
There was a problem hiding this comment.
The dispose in Android is now too only Dispose() without (boolean) Parameter so making an #if Android compilation made no sense anymore because it is the same code as the #else case.
Samples/Mapsui.Samples.Uno.WinUI/Mapsui.Samples.Uno.WinUI/Platforms/Android/Main.Android.cs
Show resolved
Hide resolved
pauldendulk
left a comment
pauldendulk
left a comment
There was a problem hiding this comment.
Approving this. The questions can be answered later. I want to release this fix soon.
pauldendulk
changed the title
3 participants
Please summarize your PR and explain which problem it solves.
SharedConstructor should only be called after Canvas Initialization. Because else the First InvalidateCanvas crashes because the _canvas is not set yet.