-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Port from WPF to Avalonia #361
Conversation
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.
Going great so far :) I left some comments
YoutubeDownloader/App.xaml.cs
Outdated
@@ -21,37 +33,103 @@ public partial class App | |||
public static string ChangelogUrl { get; } = ProjectUrl + "/blob/master/Changelog.md"; | |||
} | |||
|
|||
public partial class App | |||
[DoNotNotify] | |||
public partial class App : StyletApplication<RootViewModel> |
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.
Is there a specific reason you merged the bootstrapper with the App?
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.
because it seems like these were merged in Stylet.Avalonia https://github-com.translate.goog/sealoyal2018/Stylet.Avalonia?_x_tr_sl=auto&_x_tr_tl=de&_x_tr_hl=de
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 you think we need Stylet at all? I remember we discussed this and you said Avalonia already provides many of its features out of the box (such as method binding). Does Avalonia have something for binding views to view models automatically too?
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 had started first without Stylet because it was still lacking behind in Avalonia previews when Avalonia 11 was still in preview. But since somethings changed with Avalonia till the release and also YoutubeDownloader moved forward I somewhat started again to not miss any of the changes. When I started again I used Stylet.Avalonia because it was faster. But I can again migrate of it. A drawback of the Stylet.Avalonia (which is a independet fork of Stylet) is also that is completey switched its readme to Chinese only.
Yes Avalonia has some parts already build in like binding to methods form commands (not from events) and one can easily build a general ViewLocator which is provided with the MVVM templates. I can change to that approach together with CommunityToolkit.MVVM from microsoft for some MVVM stuff. But I ended up rebuild some parts of Stylet, it would not be a problem for me since I had done that already.
You can also look at my first attempt here: https://github.dev/Mrxx99/YoutubeDownloader/tree/Avalonia/YoutubeDownloader.Avaloniav11
We could also wait until Caliburn.Micro is released (officially) for Avalonia which should be (hopefully) soon: Caliburn-Micro/Caliburn.Micro#851
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.
Yes Avalonia has some parts already build in like binding to methods form commands (not from events) and one can easily build a general ViewLocator which is provided with the MVVM templates. I can change to that approach together with CommunityToolkit.MVVM from microsoft for some MVVM stuff. But I ended up rebuild some parts of Stylet, it would not be a problem for me since I had done that already.
I'm pretty ambivalent, I think. We already have some "plumbing code" in ViewModels/Framework
I think, so it wouldn't be too big of a deal if we added more. I'm also not opposed to adding 3rd party packages, as long as they're fairly widely used and are trustworthy.
I'll leave the decision up to you, let's go with whatever you think provides the best balance in terms of effort/control.
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.
one can easily build a general ViewLocator which is provided with the MVVM templates. I can change to that approach together with CommunityToolkit.MVVM
if you're thinking of using the CommunityToolkit.MVVM
tools, they also built a DependencyInjection package with source generators that:
- Makes the service registration completely trim/AOT-friendly
- Provides built-time diagnostics on registration errors
With that, it's possible to build an improved version of the ViewLocator
. Here's a demo project from @stevemonaco that uses the DI package.
The package is experimental, but apparently it is used in production and they're just waiting for the first chance to push it into the stable Nuget feed.
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.
The package is experimental, but apparently it is used in production and they're just waiting for the first chance to push it into the stable Nuget feed.
I asked Sergio about this in the past week or so. He said that his MEDI sourcegen is currently being used in Windows Store, but he's lacking the time to move the code base/packaging into CommunityToolkit
proper.
Co-authored-by: Oleksii Holub <[email protected]>
…tubeDownloader into feature/avalonia-port
@Tyrrrz Do you want to add support for Android and iOS at some point too? |
No, I think it would be unnecessary. |
Might want to merge/rebase to get the |
I did already |
Nice, I see now. Had outdated diff for a sec. |
YoutubeDownloader/Converters/VideoQualityPreferenceToStringConverter.cs
Outdated
Show resolved
Hide resolved
I'm thinking that the best way is to download it on-demand during application run time. That way we don't have to package a million different versions of ffmpeg. We should do it separately though, not in this PR. For now let's just keep ffmpeg Windows-only in this iteration. |
Do you think we should change |
…ylet action for commands
…olve viewmodels from DI as well
|
||
// We handle the event here so we have to directly "press" the default button | ||
AccessKeyManager.ProcessKey(null, "\x000D", false); | ||
await downloadViewModel.CopyErrorMessage(); |
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.
Probably can just do DataContext.CopyErrorMessage
now and simplify the if
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.
You mean because of the ViewModelAwareUserControl generics? That won't work, because the DataContext of the view is DashboardViewModel. Or do you mean just doing casting on the call site instead of type check in the if?
{ | ||
public DashboardView() | ||
{ | ||
InitializeComponent(); | ||
QueryTextBox.AddHandler( |
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.
Why is this event handled with AddHandler
and the other (OnStatusPointerReleased
) with XAML binding. Is one approach better than the other somehow? If they're the same, can we standardize?
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.
AddHanlder is used here as in Avalonia there are no Preview* helper events, so with using AddHandler you can specify the routing strategy as tunnel, which is the equivalent of preview events in WPF. A "preview" event is required here, as otherwise the enter key would have already created a new line.
private void WebBrowser_OnUnloaded(object? sender, RoutedEventArgs e) | ||
{ | ||
// This will most likely not work because WebView2 would still be running at this point, | ||
// and there doesn't seem to be any way to shut it down using the .NET API. | ||
if (WebBrowser.CoreWebView2?.Profile is not null) | ||
await WebBrowser.CoreWebView2.Profile.ClearBrowsingDataAsync(); | ||
// if (_coreWebView2?.Profile is not null) | ||
// await _coreWebView2.Profile.ClearBrowsingDataAsync(); | ||
} |
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.
Are these methods not accessible or why are they commented out?
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.
the webview is already disposed at that point so it throws an exception:
System.InvalidOperationException: CoreWebView2 members cannot be accessed after the WebView2 control is disposed.
So I guess the handler can just be removed.
Finally finished reviewing! One more thing: can you please add the |
Also, feel free to rename |
@Mrxx99 let me know if you're too busy and would rather have this merged as is. I can then just apply my suggestions locally. |
I started working on the Async postfixes, which required more changes as with the renaming the auto detect CanExecute of Avalonia does not work anymore (because of different names) and there were also some issues with them anyway, do I am changing them to use the RelayCommand generator of the MVVM toolkit |
Okay, no worries. If those changes end up being more complicated than expected, let me know and we can descope them. |
Yes, I know, I just had them as xaml to have better diff at the PR. So I think it would make sense to change the extension after merging the PR so the changes could still be seen at the PR. |
Yes, very good point. |
Hi @Tyrrrz I think this can be merged, you can maybe look at the unresolved comments. |
One thing I noticed, the build output contains the runtimes folder, with native binaries for linux, mac and windows. For windows users that would be at least 50 MB additional. You can publish with --runtime to only get the binaries for windows, but then it would need three different publishes (and three resulting binaries) for win-x64, win-86 and win-arm64. |
Thanks again for the massive effort @Mrxx99! I'll take it from here now. |
Related discussion: #295
This almost finished work-in-progress of the promised Avalonia port.
Only some small differences in UI, and the web authentication is not working yet (WebView.Avalonia does not give cookie access, I have to look for alternatives), and the ffmpeg downloading has to be refactored for cross platform.
Normally Avalonia uses the .axaml file extension by default, but to have a better diff view in git I kept the .xaml file extension for now, it only needs a small addition in the project file to be supported