Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Uno Platform Support #121

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

MartinZikmund
Copy link

@MartinZikmund MartinZikmund commented May 30, 2020

Support for Uno Platform/non-XF UWP

Draft for #120

TODOs:

  • macOS support (will require macOS support for Uno.SkiaSharp.Views)
  • More samples (porting from XF samples)
  • Input support
  • float -> double dependency properties
  • Fix WASM build (I will probably need @jeromelaban)
  • Make default foreground depend on current theme

@MartinZikmund
Copy link
Author

@Happypig375 Does not build properly yet, need to resolve some WASM issues, plus add some samples :-)

@charlesroddie
Copy link
Collaborator

charlesroddie commented May 30, 2020

I read through the list of changed files.

  • CSharpMath.Xaml/Views.cs: small changes and not part of the core of the repo so OK
  • CSharpMath.Uno/CSharpMath.Uno.csproj : small and not part of the core repo so OK
  • CSharpMath.SkiaSharp.Wasm: this is Uno-specific so is misnamed. Should be moved into CSharpMath.Uno or renamed CSharpMath.Uno.SkiaSharp. It references Uno.SkiaSharp.Wasm so would not be expected to work on other wasm platforms such as Blazor.

This is definitely a valuable spike if only to test that CSharpMath works with Uno.

Main issue with merging IMO is

+3,397 −4

The aim of this PR, as clarified in the linked issue, is to allow usage from CSharpMath in a way that doesn't require writing the 10 lines of code needed to interface with CSharpMath.SkiaSharp. Adding 3400 lines to the repo in order to save perhaps 1-3 end-users having to write 10 lines of code each doesn't seem like good arithmetic to me. If there were hundreds of projects dependent on CSharpMath it would be a different story.

An ultra-clean extreme would be: it's SkiaSharp's responsibility to show people how to install a SkiaSharp view in any .net platform, and CSharpMath's responsibility to inject manage latex rendering in this view: then we'd only need to maintain one example project.

@Happypig375
Copy link
Collaborator

@charlesroddie Most of it is for an example project. It is a CSharpMath.Uno.Example just like CSharpMath.Avalonia.Example and CSharpMath.Forms.Example.

@charlesroddie
Copy link
Collaborator

charlesroddie commented May 30, 2020

Understood that it is example projects but example projects still take maintenance (e.g. having to make updates, and adding to build times for the solution). As mentioned the my paragraph it doesn't need to be CSharpMath's responsibility to maintain a lot of example projects and have them build and work.

@MartinZikmund
Copy link
Author

@charlesroddie so the suggestion would be to remove the example project or to create a completely separate repository for Uno support? Most of the added code are the samples, which are helpful, but I can provide them separately in my own repo if they do not fit here well.

The goal of the SkiaSharp.Wasm going forward is to be the official first-party solution for WebAssembly (not just Uno specific)

@Happypig375
Copy link
Collaborator

Happypig375 commented May 30, 2020

@charlesroddie Not really. Each platform make minimal breaking changes and maintenance is low-cost. Build times of one front end do not affect other front ends.

@Happypig375
Copy link
Collaborator

@MartinZikmund I don't like separating repos after what I saw with https://github.com/oxyplot: repos other than the main one get little maintenance.

@MartinZikmund
Copy link
Author

@MartinZikmund I don't like separating repos after what I saw with https://github.com/oxyplot: repos other than the main one get little maintenance.

First party support is better, but if you preferred not to have here directly, I would create a Uno fork separately (this was my original plan, until I noticed Uno was among the ideas for a future update)

@charlesroddie
Copy link
Collaborator

As long as we don't require platform example builds to succeed for PRs, and for @Happypig375 and @MartinZikmund to maintain the additional projects then it's OK for then another examples project can go through.

We'll definitely need to tidy up the repo - inlcuding creating a folder structure for examples, platforms, etc.. And creating a solution without the peripheral projects would be a good idea too.

My note on CSharpMath.SkiaSharp.Wasm should be addressed as even if

The goal of the SkiaSharp.Wasm going forward is to be the official first-party solution for WebAssembly

, it's not at the moment as is clear from the naming, and when it becomes official it would probably be merged into SkiaSharp anyway so that part of this PR would be deleted at that point.

@Happypig375
Copy link
Collaborator

As long as we don't require platform example builds to succeed for PRs

Why? The whole solution should be buildable, updates should come in via PRs.

@charlesroddie
Copy link
Collaborator

