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

open xaml file first in editor #1007

Merged

Conversation

jmarolf
Copy link
Contributor

@jmarolf jmarolf commented Jun 19, 2019

fixes AB#832526

@ghost ghost requested review from vatsan-madhavan, rladuca, ryalanms and stevenbrix June 19, 2019 00:14
@ghost ghost added the PR metadata: Label to tag PRs, to facilitate with triage label Jun 19, 2019
@vatsan-madhavan vatsan-madhavan added the * NO MERGE * metadata: The PR is not ready for merge yet (see discussion for detailed reasons) label Jun 19, 2019
@vatsan-madhavan
Copy link
Member

@jmarolf - Please start by opening an issue per guidelines. Let's discuss what you are trying to achieve and we can identify the appropriate solution, timelines etc. fromt there. Same goes for #1005

@jmarolf
Copy link
Contributor Author

jmarolf commented Jun 19, 2019

@debonte can you file an issue on this repo and talk to @vatsan-madhavan to justigy this change?

@debonte
Copy link

debonte commented Jun 19, 2019

@jmarolf, I opened #1017

@debonte
Copy link

debonte commented Jun 19, 2019

@jmarolf, two thoughts on this change:

  1. It looks like this will result in only opening MainWindow.xaml. While this is the expected behavior for Blend, in VS the WPF Framework experience is that both MainWindow.xaml and MainWindow.xaml.cs/vb are open (and the XAML file is active).
  2. I wonder if the choice of which documents to open should be made in the .vstemplate (or at least be overridable there) instead of in the dotnet template?

@mgoertz-msft
Copy link

@vatsan-madhavan We have to do something about the Blend experience. It is not acceptable for Blend to open a C# file instead of a XAML file when creating a new project.

Perhaps there is something we can do with the HostIdentifier condition to get one experience in Blend and another in VS.

@stevenbrix
Copy link
Contributor

stevenbrix commented Jun 20, 2019

This change seems righteous, we should be striving to have the right user experience in both VS and Blend.

I wonder if the choice of which documents to open should be made in the .vstemplate (or at least be overridable there) instead of in the dotnet template?

@debonte i don't see a .vstemplate, but from the json schema it looks like you can have multiple primaryOutputs and specify which one is the default. Does that fix the scenario?

@debonte
Copy link

debonte commented Jun 20, 2019

it looks like you can have multiple primaryOutputs and specify which one is the default. Does that fix the scenario?

@stevenbrix I'm not super-concerned where it's done. I just thought the .NET Core SDK team might not want to encode VS/Blend-specific behavior in their templates.

I think HostIdentifier may currently be a fixed value for both VS and Blend (VSConfigurationHelpers.DefaultHostId), so I think that would need to be changed first.

Btw, from @jmarolf's changes it's not clear to me if its the primaryOutputs or the postActions that actually determines which files are opened.

i don't see a .vstemplate

Look for Wpf.vstemplate either under c:\Program Files (x86)\Microsoft Visual Studio<vs-instance>\Common7\IDE\Extensions or %LOCALAPPDATA%\Microsoft\VisualStudio<vs-instance-id>

@stevenbrix
Copy link
Contributor

stevenbrix commented Jun 20, 2019

Ah ok i see, i thought this was something in our repo, but those ship with VS.

I just thought the .NET Core SDK team might not want to encode VS/Blend-specific behavior in their templates.

I think this is a good general goal to strive for. How was this done in framework?

@debonte
Copy link

debonte commented Jun 20, 2019

I think this is a good general goal to strive for. How was this done in framework?

The .NET Framework .vstemplates are self-contained. They don't call into the dotnet template engine. Those .vstemplates have metadata for each file that is generated, some of which specifies which files should be opened and in which order. See #1017 for a snippet that shows how it controls the MainWindow.xaml/MainWindow.xaml.cs behavior.

@jmarolf
Copy link
Contributor Author

jmarolf commented Jun 21, 2019

