-
Notifications
You must be signed in to change notification settings - Fork 779
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
[WIP] AvaloniaUI support #738
Conversation
* Changed WindowConductor to not use the Window.Closed event * Changed ExecuteOnLoad to use AttachedToLogicalTree instead of visual tree
This look interesting, I will try and a review it once I get an hour or two of free time. |
if (owner != null && isDialog) | ||
{ | ||
//TODO: (Avalonia) Set window owner | ||
//window.Owner = owner; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be commented out?
{ | ||
window.WindowStartupLocation = WindowStartupLocation.CenterOwner; | ||
//TODO: (Avalonia) Set window owner | ||
//window.Owner = owner; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to set window owner here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
judging from the methods name, it should.
why has nobody answered your review-questions for such a long time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the question is because this flow is not valid for Avalonia.
You pass the owner as parameter to ShowDialog (mandatory) or Show (optional). So it is really a business/architectural decision if we divert from traditional Caliburn way of doing it at the cost of introducing inconsistencies between platforms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also beware that work has continued here: https://github.com/Caliburn-Micro/Caliburn.Micro/tree/556-compatibility-inquiry-avaloniaui
It can use a rebase btw cause it is not compatible anymore with current master, there are conflicts due to merge of MAUI support.
Could you add a sample AvaloniaUI app to the samples also? |
Done. I'm not sure where it would be ideal to start the CM bootstrapper. Now it's created in App.OnFrameworkInitializationCompleted() in App.axaml.cs. Another solution might be to include it as a resource in App.axaml like in WPF. |
Should we namespace this similarly to the Xamarin.Forms package? I don’t think we can determine whether or not a project uses Avalonia otherwise. |
I agree we need an Avalonia package similar to the xamarin.forms |
Also I think this was made before we reintroduced the |
What are the steps left to have this into Caliburn? It looks like it works but we need some restructuring? I could help some but am not sure where in the process we are atm. |
will work on fixing merge conflicts and see if we can get it merged in this weekend. It will need testing |
I tried a while back to include Avalonia in the features sample project, and ran into some issues with the way navigation was handled via frame adapter. I think if we can make it work with the features sample project it will be a good test. |
It was my fault. Ignore it. |
Created a branch of my own to keep working on this. https://github.com/Caliburn-Micro/Caliburn.Micro/tree/556-compatibility-inquiry-avaloniaui The biggest issue I have remaining is getting
events to fire. |
|
The latest Avalonia nightlies have some breaking changes. Luckily they are focussing on bug fixes for the upcoming version from now on so I don't expect more breaking stuff to be added. One has to do with different parameters in InitializeComponent which is called through reflection by Caliburn. I have a fix for this locally that I'll create a PR for. Then we need to also rebase this branch on master because I am sure there are some conflicts. Some other thing might still be missing that I don't encounter in my project, @vb2ae has more details about them I think. |
I apologize for my absence. Thank you for looking into this and continuing the development. |
@megazyz you do this in your free time so no apologies needed mate |
This makes Caliburn.Micro work with AvaloniaUI.
Related to #556.
Needs review and testing.