-
Notifications
You must be signed in to change notification settings - Fork 3
Csharpify this lib #1
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
base: main
Are you sure you want to change the base?
Conversation
…ized code flow paths, performance optimizations See PR Juff-Ma#1 for more details
|
Thank you for taking a look at this library! |
It made me smile when I went to predict what the PR # would be to note it in the commit message only to realize it would be #1 :)
Sorry, this was kind of swing for the fences in a few hours time to see just how simple of a wrapper I could make. I tweaked things along the way as well so I ended up backing out the first few commits I did and cherry picked code from scratch at the end to make new.
Somewhat, I love you are using the new P/invoke methodology (even though it also meant writing it twice for for older .net). Cswin32 has a good bit and even some of the native interop code is certainly moving to support Spans directly. Here though I shied away from actually using it at the extern level, still using pointers. There isn't really a performance downside, Spans here are essentially representing the block of standard allocated memory. There isn't any copying going on, it just results is the slightly less elegant I like to think of Span's as just Views into the underlying datatype as they don't really manage any resources on their own. They do allow us to use raw memory in more managed contexts where we would normally have to copy it to get the same abilities.
A bit. It is certainly more of an event-ish based system but things like executing JS, or navigation, etc shouldn't have non async based calls. Best case people are just a bit confused about blocking/synchronous calls to clearly async actions. One of the signatures of C# at this point is most things blocking should be wrapped in tasks. Javascript itself can easily have multiple events and other things firing at once the idea your main thread could be blocked on a JS promise is a bit odd. Yes you could spin up a new thread for every event / call / etc but thats onerous and error prone. To a developer it may be more straight forward if the library however presents a normal async interface. I like what you did adding normal event handlers on the base class for example. For other things though the idea of getting an event ID number that you hold on to and check in a global event message loop to see if an event is for you feels a bit awkward and like you are writing the direct pump message loop. Even if you simply wrapped things is Task.CompletedTask returns at this point I think it is beneficial to present Task based from the start as it gives you flexibility. It is very hard for a library to just switch a function from sync to async so then you end up with BindAsync, RunAsync, ScriptAsync and this is not 2012.
Yeah not something I knew/considered in this PR. Yes there is a good bit of Newtonsoft Json but this could be moved to an extension library to let users choose between saving 800KB and pain or not. I also think that I would move to class based Binds into JS land rather than function based. This would make it much easier to apply one attribute to the class and just assume all public methods need type information preserved for tree shaking and reflection support. It could pretty easily to be moved to another JS library (if System.Json was much butter for example) and the reflection calls themselves get very basic information. Heck even if you just returned a simple JSON reader that let you access properties/data like JObject without all the deserialization I think that would be a benefit for people.
I think I disagree on the IlSpy or similar will happily auto-translate most c# code down to w/e old language version you would like. At the end of the day it is your project and I am happy to follow whatever direction you want to go on it and will attempt to update the PR to meet that. I think for most developers though the easier you make this library to use and the more closely it follows a pattern they are familiar with the far greater the uptake will be. I hope the library succeeds very well as I find the concept behind WebUI pretty killer. If you can combine the near non-existent footprint without giving up most of the niceties (or at least having another nuget to add them: )) by all means. Hopefully more usage = more popularity = more contributions as this concept far beats of many other multi-platform UI frameworks. Unrelated but if you were looking for some more drastic changes;000For reference I also tried to fully auto-gen this API. I don't know if you are interested in doing so, clearly WebUI's interface isn't very large. Still I like the idea of auto-generating off the base library and then enhancing around that (rather than the kind of mix here with some things following the standard API and others not). Changing the C to c# was easy enough for it, but with some rule tweaking I was able to coerce it into auto-generating the Window and Event classes similar to a c# first API might be. It also makes it so you can use the native WebUI bind, etc calls rather than the interface versions. You can see it at: https://github.com/mitchcapper/WebUIAutoBuild . I did at as a separate repo just for test and have no plans maintaining it separate. If you have interest in having the webui->C# conversion be automated happy to put it into a PR. CppSharp is controlled through CS itself, the CodeGen project there with https://github.com/mitchcapper/WebUIAutoBuild/blob/master/CodeGen/WebUIGenerator.cs doing the configuration. The generated output is at https://github.com/mitchcapper/WebUIAutoBuild/blob/master/Generated/WebUILib.cs . It looks a bit ugly but its not inefficient or hard to understand. It is autogenerated so it isn't like you are expected to look at it. Here is some sample of what the resulting code looks like in use, which I thought came off pretty clean: It was done quickly so If you are interested I would definitely do some more changes. It has some things as properties that really should be function calls IMOP for example. Still being able to use Window.SetIcon or not having to worry about nearly all the native marshalling is nice. I did have to hand code the constructor and a wrapper around the bind delegate as I couldn't get CppSharp to do it for me, but it is pretty minimal and built on their standard calls: https://github.com/mitchcapper/WebUIAutoBuild/blob/master/WebUILib/WebUIClassAdditions.cs Then for much a few of the assist tools I did in this PR I figured I would test with. I moved them to simply be extensions on Window however: https://github.com/mitchcapper/WebUIAutoBuild/blob/master/WebUILib/WindowExtensions.cs the experience for users is nearly the same, but they could easily be stuck in a separate lib this way. Again it was a quick hack test to see what automating the API would look like but something I found interesting. |
You don't have to be sorry, I had some more time to look more deeply into this PR now and think it would be a welcome addition to this project. However some parts i think will still need rework.
This was done specifically to allow for better performance and AOT compatibility. And (if I may say so) it was a headache, the new
The main thing that confused me about WebUI was that it's methods weren't blocking so I thought it would be unnecessary. However I think you are right to be better safe than sorry.
As you already suggested i think all of this code yould be replaced with System.Text.Json, and in fact i think this would eliminate the JSON problem as System.Text.Json allows for source-generated (de)serializers.
It isn't about the SDK aspect, in fact, WebUI.NET requires the newest .NET 8 SDK even if not compiling for .NET 8. This is about the compiler errors and warnings you get when using unsupported features. I considered using PolySharp in the beginning but didn't go that step because it's technically unsuported by microsoft and the .NET team. Using a newer language version than the default can lead to using features that will seem to work but cause unexpected and hard to diagnose errors. So while most of the new features will work some probably wouldn't, and nothing is as stupid as trying to diagnose an error in a project and finding out it's an error in the library you use that could have been solved by 1 or 2 lines of code and just seemed to be ignored because it was just a bad practice but not a requirement (I was in this situation recently). /rant off
You are correct about this and this is why i think your PR will be a welcome addition. However shipping it like this would be causing problems, at least in my opinion. Some points i think would need to be changed in order for this to work better in the projects current state:
Some Minor points (found while looking but i don't think would be required to fix):
I would really like to help with this PR because it would greatly improve the usability and propably performance of this library but currently I am working on another (non oss) project and don't have that much time to work on this. (however this library is certainly not abandoned as I use it in the mentioned project) Regarding your CppSharp project, this library once was using CppSharp (before being on Github and before the git repo was even created locally) but i couldn't get it to work correctly and at one point I just wrote the bindings manually. This also allowed me to have more control about how the wrapper interfaced with the native code and more easily implement the new |
22c16ed to
3b8b18e
Compare
3b8b18e to
85ef6eb
Compare
Luckily I have not run into that problem with any libraries myself but happy to switch back 5 versions to C#7. I need you to review the new system I just pushed here before proceeding with most of your requested changes. This is problematic as without async support for c# bindings developers will be tempted to do whatever sync code they can in those callbacks which will further worsen the experience due to the queue system. To fix this we needed to essentially implement our own binding protocol and this is what most of the last commit here is. webui_net.js is the additional JS to support this. As we were doing function binding while events don't need to support async tasks (as webui doesn't let you edit the event properties anyway) I wrote an event binder to go with the new code. This allows you to bind to any event not just click events, and to set extended event properties (like abort controllers). Now you can see in my sample the including of the webui_net.js file and in WebUINetUI registering a handler for that file. We could avoid that by just injecting it as a text string in the onConnect function. I need this reviewed before I perform the cleanup and refactoring as its a major departure from before and the refactoring would use some components of this. This also has the nice side effect of fixing the problem where registered functions did not show up in the global / webui namespaces. Right now we only register functions globally but it may be a good idea to stick them into the webui namespace incase someone expects it there too. With this set of changes you can:
Assuming we go forward with this I would also add an AddEventListener function that allows you to pass a query selector. Add an option for event listener additions to automatically apply to matching new dom elements. We can do this pretty simply by hijacking the webui |
|
To be clear, i think this is too much. WebUI.NET wants to be a wrapper of the native WebUI library with some syntactic sugar that will make it easier to work with. The original library already was (in my opinion) much more high level than any other available wrapper for WebUI. I could get behind your initial additions like automatic argument serialisation but i think that this is too much. I don't want this to become a whole library on top of WebUI itself, and with a whole binding system that is what your pr currently seems to become. I don't think that such things need to be in WebUI.NET itself, i still think that your additions are good but currently they seem to get kinda out of the scope of this project. If the upstream WebUI project were to add such features like asynchronous binding that of course would change but currently it doesn't seem like they plan to do that. To summarize: I think you're taking this PR out of scope and while I do like the additions of this PR, WebUI.NET was kinda never intended to do these things. I don't want to take make this a full blown UI framework but rather a simple framework intended for everyone to build his own logic on top of. Before the addition of the new binding system i still could see this being a part of WebUI but I personally don't want to maintain a fully custom javascript binder for a library that (while it has flaws) already has one. With those changes it would propably be required to spin off a whole new library basd on WebUI.NET becausee i just don't fit such a change fits in this project. The main goal of this project was to be a simple, portable, low requirement UI kit and all of this stuff was handled by WebUI and WebUI.NET could just focus on being a nice Wrapper with some syntactic sugar. And to be real, I don't have the experience neccesary to maintain this code, this is my 3rd big C# project, the second one that isn't completely terrible and the 1st one that i think is actually good in its current state and having to maintain my own binding system would propably lead to me abandoning this project because it would grow over my head. To be clear once again, I am really happy to see someone other than myself working on this but a whole binding system just for the sake of having an async UI is propably not the best choice for this project (at least in my opinion) because if you want a really advanced C# UI system you're propably gonna choose Avalonia, Uno or maybe Eto. This is not a framework I see being used in giant projects but rather quick side projects that need a good-looking UI or projects that are already web-based and want to add a native backend. If you still want to take your PR further down this road i will be happy to help but i don't think i could merge it into this repo. |
|
Understood and valid points. I love the simplicity of WebUI and certainly don't want to go investing a whole UI library ontop of it. I think the idea of being able to make JS->C# calls that can happen asynchronously is a key feature of what many projects one might use this for would want to do. Of course the dev can implement it themselves but 1) They can't even pass a callback to support the ugly pre async/await JS style of asynchronous code. 2) It is complicated and far from seamless to do. So I will change things up. I submit some PR for the following:
I will then setup on a separate repo a WebUI.NET extension library. Adding the Async support, full event binding, reflection based method binding and maybe a POC for whole class based binding. You can review and see if you want to add it here or not. If so you can certainly do it as a separate CS Project/nuget package so users don't get it by default. At the same time I will submit the JS binder up to upstream to see if they want to go down that route. It is an annoyance that JS->native calls already use promises but due to how WebUI is implemented I think a JS binder is the only way to actually do it without them making massive changes. As said, I have no plans to maintain a separate webui lib so if you dont want the other additions (or only some of them) whatever else can just sit there as reference, with maybe a note in the readme for anyone looking for an idea on how to do so. If that sounds good ill go ahead and refactor it out. Would do things like the path reg system as a separate PR as well. |
|
If you do make these changes, I will propably be able to merge it into a new branch, I will propably not merge it into the main branch yet but maybe I will create an experimental nuget to test things out. And as I said, if you want to implement this stuff in another repo I will take a look at it and help where possible, i just don't think the whole async binding system currently fits in this library directly. If upstream implements it that would of course reduce the work required to implement it and it would of course be highly likely to be implemented here too. |
Thanks for putting this together, WebUI looks interesting, hopefully this makes it friendlier to use. Sorry for such a large set of changes but they kind of stack.
Added to match the normal API calls (not rename their function):
public unsafe bool Script(string javaScript, uint timeout_secs, Span<Byte> buffer)Use Span for better memory efficiency and less allocation.
Use a memory pool for alleviating the need for the library consumer to handle buffers if they do not want to example:
public async Task<T?> ScriptEvaluate<T>(string javascript, TimeSpan? timeout = null, bool? stringReturnsAreSerialized = null)That function also handles basic de-serialization to support various return types.
Moved this and future calls to use Task, even i the native library doesn't yet have async style calls we can still run on background threads and avoid UI wait.
Simplified event binding to bind click events:
public void RegisterOnClick(Action<string> OnClick, String domId)Added direct method binding with:
public void RegisterBoundFunction(LambdaExpression methodExpression, String OverrideRegisteredName = null, object OverrideInstance = null)This is a heavier function. It allows you to bind any static or instance method into the Javascript namespace (at least it should, I don't get why the interface bind calls only are accessible via the "webui.call" functionality). If this isn't fixed in the base library can work around just be creating our own JS alias. It automatically will attempt to convert all args to the correct format, and handles missing args properly. It supports binding an async function and will properly wait for you to return. Note this does result in an ugly .Wait() call in the code, but as webui doesn't have any deferrals yet for return values I don't think there is another great option. We could design all callbacks to run on a dedicated BG thread potentially if needed.
It takes Lambdas to allow for a very easy way to specify both the method and instance without having to use reflection as the caller.
Added calling javascript methods passing the args separately. I noticed there is the raw function but I didn't dive into exactly how all the raw data would have to be presented. This internally converts the call to a string, but it does it fairly robustly. That code is 100% not mine, just the great work of @amaitland from https://github.com/cefsharp/CefSharp the license is on the helper class page. If for some reason you want to rip that out you can throw away the class and rewrite the functions to do it another way (or just take a full string call again).
public async Task<T?> ScriptEvaluateMethod<T>(string javascriptMethod, params object[] args)This also automatically serializes the JS responses and can deserialize them on the c# side allow for complex arbitrary types to be passed. On the c# side my example uses a static class definition but if you just do
ScriptEvaluateMethods<JObject>you can walk the JS like normal.I'd suggest seeing what is required to get listed on the official site for the c# package. Looking at the other language libraries I think these changes make it one of the most advanced wrappers / friendly options available. I would try to stick to official function naming for the common calls but then as you add additional user helpers you can use different naming that seems more natural. This allows easy porting between WebUI languages.
Assume calls succeed throw exceptions if they don't.
Added a WPF demo project showing these features off. Note the
WebUI.Window.UseSpecificDispatcheris a bit clunky. Could be done automatically when running with UI (and far easier if we can just assume we can access Application.Current.Dispatcher as we can use the invoke methods there and store the dispatcher in the constructor.Hopefully these building block wrappers and tools can increase performance and make it easier for people to use the library. I love the low overhead idea of WebUI. While the API is also very basic because of it, hopefully this can show how rich experiences can be done with just wrapping those basic calls.
I would suggest using project references rather for WebUI .NET lib rather than the nuget style you were doing for examples. Will make debugging and editing code much faster.
Future considerations:
Using a JS Proxy class one could bind entire C# classes into the javascript namespace with decent ease. The proxy class would just change any calls to a middleman script (ie say webui.call("proxy_helper",bound_class_name,bound_func_name,args...)). That C# function would just use reflection to find the function on the class and convert the args as I currently do.
Could easily add binder for any HTML eventListener with just some wrapped JS to be able to bind to. Given the idea of this being used as a generic UI interface having only a 'click' handler is a bit silly.
Good luck, hope you continue to maintain this!