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

added design-time view models for all samples #249

Merged
merged 1 commit into from
Aug 4, 2020

Conversation

TysonMN
Copy link
Member

@TysonMN TysonMN commented Jul 27, 2020

Resolves #246

This branch adds design-time view models to each sample.

There were a couple that didn't work as I expected. For example, data isn't showing up for CounterWithClock.xaml in the SubModel sample. Not sure why. I will look into it more.

This branch also includes some minor formatting adjustments near where I made some changes. I think I will extract those changes, merge them into master, and then rebase this branch so that the diff is cleaner.

I am open to feedback now. I marked the PR as a draft while I address the two things I mentioned above.

@cmeeren
Copy link
Member

cmeeren commented Jul 27, 2020

Thanks! I'll have a look at it once the the two things you mentioned are addressed.

@TysonMN TysonMN force-pushed the design_time_VMs_for_samples branch 2 times, most recently from 3380c2b to 5bd8660 Compare July 29, 2020 15:37
@TysonMN
Copy link
Member Author

TysonMN commented Jul 29, 2020

The diff is very clean now.

I tried and again failed to get the the design-time view model working for CounterWithClock.xaml in the SubModel sample. Issue #154 was about design-time view models working for .NET Framework but not for .NET Core. But the current issue exists for me for both targets.

I will try again soon.

src/Samples/FileDialogs.CmdMsg/Program.fs Outdated Show resolved Hide resolved
src/Samples/FileDialogs.CmdMsg/Program.fs Outdated Show resolved Hide resolved
src/Samples/FileDialogs/Program.fs Outdated Show resolved Hide resolved
src/Samples/NewWindow/Program.fs Outdated Show resolved Hide resolved
src/Samples/NewWindow/Program.fs Outdated Show resolved Hide resolved
src/Samples/SubModelSeq/Program.fs Outdated Show resolved Hide resolved
src/Samples/FileDialogs.CmdMsg/Program.fs Outdated Show resolved Hide resolved
@cmeeren
Copy link
Member

cmeeren commented Jul 30, 2020

As far as GitHub shows me, all the conversaions are now resolved. That just leaves the problematic CounterWithClock.xaml view model, right?

@TysonMN
Copy link
Member Author

TysonMN commented Jul 30, 2020

Correct. There is more than one with that same thigh. I don't remember all of them, but one of them is the main window in the same project.

@TysonMN
Copy link
Member Author

TysonMN commented Aug 1, 2020

After an exceedingly long time debugging. I figured out why DataContext bindings were not working at design-time.

The main issue is .NET Core. The samples target .NET Core 3.1. Switching to .NET Framework 4.7.2 (which I tried almost immediately when initially debugging this problem) and also executing git clean -fdx fixes the problem.

I recommend that we have the samples target .NET Framework 4.7.2 and expand the existing warning in the README about the issues with design-time bindings working when targeting .NET Core.

On this topic of the other issue with bindings when targeting .NET Core, I would like to understand it better. It was originally reported in issue #154 and then discussed again in issue #197. @BentTranberg or @cmeeren, can you test this again when using the latest versions of .NET Core and Visual Studio? I only half followed along before, so I am not sure how to reproduce this bug. I tried several things, so maybe that issue is fixd, but it could also be me not testing for it correctly.

