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

Add Workflow definition build rule (XamlAppDef) #3810

Merged
merged 8 commits into from
Aug 1, 2018

Conversation

wjk
Copy link
Contributor

@wjk wjk commented Jul 31, 2018

Does mostly what the title says. Some notes:

  • I copied the XamlAppDef.xaml file from None.xaml, because I didn't think that any extra properties would be required. If additional properties needs to be placed in this file, please let me know.
  • When I load a project that uses this functionality in the VS Experimental Instance, I still see the issue described in XamlAppDef not available #3722 (comment). I think this is because the updated design-time targets are not being referenced properly. Again, please let me know if I've missed anything here.

Thanks!

@wjk wjk requested a review from a team as a code owner July 31, 2018 01:25
@jp2masa
Copy link
Member

jp2masa commented Jul 31, 2018

When I load a project that uses this functionality in the VS Experimental Instance, I still see the issue described in #3722 (comment).

Did you try setting the Context to File;BrowseObject?

@@ -171,6 +171,10 @@
<Context>File</Context>
</PropertyPageSchema>

<PropertyPageSchema Include="$(ManagedXamlResourceDirectory)XamlAppDef.xaml">
<Context>File</Context>
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be 'File;BrowseObject'.

@davkean
Copy link
Member

davkean commented Jul 31, 2018

F5 will correctly setup the environment to pick up new rules + targets, so if Experimental instance isn't working, then the bug hasn't been fixed.

Also you need to modify setup\Microsoft.VisualStudio.ProjectSystem.Managed.CommonFiles\CommonFiles.swr to make sure the new rule gets installed.

@davkean davkean added the Community Pull request from the community. Thanks for your contribution! label Jul 31, 2018
@codecov-io
Copy link

codecov-io commented Jul 31, 2018

Codecov Report

Merging #3810 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3810   +/-   ##
=======================================
  Coverage   68.36%   68.36%           
=======================================
  Files         595      595           
  Lines       34248    34248           
  Branches     1935     1935           
=======================================
  Hits        23414    23414           
  Misses      10484    10484           
  Partials      350      350
Flag Coverage Δ
#debug 68.36% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 82d4c0f...b49e570. Read the comment docs.

@wjk
Copy link
Contributor Author

wjk commented Jul 31, 2018

Now this is really weird...

I added BrowseObject to the XAML file as @jp2masa suggested (in commit 71cb46d), and yet the Experimental Instance still doesn't show the file! I looked inside the Extensions folder for the Exp root-suffix, and none of the design-time targets files show up anywhere! Is this expected, or is something else going wrong?

@davkean
Copy link
Member

davkean commented Jul 31, 2018

This should cause us to pick it up from the recently build artifacts folder. Look in the Output window of the debugger for CPS tracing that might indicate what's going on. I can look latter to see what's going on.

@wjk
Copy link
Contributor Author

wjk commented Jul 31, 2018

I'll do so tomorrow, after I get off work. Thanks!

@wjk
Copy link
Contributor Author

wjk commented Jul 31, 2018

@davkean I've looked at the Output window, and see nothing that looks like CPS tracing. All I see are a lot of rote assembly load/unload log statements. Here's the full log.

Name="XamlAppDef"
DisplayName="General"
PageTemplate="generic"
Description="File Properties"
Copy link
Member

Choose a reason for hiding this comment

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

This is missing PropertyPagesHidden="true"

@davkean
Copy link
Member

davkean commented Jul 31, 2018

I'm debugging this to see why this is occurring. This occurs because the property as called above is spelt wrong.

As a side note, I'd like this contribution to be squashed - feel free to do it, or I'll do when I merge this in. I'm going to use it as an example for adding a new item type.

@@ -171,6 +171,10 @@
<Context>File</Context>
</PropertyPageSchema>

<PropertyPageSchema Include="$(ManagedXamlResourceDirectory)XamlAppDef.xaml">
Copy link
Member

Choose a reason for hiding this comment

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

This property is missing an "s"; should be $(ManagedXamlResourcesDirectory).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, but it still doesn't seem to work. Please keep looking. Thanks!

Category="Misc"
Description="The item specified in the Include attribute.">
<StringProperty.DataSource>
<DataSource Persistence="Intrinsic" ItemType="None" PersistedName="Identity" SourceOfDefaultValue="AfterContext" />
Copy link
Member

Choose a reason for hiding this comment

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

Replace every usage of ItemType="None" with ItemType="XamlAppDef".

Copy link
Member

Choose a reason for hiding this comment

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

Even better, delete the ItemType from all properties. This is automatically pulled from the DataSource on the rule itself.

Copy link
Member

Choose a reason for hiding this comment

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

Filed to #3815 to basically do this across all the rules so we never run into this again.

Copy link
Member

Choose a reason for hiding this comment

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

Also filed #3816. We have tracing for a bunch of this, but we hook up too late for it to be output into the Output window.

Copy link
Member

Choose a reason for hiding this comment

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

Strike removing the ItemType, I'm replacing it with XamlAppDef. It's required for the BrowseObject but not for the File context.

@davkean
Copy link
Member

davkean commented Aug 1, 2018

With the latest changes I've just pushed, this works. I'll merge it in when we are green.

@davkean davkean merged commit fc7b8b6 into dotnet:master Aug 1, 2018
@davkean davkean mentioned this pull request Aug 1, 2018
@wjk wjk deleted the XamlAppDef branch August 1, 2018 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community Pull request from the community. Thanks for your contribution!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants