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

DBus Codegen #9810

Merged
merged 41 commits into from
Feb 21, 2023
Merged

DBus Codegen #9810

merged 41 commits into from
Feb 21, 2023

Conversation

affederaffe
Copy link
Contributor

@affederaffe affederaffe commented Dec 29, 2022

What does the pull request do?

Use Tmds.DBus.Protocol directly instead of relying on the System.Reflection.Emit backend.
This will greatly help with trimming and in aot-scenarios.
Note that it's currently using a submodule (hence WIP) since a) Tmds.DBus.Protocol isn't released on NuGet yet and b) sending large buffers is currently broken (e.g. sending tray icon images)

What is the current behavior?

Usage of the S.R.E backend, which breaks when using trimming/aot.

What is the updated/expected behavior with this PR?

Trimming/Aot should work.

How was the solution implemented (if it's not obvious)?

Checklist

Breaking changes

None(?)

Obsoletions / Deprecations

None

Fixed issues

#2359
#10069

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0027960-beta. (feed url: https://pkgs.dev.azure.com/AvaloniaUI/AvaloniaUI/_packaging/avalonia-all/nuget/v3/index.json) [PRBUILDID]

@kekekeks
Copy link
Member

Are you using some codegen tool to generate implementations? If so, it would be way better to have it as a Roslyn Generator.

@affederaffe
Copy link
Contributor Author

Are you using some codegen tool to generate implementations? If so, it would be way better to have it as a Roslyn Generator.

I'm currently working on a proper roslyn code generator for this purpose, but a first proof-of-concept I used the Tmds.DBus.Tool dotnet tool.

# Conflicts:
#	src/Avalonia.FreeDesktop/DBusMenu.cs
# Conflicts:
#	src/Avalonia.FreeDesktop/DBusIme/Fcitx/FcitxX11TextInputMethod.cs
#	src/Avalonia.FreeDesktop/DBusMenuExporter.cs
#	src/Avalonia.FreeDesktop/DBusTrayIconImpl.cs
@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0028463-beta. (feed url: https://pkgs.dev.azure.com/AvaloniaUI/AvaloniaUI/_packaging/avalonia-all/nuget/v3/index.json) [PRBUILDID]

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0028472-beta. (feed url: https://pkgs.dev.azure.com/AvaloniaUI/AvaloniaUI/_packaging/avalonia-all/nuget/v3/index.json) [PRBUILDID]

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0028634-beta. (feed url: https://pkgs.dev.azure.com/AvaloniaUI/AvaloniaUI/_packaging/avalonia-all/nuget/v3/index.json) [PRBUILDID]

affederaffe referenced this pull request in affederaffe/BeatSaberModManager Jan 19, 2023
@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0028831-beta. (feed url: https://pkgs.dev.azure.com/AvaloniaUI/AvaloniaUI/_packaging/avalonia-all/nuget/v3/index.json) [PRBUILDID]

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0028872-beta. (feed url: https://pkgs.dev.azure.com/AvaloniaUI/AvaloniaUI/_packaging/avalonia-all/nuget/v3/index.json) [PRBUILDID]

// Connect synchronously
conn.ConnectAsync().Wait();
conn.ConnectAsync().GetAwaiter().GetResult();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make TryInitialize async perhaps? To avoid this code smell?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That'd either require the usage of an async void method (which, at least in my opinion, isn't any better), or making the complete initialization process async as well. While I'd like to see the latter at some point since nearly everything is async today anyway, it's probably out of scope for this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh cool cool, the latter is a good thing to do afterwards 👍

@maxkatz6
Copy link
Member

maxkatz6 commented Feb 8, 2023

@kekekeks are we good to merge it with submodules, or should we wait until it's available on the nuget?

@kekekeks
Copy link
Member

kekekeks commented Feb 8, 2023

We need to somehow internalize all public types from DBus libs. Otherwise there will be name conflicts later.

@affederaffe
Copy link
Contributor Author

Tmds.DBus.Protocol is probably going to be published on NuGet "earyl next week", see tmds/Tmds.DBus#79 (comment).
Once that's done I'll adapt the source generator to use the package and try to upload it to NuGet too (in case I can figure out how to sign a package by then)

@maxkatz6
Copy link
Member

@affederaffe in general I think it's enough to create a key and sign from the Visual Studio project properties page.
Other than that, you can set AssemblyOriginatorKeyFile and SignAssembly properties like here https://github.com/tmds/Tmds.DBus/pull/82/files#

Not sure what's advantage of adding PublicSign property though.

affederaffe and others added 5 commits February 20, 2023 16:39
@maxkatz6
Copy link
Member

I don't quite understand why and how render tests are failing on this PR for all platforms.

@maxkatz6 maxkatz6 merged commit 657c544 into AvaloniaUI:master Feb 21, 2023
@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0030315-beta. (feed url: https://pkgs.dev.azure.com/AvaloniaUI/AvaloniaUI/_packaging/avalonia-all/nuget/v3/index.json) [PRBUILDID]

@kekekeks
Copy link
Member

kekekeks commented Mar 7, 2023

After this PR dbus callbacks are executed on some random thread instead of dispatcher one

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.

6 participants