feat: Platform runtime helper #18758
Conversation
github-actions
bot
added
the
area/skia ✏️
| SkiaGtk, | ||
| SkiaWpf, | ||
| SkiaX11, | ||
| SkiaFrameBuffer, | ||
| SkiaMacOS, |
There was a problem hiding this comment.
In such case I would go with SkiaWpfIslands to make it even more specific
| SkiaWpf, | ||
| SkiaX11, | ||
| SkiaFrameBuffer, | ||
| SkiaMacOS, |
There was a problem hiding this comment.
When\where\how would Skia* (e.g. SkiaMacOS) be returned ?
There's no code to allow to set it (from the host) since it's returned from a method. An internal Set... method (only for __SKIA__ could be used by the different hosts).
Also if Skia* values are returned when would the lone Skia value be returned ?
You might want to add an extension method, on the enum, like IsSkia() to make it easier to detect all Skia-based platforms (and remove Skia).
There was a problem hiding this comment.
This sounds good, in some cases you want to make some code skia specific (maybe because of native rendering limitations), in other cases it is the "native platform" that matters only; so a combination of IsSkia() and GetPlatform() makes sense; and dropping Skia enum value completely (and use unknown as the "default")
Co-authored-by: Jérôme Laban <[email protected]>
… into platform-runtime-helper
MartinZikmund
force-pushed
the
platform-runtime-helper
branch
from
a0cdd81
to
b5035d9
Compare
|
@mcNets rebased on latest master and adjusted the faulty commit message, hopefully now the PR will be green |
| SkiaGtk, | ||
| SkiaWpf, | ||
| SkiaX11, | ||
| SkiaFrameBuffer, | ||
| SkiaMacOS, |
There was a problem hiding this comment.
In such case I would go with SkiaWpfIslands to make it even more specific
| Android, | ||
| iOS, | ||
| MacCatalyst, | ||
| MacOSX, |
There was a problem hiding this comment.
I think this entry is not necessary, as it is actually only temporary anyway, as we will be dropping the legacy macOS implementation
| SkiaWpf, | ||
| SkiaX11, | ||
| SkiaFrameBuffer, | ||
| SkiaMacOS, |
There was a problem hiding this comment.
This sounds good, in some cases you want to make some code skia specific (maybe because of native rendering limitations), in other cases it is the "native platform" that matters only; so a combination of IsSkia() and GetPlatform() makes sense; and dropping Skia enum value completely (and use unknown as the "default")
|
|
||
| public static UnoRuntimePlatform Current => GetPlatform(); | ||
|
|
||
| private static UnoRuntimePlatform GetPlatform() => |
There was a problem hiding this comment.
Please add a runtime test that gets the value from GetPlatform() at runtime and checks that the result is not Unknown - this will future proof us against potential new platforms being added and us forgetting to update this code path
| SkiaX11, | ||
| SkiaFrameBuffer, | ||
| SkiaMacOS, | ||
| Unknown |
There was a problem hiding this comment.
I think Unknown should be the default value - (= 0)
|
@mcNets Nice PR! I know we are thorough with the review, but as this can be a very useful and frequently used API and it is public, we want to make sure to get it right 🙏 |
|
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-18758/index.html |
|
@MartinZikmund I've set default to Unknown, I removed IsAndroid, IsIOS, etc, and changed SkiaWpf by SkiaWpfIslands. I'll merged main branch again but still not green. Where should I add the test: |
|
|
|
|
|
@mcNets the test belongs in Uno.UI.RuntimeTests as it needs to run on each platform at runtime. There is this folder - https://github.com/unoplatform/uno/tree/master/src/Uno.UI.RuntimeTests/Tests/Uno_UI_Toolkit where you can add your new test class. Theoretically you can also add platform-specific tests in that test class as well - e.g.:
|
|
@MartinZikmund Do you know why CodeQL test is always failling? |
GitHub Issue (If applicable): closes #11545
PR Type
What kind of change does this PR introduce?
What is the current behavior?
You have to use conditionals to knot the current platform runtime.
What is the new behavior?
You can use RuntimeHelper.CurrentPlatform enumeration.
PR Checklist
Please check if your PR fulfills the following requirements:
Screenshots Compare Test Runresults.Other information
Follow this conversation:
#18432 (reply in thread)
I've had some issues with the filtered solutions, I've had been able to test it in Skia only.
Please, someone else should check the other platforms.
Internal Issue (If applicable):
#11545