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

[MVUX] [Bindable] attribute is not considered by uno's type info generator #2648

Open
dr1rrb opened this issue Dec 19, 2024 · 10 comments
Open
Assignees
Labels
kind/bug Something isn't working triage/untriaged Indicates an issue requires triaging or verification.

Comments

@dr1rrb
Copy link
Member

dr1rrb commented Dec 19, 2024

Current behavior

When we generate a ViewModel we are adding a [BindableAttribute]. This attribute should be considered by the bindable type provider generator to generate the type info for bindings.

However it seems it's not : #2515 (comment)

Expected behavior

_ View models_ generated by MVUX should be binding friendly!

How to reproduce it (as minimally and precisely as possible)

cf. #2515 (comment)

@dr1rrb dr1rrb added kind/bug Something isn't working triage/untriaged Indicates an issue requires triaging or verification. labels Dec 19, 2024
@dr1rrb
Copy link
Member Author

dr1rrb commented Dec 19, 2024

Note: AN hypothesis would also be that MVUX's generator is executed after uno's type info generator.

@ajpinedam ajpinedam self-assigned this Dec 20, 2024
@DevTKSS
Copy link

DevTKSS commented Jan 1, 2025

Looked this up while investigating logging of navigation to see in first instance, at which event the DataContext of my MainPage will be set correctly and why my Content of MainPage (NavigationView) is everytime kind of broken after HotReload.

  1. Which particular attribute is the correct one to set on MainPage Codebehind class, for the MetadataProvider Line in DebugConsole? Would say that would be the Reactive.Bindable.BindableAttrubute, !But! I dont think so because in the intellisense is stated it is meant for the viewModel which is not equal to the view itself.
  2. The MainPage seems to get called twice? No idea why that should be done. First one after FrameNavigator via default Route, thats alright! But then the strange part beginns: all Controls which I did name with x:Name are popping up The DepencyProperty [...] is not defined in MainPage which makes me curious why it should even expect this to be a DependencyProperty? Maybe the MainPage_.... Partial generated Class from Microsoft Generator Analyzer is also delayed as imagined for the ViewModel Class?
  3. Looked up also the generated MetaDataTypeProvider (or similar named, you know which one i am meaning) which should hold the links and types from model and Viewmodel. This is looking fine as expected, so I would also vote here for delayed.

BUT!
4. Given that thought, is it possible that HotReload is missing some reload action which would re-insert the needed DataContext? To replicate, do a simple uno mvux application incl. Navigator Extension add a nav-item and e.g. contentcontrol with uen:Region.Attached + uen:Region.Navigator="Visibility" like its told in the docs.
The current page will be invisible after HotReload (with changed something on mainpage like adding a textblock somewhere)
The Navigator is expected at this point to navigate at least to default Route and this would be this current page! Not one line of the logger set to trace is telling about DataContext to be set before Initialization of the Bindings of this page.
If you set this.DataContextChanged += (s,e) => this.DataContext = (MainViewModel)e.newValue you will be greeted at runtime with a InvalidCastException, which is telling you would try to Case ShellModel into MainViewModel. Speaking also for wrong timing or non-optimal Navigation Routing not looking for that type its going to at this moment. Can someone tell where exactly this navigator is expected to set this DataContext to check this? Because if this also not know this types, just silencing that warning, it would have to be fixed there also.

Idea for that Binding of viewModel in general... Could we check if it is possible to do a extension for the Page partial class generated currently from Microsoft Generator, move it for Reactive generated ViewModel to same analyser (possible that could help with initial issue also? More control and so on?) and if the user is applying x:Bind in main instance of SomePage.xaml (using Binding would tell me, he dont cares about performance, so that sourcegen stepping in would not be needed) the generator which should already knowing about the ViewModel he has to bind that View to, at least if also navigation extension with routes is used! Could add a property ViewModel or _vm or whatever you prefer in sourcegenerators to that generated class? Even if we can not tell a sourcegenerator from the Roslyn issue to depend on other generators, could we opt 1 depend on uno own generators? Opt.2 setup the correct bound and correctly updated on ViewModel changed in there? That would be at least a little step forward and for me who only read the ViewModelSourceGen code in the repo that sounds at least imaginable...

@Youssef1313
Copy link
Member

All Roslyn generators are given the same compilation. So there is no concept of order. No generator can see the output of another.

@Youssef1313
Copy link
Member

The safest action is to not generate [Bindable], and instead implement an analyzer that warns the user for this case and suggest adding a non-generated partial with [Bindable]. Yes it's more code, but that is how life is with SGs.

@Youssef1313
Copy link
Member

Alternatively, the bindable generator may be complicated so that it tries to anticipate what the view model generator would be generating and act as if the attribute is seen. This was already done in XAML generator to support CommunityToolkit.Mvvm SGs.

@dr1rrb
Copy link
Member Author

dr1rrb commented Jan 7, 2025

All Roslyn generators are given the same compilation. So there is no concept of order. No generator can see the output of another.

Yes ... unfortunately

The safest action is to not generate [Bindable], and instead implement an analyzer that warns the user for this case and suggest adding a non-generated partial with [Bindable]. Yes it's more code, but that is how life is with SGs.

This is not possible: the "bindable" applies to the code generated by the MVUX generator. Even if user adds the [Bindable] himself the code will not be present yet.

@dr1rrb
Copy link
Member Author

dr1rrb commented Jan 7, 2025

@ajpinedam For now we should only validate that the issue is indeed because the bindable generator is not ran after MVUX's VM generator (or more precisely is run using the same code base), meaning bin-gen does not have access to the code generated by mvux-gen.

If so, we will put this hold for now as we will have to work on a larger fix to allow some sort of dependencies between generators as this is not the single case (for instance we already had to workaround the exact same case for toolkit's MVVM code gen).

(An idea would be to have some sort of discovery + add-in support between generators so a generator would be able to give info about the code it will generate to another generator without having to create a new roslyn generation which would significantly impact performance of generators).

@Youssef1313
Copy link
Member

This is not possible: the "bindable" applies to the code generated by the MVUX generator. Even if user adds the [Bindable] himself the code will not be present yet.

I don't see any reason why it won't be possible. The user will add a partial of the class that will be generated. And the analyzer should be able to flag that easily and warn the user. A CodeFixProvider can also be implemented to make it easier for developers. The only downside I see is that the user has to take an action and things are not happen magically for him. But as mentioned, the analyzer/codefix will help guide users.


If so, we will put this hold for now as we will have to work on a larger fix to allow some sort of dependencies between generators as this is not the single case (for instance we already had to workaround the exact same case for toolkit's MVVM code gen).

This is not possible. Source generators cannot have dependencies or ordering, not they can communicate with each others. However, in some cases, you can go with hacks if one generator can anticipate what another generator will do (most likely by duplicating some logic between generators). This was the case for XamlCodeGenerator where it can guess what will be generated by CommunityToolkit.Mvvm.

@dr1rrb
Copy link
Member Author

dr1rrb commented Jan 7, 2025

This is not possible: the "bindable" applies to the code generated by the MVUX generator. Even if user adds the [Bindable] himself the code will not be present yet.

I don't see any reason why it won't be possible. The user will add a partial of the class that will be generated. And the analyzer should be able to flag that easily and warn the user. A CodeFixProvider can also be implemented to make it easier for developers. The only downside I see is that the user has to take an action and things are not happen magically for him. But as mentioned, the analyzer/codefix will help guide users.

Because even if the user creates a partial VM class it will be empty as content is fully generated by the mvux-gen (including properties), so the bin-gen won't be able to do anything.

If so, we will put this hold for now as we will have to work on a larger fix to allow some sort of dependencies between generators as this is not the single case (for instance we already had to workaround the exact same case for toolkit's MVVM code gen).

This is not possible. Source generators cannot have dependencies or ordering, not they can communicate with each others. However, in some cases, you can go with hacks if one generator can anticipate what another generator will do (most likely by duplicating some logic between generators). This was the case for XamlCodeGenerator where it can guess what will be generated by CommunityToolkit.Mvvm.

This somehow what we were discussing. The idea would be to have some magic / hacks to discover other generators, then a generator-A could ask to another generator-B info about what it is going to generate so gen-A could also generate content based on that (one of the possible tracks). But yes the goal is to avoid duplication and/or double execution of generators.

@Youssef1313
Copy link
Member

Youssef1313 commented Jan 7, 2025

Because even if the user creates a partial VM class it will be empty as content is fully generated by the mvux-gen (including properties), so the bin-gen won't be able to do anything.

Ah I see, it will only see the type without any of its members. That's not enough for the bindable gen. I don't see any viable solution for this though. This was clearly stated by Roslyn team that this scenario can never work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working triage/untriaged Indicates an issue requires triaging or verification.
Projects
None yet
Development

No branches or pull requests

4 participants