I will also either find an existing GitHub issue or create one (but don't have the time now).

@cmeeren
Copy link
Member

cmeeren commented Aug 1, 2020

Sorry, I don't quite follow you. What specifically is it you want us to test?

Also, what exactly is not working for netcoreapp3.1 but working for net472?

@TysonMN
Copy link
Member Author

TysonMN commented Aug 1, 2020

On this topic of the other issue with bindings when targeting .NET Core, I would like to understand it better. It was originally reported in issue #154 and then discussed again in issue #197. @BentTranberg or @cmeeren, can you test this again when using the latest versions of .NET Core and Visual Studio? I only half followed along before, so I am not sure how to reproduce this bug. I tried several things, so maybe that issue is fixd, but it could also be me not testing for it correctly.

What specifically is it you want us to test?

Does the bug described in issue #154 still exist when using the latest version of .NET Core and Visual Studio?

@TysonMN
Copy link
Member Author

TysonMN commented Aug 1, 2020

what exactly is not working for netcoreapp3.1 but working for net472?

Consider the SubModel sample. Follow these steps.

.NET Core

  1. Execute git clean -fdx at the root of the repository.
  2. Check out the branch design_time_VMs_for_samples, which is the branch for this PR and currently targets .NET Core 3.1.
  3. Build the code.
  4. Open CounterWithClock.xaml
  5. Observe that design-time data within DataContext bindings are not displayed.

2020-08-01_09-26-10_220_devenv

.NET Framework

  1. Execute git clean -fdx at the root of the repository.
  2. Check out the branch design_time_VMs_for_samples_.Net_Framework, which targets .NET Framework 4.7.2.
  3. Build the code.
  4. Open CounterWithClock.xaml
  5. Observe that design-time data within DataContext bindings are displayed.

2020-08-01_09-29-46_221_devenv

@BentTranberg
Copy link

BentTranberg commented Aug 1, 2020

Does the bug described in issue #154 still exist when using the latest version of .NET Core and Visual Studio?

Yes, bug still there. Tested it just now with very latest public versions of VS and .NET Core SDK. And Windows 10 updated. EDIT: And <TargetFramework>netcoreapp3.1</TargetFramework>

I believe @cmeeren suggested long time ago that this is a problem in WPF in .NET Core, and I can certainly believe that. I never got the time to make a minimal repro and post it as an issue in the dotnet/wpf repo before being laid off, so it'll have to wait one or two more months if I'm to do that when I'm back at work. I'm thinking that if this can be reproduced using C# rather than F# for the design time model (I haven't even thought about whether it can be accomplished technically), then maybe that would help attract more attention in that repo. On the other hand I like the idea of promoting F#.

@cmeeren
Copy link
Member

cmeeren commented Aug 1, 2020

what exactly is not working for netcoreapp3.1 but working for net472?

Consider the SubModel sample. Follow these steps.

Since the others samples/files work, what is different for CounterWithClock.xaml?

@TysonMN
Copy link
Member Author

TysonMN commented Aug 1, 2020

what exactly is not working for netcoreapp3.1 but working for net472?

Consider the SubModel sample. Follow these steps.

Since the others samples/files work, what is different for CounterWithClock.xaml?

The difference is a binding to DataContext

@TysonMN
Copy link
Member Author

TysonMN commented Aug 1, 2020

The difference is a binding to DataContext

I will post an issue soon to the .NET WPF repo. It will be a bit easier to see the issue in the MWE that I provide there than via the SubModel sample and the CounterWithClock.xaml file.

@cmeeren
Copy link
Member

cmeeren commented Aug 1, 2020

The difference is a binding to DataContext

Thanks, I see. For good measure, could you see if it works if add a d:DataContext binding parallell 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.

I recommend that we have the samples target .NET Framework 4.7.2 and expand the existing warning in the README about the issues with design-time bindings working when targeting .NET Core.

I'm on board with improving the readme, but I'm a bit hesitant to make the samples target the now more or less legacy .NET Framework. I'm comfortable with having design-time VMs not work properly in all samples and chalk that up to an upstream WPF/tooling bug for now.

I will post an issue soon to the .NET WPF repo. It will be a bit easier to see the issue in the MWE that I provide there than via the SubModel sample and the CounterWithClock.xaml file.

Thanks a lot! Regarding the following C#/F# remarks by @BentTranberg:

I'm thinking that if this can be reproduced using C# rather than F# for the design time model (I haven't even thought about whether it can be accomplished technically), then maybe that would help attract more attention in that repo. On the other hand I like the idea of promoting F#.

I'm thinking that if it's reproducible in C#, that should be very clearly stated to get it prioritized higher. As long as that's clear, I guess it won't matter particularly much whether it's demonstrated in C# or F#.

@cmeeren
Copy link
Member

cmeeren commented Aug 1, 2020

As an aside, I think there are other ways than d:DataContext to get design-time data in WPF, but it's been a long time since I investigated. There may exist another method that works.

@BentTranberg
Copy link

There's a risk I may have mislead you, so I just want to make it clear that what I tested was my old sample with d:DataContext="{x:Static model:CounterPane.designTimeModel}" and the design time support from Elmish.WPF. I see that the binding in your samples are not like that, so I'm going to test these samples now.

@BentTranberg
Copy link

BentTranberg commented Aug 1, 2020

Allright, I can confirm that CounterWithClock.xaml shows design time data in branch design_time_VMs_for_samples_.Net_Framework, but not in branch design_time_VMs_for_samples. Just as shown in the screenshots above. EDIT: As previously indicated, I have the latest public VS (16.6.5) and .NET Core SDK (3.1.302 + some even later preview SDKs) EDIT2: Just to be clear in case you wonder what I'm doing this time around, I did compile the entire repos unmodified before looking at the samples.

@TysonMN
Copy link
Member Author

TysonMN commented Aug 2, 2020

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.

Sure. I tired your specific suggestion, but the behavior was the same. I didn't try anything else.

Here is my bug report. The dotnet/wpf repo said (via the pre-populated text in a new issue) that the correct place to submit this bug report is with the Visual Studio Developer community, so that is what I did.

@TysonMN
Copy link
Member Author

TysonMN commented Aug 2, 2020

I'm on board with improving the readme, but I'm a bit hesitant to make the samples target the now more or less legacy .NET Framework. I'm comfortable with having design-time VMs not work properly in all samples and chalk that up to an upstream WPF/tooling bug for now.

Ok, I am fine with that.

I would prefer to reproduce the bug from #154, submit another bug report, and then link to both bug reports from our README.

@TysonMN
Copy link
Member Author

TysonMN commented Aug 2, 2020

I never got the time to make a minimal repro and post it as an issue in the dotnet/wpf repo before being laid off, so it'll have to wait one or two more months if I'm to do that when I'm back at work. I'm thinking that if this can be reproduced using C# rather than F# for the design time model (I haven't even thought about whether it can be accomplished technically), then maybe that would help attract more attention in that repo. On the other hand I like the idea of promoting F#.

I am willing to create a minimal repro using only C# and post the issue with Visual Studio's Developer Community. @BentTranberg, do you mind helping me reproduce the problem? Is there a branch that you can share with me? I don't mind if it is not minimal. I can simplify it after I can reproduce it. However, I remember that you wanted to not work on Elmish.WPF while temporarily laid off to avoid a potential conflict of interest, so please turn me down if you would prefer not to help right now.

@BentTranberg
Copy link

BentTranberg commented Aug 2, 2020

I would absolutely post the issue directly in the dotnet/wpf repo rather than the VS Dev Community. The latter is for the millions of students, "MS customers", tinkerers and others that post flames and insist MS fix their problem. For me it's a waste of time. The repo is for people like you, who know darn well what they're doing, and typically somehow engage in fixing the problem after reporting it. I'm not that familiar with dotnet/wpf, but I've used dotnet/fsharp a lot. I even got a thanks from Don Syme after filing one issue report, so I sure feel welcome there as long as I engage intelligently.

My lay-off most probably ends in mid to late August, or else in September. I'll wait until then engaging in any work (also for other reasons, I'm quite busy until then), but at least I can point you to my files with the model that fails when on .NET Core 3.x

The repo is https://github.com/BentTranberg/ExploreElmishWpf

The interesting files are CounterDemo.fs, CounterDemo.xaml and CounterDemo.xaml.cs

In CounterDemo.fs there are some comments about the problem. You probably remember that we discussed the issue long ago, and I described how I used an alternative way of getting a design time model to work. The comments explain this.

I haven't followed the ongoing development you've been so busy with, so I don't know what relevance my old source has these days.

@cmeeren
Copy link
Member

cmeeren commented Aug 2, 2020

I would prefer to reproduce the bug from #154, submit another bug report, and then link to both bug reports from our README.

That would be great. Regarding #154, the readme already contains instructions based on that issue (see below), but adding a link won't hurt.

