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

implemented workaround for .NET Core design-time data binding bug #251

Merged

Conversation

TysonMN
Copy link
Member

@TysonMN TysonMN commented Aug 5, 2020

I got a reply on the issue I created in the Visual Studio Developer Community. The same issue was reported three weeks ago, and a workaround was found. This PR implements that workaround.

Quoting #249 (comment)

I think of Visual Studio Developer Community as the place where issues go to die. I would much rather create an issue on GitHub. However, I think the instruction not to post an issue with the XAML designer is clear and doing so risks angering the people best able to fix the problem.

I need to eat my words. They did a great job this time.

Quoting #249 (comment)

For good measure, could you see if it works if add a d:DataContext binding parallel to the existing DataContext binding? E.g.

<local:Counter DataContext="{Binding Counter}" d:DataContext="{Binding Counter}" HorizontalAlignment="Center" />

Or perhaps mess a little more around with the d:DataContext binding if it doesn't work. I'm just throwing stones at the wall here to see what sticks.

You were on the right track @cmeeren!

@cmeeren
Copy link
Member

cmeeren commented Aug 5, 2020

Thanks! Now that I see the workaround, I think I prefer removing x:Name and ElementName=... and instead use RelativeSource={RelativeSource AncestorType=UserControl} (or AncestorType=Window). It's essentially the same information, but the workaround is localized to the binding. What do you think?

@cmeeren
Copy link
Member

cmeeren commented Aug 5, 2020

Also, we should mention somewhere that you need to close and re-open the XAML file for the workaround to apply (I needed to do that, at least).

And if we use my alternative suggestion, that must be documented, preferably straight in the readme. (We can document both if you want.)

@TysonMN TysonMN force-pushed the workaround_.NET_Core_design-time_bug branch from 64293f2 to 80fbb60 Compare August 5, 2020 12:09
@TysonMN
Copy link
Member Author

TysonMN commented Aug 5, 2020

I think I prefer removing x:Name and ElementName=... and instead use RelativeSource={RelativeSource AncestorType=UserControl} (or AncestorType=Window). It's essentially the same information, but the workaround is localized to the binding. What do you think?

I also prefer using RelativeSource.

Using AncestorType=UserControl and AncestorType=StackPanel worked for me but AncestorType=Window did not...even after closing and opening the XAML file. I forced pushed a corresponding commit.

@TysonMN
Copy link
Member Author

TysonMN commented Aug 5, 2020

And if we use my alternative suggestion, that must be documented, preferably straight in the readme. (We can document both if you want.)

I was planning on sharing your RelativeSource idea in the issue on Visual Studio Developer Community. Then our README doesn't have to get into details of the specific workaround. The workaround will also be used in our samples.

@cmeeren
Copy link
Member

cmeeren commented Aug 5, 2020

Using AncestorType=UserControl and AncestorType=StackPanel worked for me but AncestorType=Window did not...even after closing and opening the XAML file. I forced pushed a corresponding commit.

How weird. Well, let's use StackPanel then.

I was planning on sharing your RelativeSource idea in the issue on Visual Studio Developer Community. Then our README doesn't have to get into details of the specific workaround. The workaround will also be used in our samples.

Sure! As long as we can link to a place where the user will not be confused about which particular variant of the workaround we suggest.

@TysonMN
Copy link
Member Author

TysonMN commented Aug 5, 2020

@BentTranberg, I thought of another workaround. You can target .NET Framework when a developer builds and target .NET Core when creating a release (like on a build server).

<PropertyGroup Condition="'$(Configuration)' == 'Debug'">
  <TargetFramework>net472</TargetFramework>
</PropertyGroup>

<PropertyGroup Condition="'$(Configuration)' != 'Debug'">
  <TargetFramework>netcoreapp3.1</TargetFramework>
</PropertyGroup>

One advantage of this approach is that it only requires workaround code in a constant number of spots. That is, it doesn't depend on the number of DataContext bindings. For me, the constant is three: F# project containing bindings, C# executable project containing the XAML, and F# project containing tests (since I wrote a unit test that just instantiates MainWindow just to ensure that it doesn't throw an exception). I implemented this workaround in my application at work and will probably move forward with this.

We could also use this workaround for the samples, but I prefer the simplicity of @cmeeren's RelativeSource workaround.

@TysonMN
Copy link
Member Author

TysonMN commented Aug 5, 2020

Sure! As long as we can link to a place where the user will not be confused about which particular variant of the workaround we suggest.

I added a comment to the issue. I am satisfied with the README (which doesn't mention the details of the workaround) and that issue as they are. I can add more information if you would prefer @cmeeren.

@cmeeren
Copy link
Member

cmeeren commented Aug 5, 2020

I can add more information if you would prefer @cmeeren.

Let's make it simple for our users. Particularly since the preferred workaround is not even formatted correctly (since markdown doesn't work in issue comments, as you mentioned in the issue). I have updated the readme (forgot to mention this issue in the commit message). Feel free to adjust.

@TysonMN TysonMN force-pushed the workaround_.NET_Core_design-time_bug branch from 80fbb60 to 7aadc2f Compare August 5, 2020 18:43
@TysonMN TysonMN force-pushed the workaround_.NET_Core_design-time_bug branch from 7aadc2f to 04280e3 Compare August 5, 2020 18:44
@TysonMN
Copy link
Member Author

TysonMN commented Aug 5, 2020

Feel free to adjust.

Looks great. I removed the README modifications from this PR and rebased. I will complete when the build finishes successfully.

@TysonMN TysonMN merged commit e37d7fc into elmish:master Aug 5, 2020
@TysonMN TysonMN deleted the workaround_.NET_Core_design-time_bug branch August 5, 2020 18:50
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.

2 participants