It could hold back PRs. Currently you are fast at fixing issues so not a problem if you are happy to fix PRs where any online builds are failing. But contributors shouldn't have to have all the build tooling needed to build all the example platforms, so should not be expected to confirm that the entire solution builds if it contains all of these.

@Happypig375
Copy link
Collaborator

If they can't build all the platforms, then PR checks will do it for them. The errors are publicly visible.

@jeromelaban
Copy link

@MartinZikmund at this point, building on GitHub actions or Azure Devops using a WebAssembly dependencies (Uno.SkiaSharp being one) is not supported. The only way to create a valid binary in CI is to use this docker image unoplatform/wasm-build:2.0.

I've made a change in the bootstrapper to provide the ability to get non-failing windows builds (unoplatform/Uno.Wasm.Bootstrap#216), but the resulting binary will be invalid, as the WebAssembly static linking step will be disabled.

In your case, once the bootstrapper PR will be published, you'll need to add the following:

<WasmShellForceDisableWSL Condition="'$(CI)'=='true'">true</WasmShellForceDisableWSL>

using the GitHub actions default variables.

This will allow you to make your Windows build pass, and add another job that will run on Linux to have a proper application.

@jeromelaban
Copy link

@charlesroddie Indeed, Uno.SkiaSharp.Wasm will not stay alive on a long term. It's only required at this point because of some missing features in mono for WebAssembly. Once the features are added, the official SkiaSharp package will support WebAssembly, most likely around the .NET 5 release.

@MartinZikmund
Copy link
Author

@jeromelaban I am getting the issue I mentioned when building locally as well, is there something I am missing to get it working there? Thanks :-)

@jeromelaban
Copy link

@jeromelaban I am getting the issue I mentioned when building locally as well, is there something I am missing to get it working there? Thanks :-)

If you're getting this error locally, it means you have not setup WSL. You can set it up using this.

@yoshiask
Copy link

yoshiask commented Jun 3, 2020

Would support for just UWP (not tested on Uno Platform) be helpful?

@Happypig375
Copy link
Collaborator

Would support for just UWP (not tested on Uno Platform) be helpful?

Uno runs on UWP just fine. What's the problem?

@yoshiask
Copy link

yoshiask commented Jun 3, 2020

What I mean to ask is if CSharpMath.UWP would be useful in UWP-only projects after CSharpMath.Uno is added.

@Happypig375
Copy link
Collaborator

There was never a CSharpMath.UWP.

@yoshiask
Copy link

yoshiask commented Jun 3, 2020

I know, I've just started working on it.

@Happypig375
Copy link
Collaborator

You can probably just reference CSharpMath.Uno in a UWP project with no issues. In this PR, CSharpMath.Uno only references Uno on non-UWP platforms.

@MartinZikmund
Copy link
Author

Sorry for the delay on commits, I am currently slowed down with several other commitments, but I will definitely get back to this :-)

@MartinZikmund
Copy link
Author

@Happypig375 @charlesroddie What about putting the sample projects in a separate solution (under UWPUno folder)? It would not be part of the main solution, so the sample's buildability would not affect PRs (plus would not require build adjustments for WASM build), would that be good?

@Happypig375
Copy link
Collaborator

I suggest waiting for MSBuild 16.7 and using a solution filter: dotnet/msbuild#4097

@MartinZikmund
Copy link
Author

So far I have found out one blocker - the controls in CsharpMath.XAML use float data types for DependencyProperties, which are unfortunately not properly supported in UWP XAML. To circumvent this, I will probably make a custom version of the controls for the UWPUno library

@Happypig375
Copy link
Collaborator

@MartinZikmund You can apply breaking changes to CSharpMath.Xaml as our major version is still 0. Please don't duplicate code for custom controls. Just remember to add tests for the scenarios that are enabled.

@Happypig375
Copy link
Collaborator

@MartinZikmund Visual Studio 16.7 has been out and I think Solution Filters can be tried.

@MartinZikmund
Copy link
Author

I'm finally back to this PR, so I will be rebasing this and fixing the problems. SkiaSharp now has first-party support for Uno, so that will be part of the changes as well.

@MartinZikmund
Copy link
Author

Started bringing this up to date, now includes macOS support, will wait a while before the SkiaSharp Uno 3.0 update is on NuGet too (mono/SkiaSharp#1489).

@MartinZikmund MartinZikmund marked this pull request as ready for review February 15, 2021 07:51
@MartinZikmund
Copy link
Author

🚀🚀🚀
csharpmath

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants