api/dotnet: Add internal Slint Runtime Library for .NET API #3913

microhobby · 2023-11-12T18:06:16Z

Contributing back the work from https://github.com/microhobby/slint-dotnet

CLAassistant · 2023-11-12T18:06:21Z

All committers have signed the CLA.

ogoffart · 2023-11-13T09:16:30Z

Thanks a lot for the PR! 🤩.
Unfortunately I'm traveling this week so it'll take a bit of time before I can spend enough time to review it.

As for the CI errors: the problems on the mac about the new headers can be ignored. This is a temporary failure.
The problem with the license header in the docs we can adjust the license check to accept it.

The problem in rnet about transmute of TypeId seems more worrisome. TypeId changed its size a few rust version ago. So this seems to be a problem in the rnet crate.

microhobby · 2023-11-13T14:54:53Z

@ogoffart

Thanks a lot for the PR! 🤩. Unfortunately I'm traveling this week so it'll take a bit of time before I can spend enough time to review it.

As for the CI errors: the problems on the mac about the new headers can be ignored. This is a temporary failure. The problem with the license header in the docs we can adjust the license check to accept it.

It's ok, I will remove my, I'm ok to be a Slint Developer 😄

The problem in rnet about transmute of TypeId seems more worrisome. TypeId changed its size a few rust version ago. So this seems to be a problem in the rnet crate.

yeah, this one also got me because I do not check first on the project and appears to be abandoned. I will engage with the maintainer to see the status. Thanks

microhobby · 2023-11-15T23:06:39Z

Ok, the stable has been passed, but you guys are also compiling it against the v1.70. I need to also check this.

Signed-off-by: Matheus Castello <[email protected]>

microhobby · 2023-11-16T02:12:15Z

ok, now all working as expected. I need to work now on the .NET C# specific stuff.

tronical · 2023-11-16T03:13:17Z

api/dotnet/src/lib.rs

+
+ let mut ret: Vec<DotNetValue> = Vec::new();
+
+ CURRENT_INSTANCE.with(|current| {


The use of thread locals is something I find worth improving. It would be better if the instance could be wrapped in an opaque type and become a parameter to all the functions (and return value for the initial loading).

As far as I can see rnet support Box and Arc wrapped types. Is this something you’d like to pursue?

tronical · 2023-11-16T03:15:23Z

api/dotnet/Cargo.toml

+crate-type = ["lib", "cdylib", "staticlib"]
+
+[dependencies]
+async-std = { version = "1.10.0" }


You can also use the more lightweight spin-on crate for this (block_on). We use that in other places as well.

tronical · 2023-11-16T03:24:19Z

For the record: while there's a lot that can be done (thread local, CI, etc) , I'd be fine if we decide to take this in early and incrementally improve.

ogoffart

I just went quickly through the code and made some comments.
As simon said, I don't think it needs to be perfect to be merged, and we can iterate over it within the master branch as beta.
But even in the beta stage, this is not so useful without the C# code and some C# examples/docs.
So perhaps this should be added at the same time.

ogoffart · 2023-11-17T11:34:28Z

api/dotnet/build.rs

+// Copyright © SixtyFPS GmbH <[email protected]>
+// SPDX-License-Identifier: GPL-3.0-only OR LicenseRef-Slint-Royalty-free-1.1 OR LicenseRef-Slint-commercial
+
+fn main() {}


No need to have a build.rs file if it doesn't do anything. (just need to be removed from Cargo.toml as well

ogoffart · 2023-11-17T11:36:04Z

api/dotnet/src/lib.rs

+use std::{path::Path, time::Duration};
+
+use rnet::{net, Delegate0, Net};
+
+use i_slint_core::api::{ComponentHandle, Weak};
+
+use i_slint_core::graphics::Image;
+
+use i_slint_core::timers::{Timer, TimerMode};
+
+use slint_interpreter::{ComponentInstance, Value, ValueType};
+
+use i_slint_compiler::langtype::Type;


Style: IMHO it would looks better without all the new line.

ogoffart · 2023-11-17T14:29:02Z

api/dotnet/src/lib.rs

+ // reject modernity, back to the monke
+ let weak_ref = unsafe { MAIN_WEAK_INSTANCE.take().unwrap() };
+ unsafe {
+ MAIN_WEAK_INSTANCE = Some(weak_ref.clone());
+ };
+
+ weak_ref
+ .upgrade_in_event_loop(move |_| {
+ callback.call();
+ })


You can use slint_interpreter::invoke_from_event_loop , no need to have a weak_ref. That way you can get rid of this MAIN_WEAK_INSTANCE entirely

ogoffart · 2023-11-17T14:29:48Z

api/dotnet/src/lib.rs

+
+rnet::root!();
+
+macro_rules! printdebug {


do you want to keep this macro in released code?

ogoffart · 2023-11-17T14:33:14Z

api/dotnet/src/lib.rs

+
+ let m_calls = ret_handle.callbacks().collect();
+
+ let tokens = Tokens { props: m_props, calls: m_calls };


we should probably keep the ret_handle instead of letting it go out of scope.

ogoffart · 2023-11-17T14:34:01Z

api/dotnet/src/lib.rs

+pub fn interprete(path: &str) -> Tokens {
+ let mut compiler = slint_interpreter::ComponentCompiler::default();
+ let path = std::path::Path::new(path);
+ let ret_handle = async_std::task::block_on(compiler.build_from_path(path)).unwrap();


instead of unwrap and panic, we should return an error to the caller.

ogoffart · 2023-11-17T14:35:22Z

api/dotnet/src/lib.rs

+ let val_type;
+ let mut val_struct = false;
+ let mut val_props = Vec::new();
+ let val_val = format!("{:?}", "");


Is that just "".to_string or String::new()?

ogoffart · 2023-11-17T14:35:50Z

api/dotnet/src/lib.rs

+ for prop in props {
+ let p_name = prop.0;
+ let p_type = prop.1;


style suggestion:

Suggested change

for prop in props {

let p_name = prop.0;

let p_type = prop.1;

for (p_name, p_type) in props {

microhobby marked this pull request as draft November 12, 2023 18:06

microhobby force-pushed the castello/dotnet branch 3 times, most recently from c089b73 to ba3353f Compare November 13, 2023 04:08

microhobby mentioned this pull request Nov 13, 2023

Maintainence Status Diggsey/rnet#4

Closed

microhobby force-pushed the castello/dotnet branch from ba3353f to 5d09110 Compare November 15, 2023 18:24

api/dotnet: Add rust internal Slint Runtime Library for .NET API

c686769

Signed-off-by: Matheus Castello <[email protected]>

microhobby force-pushed the castello/dotnet branch from 5d09110 to c686769 Compare November 15, 2023 23:59

tronical reviewed Nov 16, 2023

View reviewed changes

ogoffart reviewed Nov 17, 2023

View reviewed changes

Vadoola mentioned this pull request Dec 21, 2023

C# binding #4207

Open

ogoffart linked an issue Jan 15, 2024 that may be closed by this pull request

C# binding #4207

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

api/dotnet: Add internal Slint Runtime Library for .NET API #3913

api/dotnet: Add internal Slint Runtime Library for .NET API #3913

microhobby commented Nov 12, 2023

CLAassistant commented Nov 12, 2023 •

edited

ogoffart commented Nov 13, 2023

microhobby commented Nov 13, 2023 •

edited

microhobby commented Nov 15, 2023

microhobby commented Nov 16, 2023

tronical Nov 16, 2023

tronical Nov 16, 2023

tronical commented Nov 16, 2023

ogoffart left a comment

ogoffart Nov 17, 2023

ogoffart Nov 17, 2023

ogoffart Nov 17, 2023

ogoffart Nov 17, 2023

ogoffart Nov 17, 2023

ogoffart Nov 17, 2023

ogoffart Nov 17, 2023

ogoffart Nov 17, 2023


		let mut ret: Vec<DotNetValue> = Vec::new();

		CURRENT_INSTANCE.with(\|current\| {


		let m_calls = ret_handle.callbacks().collect();

		let tokens = Tokens { props: m_props, calls: m_calls };


		rnet::root!();

		macro_rules! printdebug {

api/dotnet: Add internal Slint Runtime Library for .NET API #3913

Are you sure you want to change the base?

api/dotnet: Add internal Slint Runtime Library for .NET API #3913

Conversation

microhobby commented Nov 12, 2023

CLAassistant commented Nov 12, 2023 • edited

ogoffart commented Nov 13, 2023

microhobby commented Nov 13, 2023 • edited

microhobby commented Nov 15, 2023

microhobby commented Nov 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tronical commented Nov 16, 2023

ogoffart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CLAassistant commented Nov 12, 2023 •

edited

microhobby commented Nov 13, 2023 •

edited