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

Refactored lifetime control into separate lifetime classes #2676

Merged
merged 4 commits into from Jun 22, 2019

Conversation

kekekeks
Copy link
Member

@kekekeks kekekeks commented Jun 21, 2019

See: #2564

The basic idea is to remove the lifetime control from Application class and make it something platform dependent.

Currently implemented lifetimes with interface hierarchy:

  • ClassicDesktopStyleApplicationLifetime
    • IClassicDesktopStyleApplicationLifetime
      • IControlledApplicationLifetime
  • LinuxFramebufferApplicationLifetime
    • IControlledApplicationLifetime
    • ISingleViewApplicationLifetime

Later we can add MobileStyleDesktopApplicationLifetime that implements ISingleViewApplicationLifetime and shows that view in a single Window. That would improve the portability of x-plat applications that don't need multiple independent windows. We could also implement a navigation stack that uses overlays on mobile and native window dialogs on desktop.

So, the initialization from the app perspective would look like this:

Application.Initialize - load XAML and stuff
Application.OnFrameworkInitializationCompleted - we can do pretty much everything at this point, ApplicationLifetime property contains a valid value if application lifetime was configured. This is the most convenient place to setup the main window or the main view.
IControlledApplicationLifetime.Started - just before starting the main loop

@kekekeks
Copy link
Member Author

@Gillibald

Copy link
Contributor

@Gillibald Gillibald left a comment

Choose a reason for hiding this comment

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

I like these changes a lot. It is now clear how a specific platform handles lifetime and there should be less confusion when Application is used.

If we really need a public collection of active windows we should think about maintaining this collection somewhere else.

/// The main window.
/// </value>
public Window MainWindow { get; set; }

/// <summary>
/// Gets the open windows of the application.
/// </summary>
/// <value>
/// The windows.
/// </value>
public WindowCollection Windows { get; }
Copy link
Contributor

Choose a reason for hiding this comment

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

We should think about moving this back to Window itself or make this a member of IClassicDesktopStyleApplicationLifetime. If Application.Current.ApplicationLifetime is of that interface we can access this otherwise we don't need it anyways.

Copy link
Member Author

@kekekeks kekekeks Jun 21, 2019

Choose a reason for hiding this comment

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

I was thinking about adding Created and Closed routed events, so a class like WindowsCollection can be implemented without actually touching the Window code and without mandatory global state. Then we can make it an opt-in feature that is enabled with UseGlobalWindowsCollection() while desktop lifetime keeps its own window list.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like the idea of driving it via events instead of it being a global list.

}

/// <inheritdoc/>
public event EventHandler<ControlledApplicationLifetimeStartupEventArgs> Startup;
Copy link
Contributor

@Gillibald Gillibald Jun 21, 2019

Choose a reason for hiding this comment

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

How do I subscribe to this event before it is being raised? This should be possible within the AppBuilder. A Startup, Exit action would be enough I guess. Maybe I have missed something here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess we could move it to an earlier point of application initialization, so even platform initialization callbacks would have access to it via AppBuilder.Instance.ApplicationLifetime.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, actually, it's already set before calling SetupWithoutStarting() here: https://github.com/AvaloniaUI/Avalonia/pull/2676/files#diff-01bc1e09e99bb7c8853294c6b0b206a2R94

@kekekeks
Copy link
Member Author

@worldbeater worldbeater mentioned this pull request Jun 22, 2019
3 tasks
@kekekeks kekekeks merged commit 8349b4d into master Jun 22, 2019
@grokys
Copy link
Member

grokys commented Jun 25, 2019

This has broken a number of the samples (e.g. BindingDemo, RenderDemo). I think we need to update #2499 with the changes required by this PR and get that merged.

@grokys grokys deleted the app-lifetime branch June 25, 2019 16:30
@kekekeks
Copy link
Member Author

.Start<MainWindow>(); should still work. If samples are broken then it's a bug.

@grokys
Copy link
Member

grokys commented Jun 25, 2019

Those samples hadn't even been updated for the changes to AppBuilder in 0.8 (which is what #2499 does) which is why they're failing I think.

@kekekeks
Copy link
Member Author

There was a missing Setup() call in .Start<MainWindow>();. Refactored it to use a desktop lifetime in 1e115b8

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.

None yet

4 participants