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
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
|
|
||
| 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? |
This was fixed a few merges ago, the next build will succeed. |
|
|
|
|
|
@MartinZikmund I've added the tests now, I've checked with Skia and it pass. I'm trying to reword the commit that is causing red on conventional commits, but I'm getting some issue with the git rebase. @Youssef1313 , @spouliot Would you mind taking a look at the unresolved conversations and let me know if they can be closed? |
|
|
|
|
MartinZikmund
force-pushed
the
platform-runtime-helper
branch
from
ff8e2bd to
e2675e1
Compare
|
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-18758/index.html |
|
🤖 Your WebAssembly Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-18758/index.html |
|
🤖 Your WebAssembly Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-18758/index.html |
|
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-18758/index.html |
|
@jeromelaban there are still some test that they fail, and it seems that at least one of them is required. Any advice on what should I look for? |
|
@mcNets the fails occurs specifically on WPF Islands:
You can use the https://github.com/unoplatform/uno/tree/master/src/SamplesApp/UnoIslandsSamplesApp.Skia.Wpf app to run these two locally and see what is the issue |
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