ApiEventManager Rework and refactoring of task calls

[agracio]

As discussed in #918 this PR features complete rework of ApiEventManager

• rewritten events code using code from ApiEventManager Rework #918.
• simplified multiple task calls using ApiBase.
• added more tests for multiple tasks.

There are many more tasks that could be moved to ApiBase but I will manage those in the future.
Advantage of such move is not only a simplified code that avoids repetition but most importantly thread safety that ApiBase provides.

[FlorianRappl]

Would love to get @softworkz opinion. I think once this is merged we are good for the 0.1.0 ?

[agracio]

Agreed, we need thorough review.

I do not think we have any more items pending for 0.1.0 release.

[softworkz]

Would love to get @softworkz opinion. I think once this is merged we are good for the 0.1.0 ?

I thought similar. I'm pubslihing another beta of ours right now...

.

...and I considered switching our project to the "official" nugets after agracio's changes.

[softworkz]

I'm not sure when I'll have time to review, though, might take a bit..

[agracio]

I'm not sure when I'll have time to review, though, might take a bit..

We are not in a rush. All tests pass and changes are based on the code you provided, but a good review would be appreciated.

[agracio]

@FlorianRappl can we enable tests in Nuke?

[FlorianRappl]

@FlorianRappl can we enable tests in Nuke?

If tests pass, i.e., if we have a baseline that works, yes - I'd love to. Tell me and I make the change.

[agracio]

This PR has all tests passing at least locally, so we are going to find out if GitHub agrees.

[FlorianRappl]

I've integrated the tests in NUKE - we need to find out if that setup works unchanged on the (Linux-based) GitHub action runner, or if we need to change something.