The .NET Framework .vstemplates are self-contained. They don't call into the dotnet template engine. Those .vstemplates have metadata for each file that is generated, some of which specifies which files should be opened and in which order. See #1017 for a snippet that shows how it controls the MainWindow.xaml/MainWindow.xaml.cs behavior.

I'll look and see if there is a way to get this behavior in the .NET Core templates. Today this is all the action is doing (essentially DTE.ItemOperations.OpenFile for a given list of files).

@vatsan-madhavan
Copy link
Member

I was able to spend some time looking over the C# template changes - I'll assume that the discussion extends to the VB template symmetrically.

"open xaml file first in editor" is exactly what it seems to be doing - good so far. This is what I'm noticing overall relative to the spec #1017:

Spec Actual
VS - Open MainWindow.xaml & MainWindow.xaml.cs
- Focus on MainWindow.xaml
Per spec
Blend - Open MainWindow.xaml only
- Focus on MainWindow.xaml
- Opens MainWindow.xaml & MainWindow.xaml.cs
- Focus on MainWindow.xaml
(Behavior identical to VS)
  • Unless there is a way to differentiate Blend and VS today - for e.g., a difference in HostIdentifier - it's not clear to me that achieving parity with spec will be possible on Blend. Perhaps the tentative approach should be to accept the current limitation in Blend (i.e., the fact that MainWindow.xaml.cs is also opened in the background), until its HostIdentifier can be changed to something unique?
  • It's not clear to me either whether primaryOutputs or postActions that determines which file is opened. I think it's a combination of both.
  • Empirically, postActions.args.files seems to control the order in which the files appear. If we had 3 files in primaryOutputs listed in the order {MainWindow.xaml.cs, MainWindow.xaml, App.xaml}, and postActions.args.file="1;3;2", this is what you'd see:
    • image

What kind of testing has this proposed change undergone?

@mgoertz-msft
Copy link

OK, I guess I misunderstood. It looked like this change is needed to open the XAML file but you weren't going to take it because of the NO MERGE label.

As long as the XAML file is active in Blend on File->New Project then it is just a minor issue that there is also a C# file in the background. We could address that in a separate issue.

@vatsan-madhavan
Copy link
Member

OK, I guess I misunderstood. It looked like this change is needed to open the XAML file but you weren't going to take it because of the NO MERGE label.

To clarify, the NO MERGE label was added because the PR didn't have issue description, what was being fixed, testing details etc, and we didn't want an accidental merge before Preview 7 completed.

I also reached out @jmarolf by email asking for additional details, and explaining that we just had a regression in project templates and wanted to be careful in Preview 7.

Once P7 is done, we'll remove the NO MERGE label.

@mgoertz-msft
Copy link

Sounds good, thank you.

@vatsan-madhavan vatsan-madhavan removed the * NO MERGE * metadata: The PR is not ready for merge yet (see discussion for detailed reasons) label Jun 26, 2019
@vatsan-madhavan
Copy link
Member

Removing NO MERGE since #1100 is completed and master has switched to Preview 8. This PR can continue on with usual due-diligence.

Copy link
Member

@vatsan-madhavan vatsan-madhavan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you decided that opening both .xaml and .cs simultaneously (with focus on .xaml) will be an acceptable experience for Blend?

@mgoertz-msft
Copy link

I just chatted with DoRon and as long as the .xaml file ends up being the active one then that would be acceptable for Blend.

@vatsan-madhavan
Copy link
Member

I just chatted with DoRon and as long as the .xaml file ends up being the active one then that would be acceptable for Blend.

Thanks, that's good to know!

@jmarolf, do you have a sense for when we can expect an update from you re: other open questions?

@jmarolf
Copy link
Contributor Author

jmarolf commented Jul 16, 2019

@vatsan-madhavan ready for review

@vatsan-madhavan
Copy link
Member

@jmarolf - lgtm!

@vatsan-madhavan vatsan-madhavan merged commit 08e3f07 into dotnet:master Jul 17, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Apr 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
PR metadata: Label to tag PRs, to facilitate with triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants