Removing workaround on App Notification Conferencing Config for calling IsCallingPreviewSupported#6090
Open
guimafelipe wants to merge 7 commits intomainfrom
Open
Removing workaround on App Notification Conferencing Config for calling IsCallingPreviewSupported#6090guimafelipe wants to merge 7 commits intomainfrom
guimafelipe wants to merge 7 commits intomainfrom
Conversation
Member
|
This seems like the right approach to me, but would love a second opinion. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ADO bug
This PR removes the workaround on
AppNotificationConferencingConfig.cppthat adds abool winrt::Microsoft::Windows::AppNotifications::AppNotificationConferencingConfig::IsCallingPreviewSupported()when the velocity key is not enabled for that feature.From the internal bug comment, it was suspected that the generation of the stripped winmd files was a possible reason for that inconsistency.
From my investigation, the above linked target has no influence on this issue. Removing it from the build process had no effect on fixing the linker issues when the velocity key is disabled.
According to the documentation, the terminal velocity features should just affect the stripped winmds generated by the target, but this is not what is happening. The terminal velocity features also affect the MIDL compilation, and the object files (and generated headers) resulted by it.
For the
AppNotificationConferencingConfig.cpp, depending on the velocity key state, we can have these two different scenarios:Key disabled
Key enabled
Getting the
AppNotificationManager.objas example, we have:The symbol it tries to import is only present on the
AppNotificationConferencingConfig.objgenerated with the velocity key enabled (symbol in the first line).On MIDL 3.0 documentation, there is no explanation on the detail of how the
featurekeyword works. For now, I am understanding that its behavior strips out the static method gatekept by the velocity key out of the generated headers, as that is what I saw by investigating them, as shown as following:Analyzing
Microsoft.Windows.AppNotifications.AppNotificationConferencingConfig.g.cppKey disabled
Key enabled
That way, I think the best solution is to not have this method as a winrt method conditioned to a velocity key that is called in different parts of our code, and instead have it as a regular C++ function that implementation changes depending on the velocity key locally, making it clear where the method is defined and removing the need of a workaround.
A microsoft employee must use /azp run to validate using the pipelines below.
WARNING:
Comments made by azure-pipelines bot maybe inaccurate.
Please see pipeline link to verify that the build is being ran.
For status checks on the main branch, please use TransportPackage-Foundation-PR
(https://microsoft.visualstudio.com/ProjectReunion/_build?definitionId=81063&_a=summary)
and run the build against your PR branch with the default parameters.