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

ReactiveUI.Avalonia migration #3936

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

maxkatz6
Copy link

@maxkatz6 maxkatz6 commented Dec 5, 2024

What kind of change does this PR introduce?

Pushes ReactiveUI.Avalonia backend to this repository and updates it to match other backends more.
Additionally, IntegrationTests.Avalonia was added.

What is the current behavior?

Avalonia integration is owned by Avalonia.ReactiveUI package, maintained by Avalonia core team.
Functionally, that package is not up to date with new ReactiveUI features, as well as some existing features implemented inconsistently compared to other backends.

What is the new behavior?

New ReactiveUI.Avalonia package is going to be created, with code in this repository.
This PR also already adds some features, that were missing previously:

  • ICreatesObservableForProperty is a low hanging fruit

And some are pending:

  • ICreatesCommandBinding - does it make sense to have Avalonia specific implementation? From WinForms implementation, I don't see a benefit.
  • IPlatformOperations - how this one is used? Avalonia 11.2 got Screen.Orientation API, which we can use, but it requires a window/toplevel to get screen from, as there can be multiple. Or we can assume main window from global statics.

What might this PR break?

Users who currently use Avalonia.ReactiveUI. NuGet package redirects should be created, and users should be notified.

Open questions

  1. Testing: Avalonia had ReactiveUI specific tests in the core repository: https://github.com/AvaloniaUI/Avalonia/tree/master/tests/Avalonia.ReactiveUI.UnitTests. It would make sense to move them as well, or even better integrate into existing testing projects. From the first look, there is some connection in ReactiveUI.Tests project, but these only run for windows frameworks? How would I add Avalonia implementation there?
  2. Initialization: Avalonia.ReactiveUI always had .UseReactiveUI() app builder method, it's also used in our ReactiveUI templates. It is different from other backends, which provide IWantsToRegisterStuff implementation instead. My question is, would it be preferable to do the same in Avalonia? My only concern is trimmability of library, how is it handled in other backends?

Please check if the PR fulfills these requirements

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

cc @glennawatson @ChrisPulman

/// </summary>
/// <param name="builder">This builder.</param>
/// <returns>The builder.</returns>
public static AppBuilder UseReactiveUI(this AppBuilder builder)
Copy link
Author

Choose a reason for hiding this comment

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

This extension method can be replaced with IWantsToRegisterStuff. Is this a recommended approach?

Copy link
Member

Choose a reason for hiding this comment

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

considering one of the things on the wishlist for reactiveui is to improve the start up and trimming etc, stick with this?

/// <param name="lifetime">Pass in the Application.ApplicationLifetime property.</param>
public AutoSuspendHelper(IApplicationLifetime lifetime)
{
RxApp.SuspensionHost.IsResuming = Observable.Never<Unit>();
Copy link
Author

Choose a reason for hiding this comment

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

With Avalonia 11.1 we can have this observable properly implemented. But likely should be done in dedicated PR, as well as it raises minimal Avalonia version (right now this PR targets 11.0).

@anaisbetts
Copy link
Member

anaisbetts commented Dec 5, 2024

how this one is used? Avalonia 11.2 got Screen.Orientation API, which we can use, but it requires a window/toplevel to get screen from, as there can be multiple. Or we can assume main window from global statics.

This is used in RoutedViewHost on mobile platforms - on Desktop it should follow what other desktop platforms do (probably return a constant); in general, don't worry about this too much

ICreatesCommandBinding

We probably don't need it, but it would be nice to get BindCommand working on Avalonia, afaik at the moment it does not

In general, the idea behind ICreatesCommandBinding is to recognize things that have a Command property that we can set a ReactiveCommand on. If Avalonia has any kind of specific criteria for that, this would be a good way to encode that

Users who currently use Avalonia.ReactiveUI. NuGet package redirects should be created, and users should be notified.

"Avalonia.ReactiveUI is deprecated, please move to ReactiveUI.Avalonia" is a phrase no user of either of our platforms should ever have to think about because it is pants-on-head silly. Can we make ".UseReactiveUI" in Avalonia.ReactiveUI and make it depend on ReactiveUI.Avalonia?

@glennawatson
Copy link
Contributor

More of a nuget thing. Both reactiveui and reactiveui have reserved namespaces so neither of us can push to each other packages.

@glennawatson
Copy link
Contributor

Also nuget has an automatic redirect functionality to at least point the user.

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.

4 participants