I assume some changes will be necessary (https://github.com/ElectronNET/Electron.NET/actions/runs/19298870273).

Update: Yes, it seems to break essentially everywhere that it can break (https://github.com/ElectronNET/Electron.NET/actions/runs/19299052273). I think this might need more investigation and fixes. Not only Linux is broken, but Windows also has issues (and seems to deadlock / not properly close).

[agracio]

Unfortunately some code behaves differently depending on whether it uses ApiBase for calling tasks or in-line code.
ApiBase has a built-in 1000ms timeout meaning that if a call does not get a response it will fail very quickly.
The calls that do not use it will wait pretty much indefinitely for a callback which is likely what is happening now.

We could add timeouts to tests to try and mitigate the problem.

Adding timeout to CI itself is also a good idea.

[agracio]

Should I add test timeouts to all of them in this PR? There are more tests in this PR than in current develop.

I can also add timeout to GitHub pipeline.

Found solution to OS dependant tests: https://aarnott.github.io/Xunit.SkippableFact/docs/getting-started.html

[FlorianRappl]

The calls that do not use it will wait pretty much indefinitely for a callback which is likely what is happening now.

Yes, that could be.

We could add timeouts to tests to try and mitigate the problem.

Definitely.

Should I add test timeouts to all of them in this PR?

That would be awesome! Much appreciated ❤️

Regarding the [SupportedOSPlatform("Windows")] - I think its a good solution, however, some of these actually indicate that we need to enhance the code (e.g., we should not use System.Drawing). But introducing that for now is the best way forward.

[agracio]

Regarding the [SupportedOSPlatform("Windows")] - I think its a good solution, however, some of these actually indicate that we need to enhance the code (e.g., we should not use System.Drawing). But introducing that for now is the best way forward.

The issue is not limited to System.Drawing I will investigate further but there are multiple items that are not supported on Linux by Electron itself.

[agracio]

I have merged your changes from develop and added the following:

• Added timeout of 5000ms to each test
• Skipped NativeImageTests on Linux
• Added 20 minutes workflow timeout to CI

Good news: Windows is magically fixed itself and completed all tests without errors or hanging. Linux skipped NativeImageTests.

Bad news: Linux appears to be still hanging which likely means that test timeouts were not the only problem. It should timeout workflow at 20 minutes although I have been more than generous, we can make it 10.

I am not entirely convinced about adding .net 10 to project. People who do not have .net 10 SDK installed are unable to work and contribute to the project. While installing it seams trivial, if someone is working from their work PC that has installs locked down it could be a problem.

Would like to hear other people opinions about .net 10 perhaps I am overthinking it.

[agracio]

I test Electron in GitHub CI and for Linux it requires specific setup while both Windows and macOS run without it.
I will try adding it here to see if it solves the problem.

[agracio]

No luck and for some reason one of the tests became flaky on Windows, will try to skip it on CI.

[agracio]

Well now all tests on Windows are flaky although they refuse to fail on my branch CI so not sure what to do.
It appears that tests are at the mercy of a specific runner that picked up the job and fail randomly.

[agracio]

Increased test timeouts to 20000ms, seams to be working.
As for linux I guess the best course of action is to disable tests.

[markatosi]

I am not entirely convinced about adding .net 10 to project. People who do not have .net 10 SDK installed are unable to work and contribute to the project. While installing it seams trivial, if someone is working from their work PC that has installs locked down it could be a problem.

Would like to hear other people opinions about .net 10 perhaps I am overthinking it.

In my opinion you are overthinking this. Unless there is a technical issue due to breaking changes between 8.0 and 10.0 which can't be worked around you should prefer to be as current as possible with the SDKs which are supported.

In regard to your point about people not having 10 installed being unable to contribute to the project I would ask you who these people are and just how many of them are there? Unless you can point to people who are currently actively contributing and assess the level of impact on the project if they somehow found it impossible to install dotnet 10 and continue their labors I think you should not worry much about potential future contributors.

Will some of the new users of Electron.Net become contributors? Probably. If they choose to do so I doubt very much any of them are going to complain that the project is up to date on it's support of the latest .net version.

As a business owner if some of my employees are contributing to Electron.net on their work computer that means a few things.

1. That work is in furtherance of defined business goals.
2. The work that they are engaging in is or will be a net positive to the cash flow of the business.
3. All tools will be provided to them to achieve their tasks. I.E. there won't be some fence installed preventing them from using whatever SDK is supported by Electron.net.

That's my two cents :-) Thank you for all your hard work.

[FlorianRappl]

Yeah let's skip the tests on Linux. I'll add a condition to the runner once we merged. Thanks for the superb work!

With respect to the SDK - yeah I am also not 100% convinced. I don't want to force anyone to install the most recent SDK, but I think having the global.json with the SDK choice is better than without. That being written it should be possible to just change it locally to the previous 8.x version and have it working. Having the most recent LTS SDK as default still makes sense imho.

[FlorianRappl]

The issue is not limited to System.Drawing I will investigate further but there are multiple items that are not supported on Linux by Electron itself.

Yes I know. Hence the "e.g." - it's multiple things but I am mostly thinking about the stuff we use / reference that could be improved / replaced / skipped. All in all keeping the cross-platform dream alive :).

[agracio]

Unfortunately I managed to make a real mess when merging your changes from develop, code should look fine but commit history is a mess.

[FlorianRappl]

Unfortunately I managed to make a real mess when merging your changes from develop, code should look fine but commit history is a mess.

Don't worry about it. It does not matter how the bricks have been carried, it's only important that they are in the right place in the end.

[FlorianRappl]

LGTM - much appreciated 🚀 !

[softworkz]

I'm not sure when I'll have time to review, though, might take a bit..

We are not in a rush. All tests pass and changes are based on the code you provided, but a good review would be appreciated.

Hey @agracio - sorry for getting back late, I've been short on time this week. But yesterday I came to look through your changes and I got not much more to say than: well done!
Thanks a lot for your contribution, this has been an important and very valuable step towards achieving a better and moe professional code base. 🚀

You'll also see that that none of my 8 new PRs from today are making substantial changes to yours, I'm rather building up on it, like expanding on SkipableFact and SupportedOSPlatform attributes and all fits together very well.
If you'd want me to say anything suggestive, it'd be that a constant would be nice for the test timeouts rather than repeating the same number 70 times. But it can still be done, maybe alongside a common base class for the test classes, but that'd be a future improvement.
Oh, and in both PRs, you had committed a lot of trailing whitespace, but that's your editor - probably you weren't even aware of it. As I'm also a contributor to FFmpeg - the champions league of nitpicking - I can't fail to notice, but for that matter it actually makes sense as it makes it hard to read submissions from other developers, when their PRs are full of whitespace changes (like the PR from Dennny after your first one). Anyway, you don't need to worry - I've added a GH action which checks PRs for this and tells you exactly which lines are affected.

For the Linux test on CI, you were right to let it go. There were quite a few things missing but it's working now (+ macOS and also locally on WSL).

So, everything is going in a very good direction, thanks a lot for being part of it!