When targeting .NET Core 3, the designer is buggy when it comes to design-time view models as used above. If you don’t get the data you expect, try to remove and re-add (e.g. undo) the d:DataContext line. That should make it work. You will have to do this as often as needed.

@TysonMN
Copy link
Member Author

TysonMN commented Aug 3, 2020

Here is my bug report. The dotnet/wpf repo said (via the pre-populated text in a new issue) that the correct place to submit this bug report is with the Visual Studio Developer community, so that is what I did.

I would absolutely post the issue directly in the dotnet/wpf repo rather than the VS Dev Community.

Here is the pre-populated text I mentioned.

Is this bug related specifically to tooling in Visual Studio (e.g. XAML Designer, Code editing, etc...)? If yes, please file the issue via the instructions here.

Continuing to quote you @BentTranberg.

The latter is for the millions of students, "MS customers", tinkerers and others that post flames and insist MS fix their problem. For me it's a waste of time. The repo is for people like you, who know darn well what they're doing, and typically somehow engage in fixing the problem after reporting it.

I feel the same way. 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.

@TysonMN
Copy link
Member Author

TysonMN commented Aug 3, 2020

@BentTranberg, do you mind helping me reproduce the problem? Is there a branch that you can share with me?

The repo is https://github.com/BentTranberg/ExploreElmishWpf

The interesting files are CounterDemo.fs, CounterDemo.xaml and CounterDemo.xaml.fs

In CounterDemo.fs there are some comments about the problem. You probably remember that we discussed the issue long ago, and I described how I used an alternative way of getting a design time model to work. The comments explain this.

I haven't followed the ongoing development you've been so busy with, so I don't know what relevance my old source has these days.

Great. I have reproduced the problem now. That was so easy. I don't know why I had trouble following along before. There are no issues with the ongoing development. (Also, I was a bit surprised when you said to look at CounterDemo.xaml.fs, but now I see that was a simple typo. You meant CounterDemo.xaml.cs.)

@BentTranberg
Copy link

You meant CounterDemo.xaml.cs

Oops! Sorry. Must be getting old. Wait, let med check the calendar ... yup, I'll be 60 in around two weeks. Yikes!

Corrected it in my post above, so as not to waste anybody's time later.

... as the place where issues go to die.

I feel the same way. But agree with you it's better to follow the instructions.

Just curious: Why is it working when I use that design time model that doesn't use Elmish.WPF? As shown as an alternative in CounterDemo.fs in ExploreElmishWpf. Is there some way this can be exploited to work around the problem?

@TysonMN
Copy link
Member Author

TysonMN commented Aug 4, 2020

@BentTranberg, I changed CounterPane to CounterDemo in

d:DataContext="{x:Static model:CounterPane.designTimeModel}"

...and now the XAML designer and design-time data bindings are working perfectly for me.

Does this change fix your problem?

@BentTranberg
Copy link

Does this change fix your problem?

Yes. What? Darn, it seems I messed up at some point in the past, and never noticed because I did not expect things to work anyway, exactly because of this bug in .NET Core. Thanks for catching that.

So now I opened that production solution I've been working on until the virus hit, and indeed the design time models that all use d:DataContext="{x:Static model: and Elmish.WPF design time support are now working without the hiccups that required fiddling with the XAML to get the visuals back online. I can also see the visual designs appears much faster than they used to.

That's great. But then what about the problem uncovered in branch design_time_VMs_for_samples? What's that really about?

@cmeeren
Copy link
Member

cmeeren commented Aug 4, 2020

So now I opened that production solution I've been working on until the virus hit, and indeed the design time models that all use d:DataContext="{x:Static model: and Elmish.WPF design time support are now working without the hiccups that required fiddling with the XAML to get the visuals back online.

(Emphasis mine)

Are you targeting .NET Core 3? If yes, are you saying that this section of the readme doesn't apply and that things work as intended?

When targeting .NET Core 3, the designer is buggy when it comes to design-time view models as used above. If you don’t get the data you expect, try to remove and re-add (e.g. undo) the d:DataContext line. That should make it work. You will have to do this as often as needed.

@TysonMN
Copy link
Member Author

TysonMN commented Aug 4, 2020

Does this change fix your problem?

Yes. What? Darn, it seems I messed up at some point in the past, and never noticed because I did not expect things to work anyway, exactly because of this bug in .NET Core. Thanks for catching that.

So now I opened that production solution I've been working on until the virus hit, and indeed the design time models that all use d:DataContext="{x:Static model: and Elmish.WPF design time support are now working without the hiccups that required fiddling with the XAML to get the visuals back online. I can also see the visual designs appears much faster than they used to.

I went back to analyze and test this commit in your ExploreElmishWpf repo. I think (based on timestamps) that that commit was the head of your master branch when you posted #154. You didn't have the typo I just found in that commit. I also don't have any issues with that commit. Since you also don't have any problems with the head of your master branch, my guess is that the bug that you experienced and @cmeeren reproduced in #154 is now fixed.

That's great. But then what about the problem uncovered in branch design_time_VMs_for_samples? What's that really about?

Yes. This is definitely a bug with the XAML designer when targeting .NET Core.

Are you targeting .NET Core 3? If yes, are you saying that this section of the readme doesn't apply and that things work as intended?

That is what I think @BentTranberg is saying. I am thinking we can remove the existing statement from the README and add a more specific statement that links to the issue I created on Visual Studio Developer Community.

@BentTranberg
Copy link

Are you targeting .NET Core 3? If yes, are you saying that this section of the readme doesn't apply and that things work as intended?

Yes, this is on .NET Core 3.1. Indeed it seems to have been fixed at some point, but I have no idea when or where. I toyed around for a while trying to provoke the problem, without success.

@cmeeren
Copy link
Member

cmeeren commented Aug 4, 2020

Yes, this is on .NET Core 3.1. Indeed it seems to have been fixed at some point, but I have no idea when or where. I toyed around for a while trying to provoke the problem, without success.

Thanks. Did you try the things I mentioned as (potentially) problematic in #154 (comment)?

@BentTranberg
Copy link

BentTranberg commented Aug 4, 2020

Did you try the things I mentioned as (potentially) problematic in #154 (comment)?

Yes. But you're right - we should be cautious. This problem didn't behave in a totally consistent manner. I want to toy around a lot more than I did before I feel perfectly confident that the problem is gone, or at least doesn't appear frequently enough to be noticed or bothersome.

I'm out of time now, but can do some more testing tomorrow afternoon. (EDIT: Need another 24 hours.)

@TysonMN TysonMN marked this pull request as ready for review August 4, 2020 19:31
@TysonMN
Copy link
Member Author

TysonMN commented Aug 4, 2020

In the meantime, I pushed a commit to update the README.

I will test all the samples one more time now.

@TysonMN
Copy link
Member Author

TysonMN commented Aug 4, 2020

All the samples and design-time bindings worked (except for the known issue).

I pushed one more commit that makes SimpleCounter.Views the default startup project.

When you are satisfied @cmeeren, I will squash, rebase, force push, wait for the build to succeed, and then complete the PR.

@cmeeren
Copy link
Member

cmeeren commented Aug 4, 2020

Yes. But you're right - we should be cautious. This problem didn't behave in a totally consistent manner. I want to toy around a lot more than I did before I feel perfectly confident that the problem is gone, or at least doesn't appear frequently enough to be noticed or bothersome.

In the meantime, I pushed a commit to update the README.

I think @BentTranberg meant we should wait with that. But it's fine, let's keep it. 😊

@cmeeren
Copy link
Member

cmeeren commented Aug 4, 2020

IIRC I already reviewed this PR. I don't see any more commits here, so feel free to complete it.

@TysonMN TysonMN force-pushed the design_time_VMs_for_samples branch from 0cb6126 to 1c5e8df Compare August 4, 2020 21:04
@TysonMN TysonMN merged commit b4d835d into elmish:master Aug 4, 2020
@TysonMN TysonMN deleted the design_time_VMs_for_samples branch August 4, 2020 21:10
@BentTranberg
Copy link

I want to toy around a lot more than I did before I feel perfectly confident that the problem is gone

I finally got the time to do that, and the problem never surfaced again. Great.

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.

Add design-time view models to all samples
3 participants