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

Bump DBus stack take 2 #15685

Merged
merged 25 commits into from
Aug 19, 2024

Conversation

affederaffe
Copy link
Contributor

@affederaffe affederaffe commented May 10, 2024

What does the pull request do?

Bump Tmds.DBus.Protocol and Tmds.DBus.SourceGenerator.
This uses the "official" Variant system instead of the SourceGenerator provided one (which was removed).
Accept the explicit ignoration of the DBus spec and assume collapsed variants.

What is the current behavior?

What is the updated/expected behavior with this PR?

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

Tmds.DBus.SourceGenerator.PropertyChanges`1 was made internal by Tmds.DBus.SourceGenerator

Checklist

Breaking changes

Obsoletions / Deprecations

None

Fixed issues

#15655

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0048326-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@maxkatz6 maxkatz6 requested a review from jmacato May 11, 2024 02:45
@maxkatz6 maxkatz6 added os-linux backport-candidate-11.1.x Consider this PR for backporting to 11.1 branch and removed backport-candidate-11.1.x Consider this PR for backporting to 11.1 branch labels May 13, 2024
@maxkatz6
Copy link
Member

@affederaffe since this PR has breaking changes, you need to run nuke ValidateApiDiff command locally (nuke tool needs to be installed, alternatively, you can run _build project with the same command directly).
As it looks like, some autogenerated dbus files were made internal, as they should be.

@affederaffe
Copy link
Contributor Author

Only the PropertyChanges type was made internal as it should be, it was a mistake to make it public at first.
It is safe to make it internal as it is unusable by itself, all members that use this type (i.e. proxies with WatchPropertyChangedAsync) are already internal, so there cannot be any users of this api anyway.

@jmacato
Copy link
Member

jmacato commented Jul 21, 2024

@affederaffe we still need to run the API validation regardless as it's a hard requirement for making stable builds at the moment

@affederaffe
Copy link
Contributor Author

Avalonia.FreeDesktop.nupkg.xml

<?xml version="1.0" encoding="utf-8"?>
<!-- https://learn.microsoft.com/en-us/dotnet/fundamentals/package-validation/diagnostic-ids -->
<Suppressions xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:xsd="http://www.w3.org/2001/XMLSchema">
  <Suppression>
    <DiagnosticId>CP0001</DiagnosticId>
    <Target>T:Tmds.DBus.SourceGenerator.PropertyChanges`1</Target>
    <Left>baseline/netstandard2.0/Avalonia.FreeDesktop.dll</Left>
    <Right>target/netstandard2.0/Avalonia.FreeDesktop.dll</Right>
  </Suppression>
</Suppressions>

@maxkatz6
Copy link
Member

maxkatz6 commented Jul 26, 2024

@affederaffe @jmacato just updated api baseline by running "nuke" tool in the solution root.
@affederaffe you can just push these nupkg.xml file changes, when it is expected to break public API.

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0050468-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0050553-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@kekekeks
Copy link
Member

Sorry, missed the PR, will test tomorrow.

@kekekeks
Copy link
Member

As of the public API, can we have source-generated classes to be private?

@affederaffe
Copy link
Contributor Author

As of the public API, can we have source-generated classes to be private?

How could they be private?
The private modifier only makes sense for nested classes, but for that we'd need an explicit entrypoint (e.g. marker attribute) on a class. Which is only possible for proxies, and even then, when using the proxy multiple times, each would have its own private class.

@maxkatz6
Copy link
Member

@affederaffe private for external consumer. I.e. internal is fine.

@affederaffe
Copy link
Contributor Author

That makes more sense.
In this case, every source-generated class is already internal and thus inaccessible for users.

@kekekeks
Copy link
Member

Why do we need

    <DiagnosticId>CP0001</DiagnosticId>
    <Target>T:Tmds.DBus.SourceGenerator.PropertyChanges`1</Target>
    <Left>baseline/netstandard2.0/Avalonia.FreeDesktop.dll</Left>
    <Right>target/netstandard2.0/Avalonia.FreeDesktop.dll</Right>
  </Suppression>

then?

@maxkatz6
Copy link
Member

@kekekeks it wasn't internal before this PR

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0051285-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@affederaffe
Copy link
Contributor Author

Sorry, missed the PR, will test tomorrow.

@kekekeks Any updates on this?

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0051341-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@kekekeks
Copy link
Member

DBus menu works now with KDE without crashes, yes

@kekekeks
Copy link
Member

I think at some point we might want to switch to using libdbus by adapting your generators. It wont be a 100% managed solution, but at least libdbus is pretty much guaranteed to follow the spec.

@jmacato jmacato added this pull request to the merge queue Aug 19, 2024
Merged via the queue into AvaloniaUI:master with commit 1d9a018 Aug 19, 2024
10 checks passed
@tmds
Copy link
Contributor

tmds commented Sep 3, 2024

I think at some point we might want to switch to using libdbus by adapting your generators. It wont be a 100% managed solution, but at least libdbus is pretty much guaranteed to follow the spec.

My 2 cents.

VariantValue isn't meant to be a match with wire-representation of a variant. It is a convenience type that is meant for reading variant values. When you are reading those values, the UX is nicer if the variants are unwrapped.

I think that VariantValue probably works very well for your use-cases, but that you got caught/surprised by the difference in behavior with what you had before.

If you prefer a different behavior, you can still use the lower-level APIs and build your own Variant representation as the SourceGenerator was doing before.

@kekekeks
Copy link
Member

kekekeks commented Sep 3, 2024

Some DBus protocols require variants to be wrapped into variants. If it's not possible to represent such thing via the API, we'd need to switch the transport layer to a spec-conformant one for future-proofing.

@kekekeks
Copy link
Member

kekekeks commented Sep 3, 2024

The current problem was caused by misrepresentation of variants on receiver side, which is an inconvenience, but it's not possible to send such messages, which would outright prevent us from being able to implement protocols.

@tmds
Copy link
Contributor

tmds commented Sep 3, 2024

but it's not possible to send such messages, which would outright prevent us from being able to implement protocols.

For sending, VariantValue isn't used. There's a different type called Variant. Currently, it doesn't allow putting a Variant in a Variant. That's not intentional, it is an oversight.

And even then, the lower-level APIs enable you to take full control of serialization.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants