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

Publish New AppNotificationBuilder API #2834

Merged
merged 12 commits into from
Aug 9, 2022
Merged

Conversation

loneursid
Copy link
Contributor

@loneursid loneursid commented Aug 8, 2022

  • Add projection project to build the AppNotification winmd file.
  • Copy the new AppNotificationBuilder winmd to the publishing directory
  • Publish the new builder API in build/NuSpecs/AppxManifest.xml
  • Add contracts to the API in the IDL

I've tested by downloading the nuget, installing it in two of our samples (one C++, one C#) and using the new APIs. I can confirm that the new APIs can be used in the samples and the samples build and run as expected.

@ghost ghost added the needs-triage label Aug 8, 2022
@loneursid loneursid self-assigned this Aug 8, 2022
@loneursid loneursid added the area-Notifications Toast notification, badges, Live Tiles, push notifications label Aug 8, 2022
@loneursid loneursid added this to the 1.2 milestone Aug 8, 2022
@loneursid
Copy link
Contributor Author

/azp run TransportPackage-Foundation-PR

@loneursid loneursid marked this pull request as ready for review August 9, 2022 00:14
@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@loneursid loneursid requested a review from MikeHillberg August 9, 2022 00:14
@loneursid
Copy link
Contributor Author

/azp run TransportPackage-Foundation-PR

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@loneursid
Copy link
Contributor Author

/azp run TransportPackage-Foundation-PR

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@loneursid
Copy link
Contributor Author

/azp run TransportPackage-Foundation-PR

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@loneursid loneursid marked this pull request as draft August 9, 2022 04:24
@loneursid
Copy link
Contributor Author

/azp run TransportPackage-Foundation-PR

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@loneursid loneursid changed the title Adding C# projections and contracts for builder API Adding Projections and Contracts for builder API Aug 9, 2022
@loneursid loneursid marked this pull request as ready for review August 9, 2022 16:41
@loneursid loneursid changed the title Adding Projections and Contracts for builder API Publish New AppNotificationBuilder API Aug 9, 2022
@jonwis
Copy link
Member

jonwis commented Aug 9, 2022

We should talk about how to test this e2e - is there a test app in the repo that would have consumed this correctly via the nuspec?

There needs to be one in the Samples repo that shows a great use, so we'd have that as a cross-check.

@loneursid
Copy link
Contributor Author

loneursid commented Aug 9, 2022

We should talk about how to test this e2e - is there a test app in the repo that would have consumed this correctly via the nuspec?

There needs to be one in the Samples repo that shows a great use, so we'd have that as a cross-check.

I've used the nuget (downloaded from the output of the build pipeline) and samples to test and verify this and I can confirm that the sample builds with the new APIs. The issue is that the pipelines only produce a nuget, no installer or the 4 MSIX needed, so it can't run. The nightly build produces everything, but I'd need to merge this first in order to get the binaries.

@loneursid
Copy link
Contributor Author

loneursid commented Aug 9, 2022

We should talk about how to test this e2e - is there a test app in the repo that would have consumed this correctly via the nuspec?
There needs to be one in the Samples repo that shows a great use, so we'd have that as a cross-check.

I've used the nuget (downloaded from the output of the build pipeline) and samples to test and verify this and I can confirm that the sample builds with the new APIs. The issue is that the pipelines only produce a nuget, no installer or the 4 MSIX needed, so it can't run. The nightly build produces everything, but I'd need to merge this first in order to get the binaries.

@jonwis - I was wrong about the testing stuff. Thanks to @kythant for setting me up straight. I've been able to test the feature e2e using our samples. The msix were produced by the pipelines. I was just not looking at the right place.

</ItemGroup>

<ItemGroup>
<ProjectReference Include="..\Microsoft.Windows.AppNotifications.Projection\Microsoft.Windows.AppNotifications.Projection.csproj" />
Copy link
Member

Choose a reason for hiding this comment

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

I misunderstood from the email thread. So you have the project reference, but it only picks up the projection assembly and not the metadata? Odd. We can debug with a binlog at some point, but the Reference below is fine for now.

Copy link
Contributor Author

@loneursid loneursid Aug 9, 2022

Choose a reason for hiding this comment

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

What I was trying to say is that after adding the dependency on the project, I was expecting that this wouldn't be necessary:

<Reference Include="Microsoft.Windows.AppNotifications">
<HintPath>$(OutDir)..\WindowsAppRuntime_DLL\StrippedWinMD\Microsoft.Windows.AppNotifications.winmd</HintPath>
<IsWinMDFile>true</IsWinMDFile>
</Reference>

but it turns out it is still needed. (VS should be able to infer this information from the reference project)

@loneursid loneursid merged commit 557625f into develop Aug 9, 2022
@loneursid loneursid deleted the user/erlangl/PublishBuilder branch August 9, 2022 18:32
@DrusTheAxe
Copy link
Member

DrusTheAxe commented Oct 11, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Notifications Toast notification, badges, Live Tiles, push notifications needs-triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants