Skip to content

Conversation

@mitchcapper
Copy link

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.UseSpecificDispatcher is 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!

@Juff-Ma
Copy link
Owner

Juff-Ma commented Mar 12, 2024

Thank you for taking a look at this library!
It's incredible seeing something like this as the first PR for the first big public side-project of mine.
This pr is pretty big and i need to inspect it much more closely later but some things already cought my attention.
Spans were a thing i wanted to add myself but the documentation on Spans and P/Invoke is pretty sparse. And currently i didn't find a need for Async methods because most of the methods WebUI provides are more or less async by definition and do not need to be declared on the C# side as such. However one of the key points this library wanted to do was be compatible with NativeAOT, some of your additions seem to rely on Reflection which is one of the main problems with NativeAOT, also you draw in Newtonsoft.Json, a library that is notorious for breaking AOT compatibility. The second thing that cought my eye is that you updated the language version, this was something i specifically avoided and worked around with preprocessor directives because its technically a hack and i wanted to keep this library simple.
I would be very happy if you could clarify on this.

@mitchcapper
Copy link
Author

It's incredible seeing something like this as the first PR

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 :)

This pr is pretty big

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.

the documentation on Spans and P/Invoke is pretty sparse.

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 fixed/unsafe blocks around things rather than having .NET handling the pinning under the hood.

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.

most of the methods WebUI provides are more or less async by definition

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.

However one of the key points this library wanted to do was be compatible with NativeAOT

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.

you updated the language version
worked around with preprocessor directives because its technically a hack and i wanted to keep this library simple.

I think I disagree on the technically a hack aspect. This is a library 99% of the users would consume via a nuget package so how the original c# code was written, or if it required a newer SDK to compile seems like a pretty minimal requirement. .NET has near unlimited side by side support so it is hard to see someone who did need to modify the source being stuck on an older SDK without the ability to SBS a newer one. Still there isn't any specific function critical in newer language versions just generally simpler syntax.

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;000

For 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:
https://github.com/mitchcapper/WebUIAutoBuild/blob/master/WPFSampleApp/MainWindow.xaml.cs#L34-L47

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.

@Juff-Ma
Copy link
Owner

Juff-Ma commented Mar 17, 2024

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.

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.

I love you are using the new P/invoke methodology

This was done specifically to allow for better performance and AOT compatibility. And (if I may say so) it was a headache, the new LibraryImport has very little ressources available and doesn't allow to marshal many types automatically that DllImport allows for.

at this point is most things blocking should be wrapped in tasks

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.

Yes there is a good bit of Newtonsoft Json

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.

I think I disagree on the technically a hack aspect. This is a library 99% of the users would consume via a nuget package so how the original c# code was written, or if it required a newer SDK to compile seems like a pretty minimal requirement.

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

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

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:

  • Use a source generated JSON library (like System.Text.Json) in order to be AOT compatible
  • Don't use reflection, or, if not possible, keep the simpler implementations and mark all the easier to use/reflection based APIs with the correct Attributes (for example RequiresDynamicCodeAttribute) so AOT users know what methods to use instead
  • Do not use a newer C# version. Like I said I think this is a bad practice, at least for libraries. Also only having specific parts of the code use a newer C# version would mean reworking the entire library so it also uses the newest language version. I think being inconsistent with this would make it way harder to maintain.

Some Minor points (found while looking but i don't think would be required to fix):

  • you seem to inconsistently use the old style obj == null and the new best practice obj is null style
  • It seems your editorconfig is causing try-catch-finally block to not wrap correctly

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 LibraryImport. If i ever want to try it again your project will certainly be a helpful reference.

@mitchcapper
Copy link
Author

mitchcapper commented Mar 20, 2024

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.

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.
I was looking at the WebUI source to add the deferral/async JS-> C# calls as it seemed like a pretty common need. Originally in this PR I just stuck a .Wait() as it was the only easy method I saw. I assumed WebUI just spun up new threads for every event to fire them concurrently. Unfortunately I discovered it does not. WebUI cannot fire more than one event at a time, with other events just queuing up.

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.
WebUINetUI.cs is most of the new wrapper code to do 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:

  • Add event listeners of any type, not just click with extended properties
  • Call C# functions from Javascript that are async and get the results (seamlessly, as webui already returned a task this interface is the exact same)
  • Have multiple events fire at once rather than a singular event block the pipeline

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 addRefreshableEventListener function. It takes a query selector string already, and if the user is just using the element ID we can easily translate that into a query string.

@Juff-Ma
Copy link
Owner

Juff-Ma commented Mar 22, 2024

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.

@mitchcapper
Copy link
Author

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:

  • the editorconfig
  • switching to HighPerformance lib and spans and reducing some excess allocations
  • the C# -> JS assist tools for calling JS methods with random args and without worrying about memory allocation
  • optional Interface for using an Invoke dispatcher (but it will require InvokeAsync and returning a Task to help remind users NOT to do things on the event threads directly).
  • throwing exceptions for errors rather than return values
  • NOT including anything using reflection.
  • Maybe a better optional path registration system (Rather than only being able to specify one handle for all Urls it could allow users to do an window.AddHandler(string path,Func<string,string> OnCalled) and maybe an event handler for any Unhandled urls that allows a user to still dynamically implement them.

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.

@Juff-Ma
Copy link
Owner

Juff-Ma commented Mar 23, 2024

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.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants