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.

Sign up for GitHub

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

Jump to bottom

Interfaces for "Enhanced UX Notification for Video and Audio Call Feature" #4783

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Interfaces for "Enhanced UX Notification for Video and Audio Call Feature" #4783

wants to merge 15 commits into from

Conversation

satkh
Copy link

@satkh satkh commented Oct 8, 2024 •
edited
Loading

[Experimental]
This Pull Request adds required interfaces for "Video Or Audio calling" feature on App Notifications.

Feature proposal link : #4809

Spec link : #4824

@satkh
Copy link
Author

satkh commented Oct 16, 2024

@satkh please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@satkh
Copy link
Author

satkh commented Oct 16, 2024

@microsoft-github-policy-service agree [company="Microsoft"]

@satkh
Copy link
Author

satkh commented Oct 16, 2024

@microsoft-github-policy-service agree company="Microsoft"

@anupriya13 anupriya13 linked an issue Oct 17, 2024 that may be closed by this pull request
[Experimental] Adding interfaces for Feature "Enhanced UX Notification for Video and Audio Call" #4809
Open
@anupriya13 anupriya13 added area-Notifications Toast notification, badges, Live Tiles, push notifications api-design Updates to Project Reunion API surfaces feature proposal labels Oct 17, 2024
@anupriya13 anupriya13 added this to the 1.7 milestone Oct 17, 2024
@jonwis
Copy link
Member

jonwis commented Oct 17, 2024

API design specs usually have clear "this is how an app would use it" samples. I see updates to tests, but not "Contoso wants to create a toast showing you this..."

@jonwis
Copy link
Member

jonwis commented Oct 17, 2024

OK, so this feature is about "before you answer the call, configure your local devices in this way"? So I can see what I'm going to look like before I answer the call?

Is there another API coming that lets the application provide a video feed of the incoming caller so you can see who you're going to connect with?

jonwis
jonwis reviewed Oct 24, 2024
View reviewed changes
dev/AppNotifications/AppNotificationBuilder/AppNotificationBuilder.idl Outdated Show resolved Hide resolved
dev/AppNotifications/AppNotificationBuilder/AppNotificationBuilder.idl Outdated Show resolved Hide resolved
dev/AppNotifications/AppNotificationBuilder/AppNotificationBuilder.idl Outdated Show resolved Hide resolved
dev/AppNotifications/AppNotificationBuilder/AppNotificationBuilder.idl Outdated Show resolved Hide resolved
dev/AppNotifications/AppNotificationBuilder/AppNotificationButton.cpp Show resolved Hide resolved
dev/AppNotifications/AppNotifications.idl Outdated Show resolved Hide resolved
dev/AppNotifications/NotificationDevicesData.cpp Outdated Show resolved Hide resolved
dev/AppNotifications/NotificationProperties.cpp Outdated Show resolved Hide resolved
@anupriya13
Copy link

anupriya13 commented Nov 9, 2024 •
edited
Loading

Let's wait for FrameworkUDK changes to be checked in here before merging

jonwis
jonwis approved these changes Nov 18, 2024
View reviewed changes
Copy link
Member

@jonwis jonwis left a comment

Choose a reason for hiding this comment

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

Please fix the tooltip topic before you merge. The "be inert on unsupported" is optional to fix.

dev/AppNotifications/AppNotification.cpp

void AppNotification::ConferencingConfig(winrt::Microsoft::Windows::AppNotifications::AppNotificationConferencingConfig const& conferencingConfig)
{
THROW_HR_IF(E_NOTIMPL, !AppNotificationConferencingConfig::IsCallingPreviewSupported());
Copy link
Member

Choose a reason for hiding this comment

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

These APIs should "do nothing" if calling preview is unsupported. Allow the caller to set/read the value, but don't use it in any way. And don't throw. :)

Copy link
Author

Choose a reason for hiding this comment

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

But how do we inform the user that this feature is not yet supported when they call these APIs?

dev/AppNotifications/AppNotificationBuilder/AppNotificationButton.cpp Show resolved Hide resolved
@satkh satkh changed the base branch from main to release/1.7-stable November 19, 2024 06:03
@satkh satkh changed the base branch from release/1.7-stable to main November 19, 2024 06:03
@satkh
Copy link
Author

satkh commented Nov 19, 2024

Please fix the tooltip topic before you merge. The "be inert on unsupported" is optional to fix.

This "m_toolTip" is not added as part of this feature its existing.
I see those special characters (<,>,',") already escaped in the "set" method below.

image

codendone
codendone requested changes Nov 19, 2024
View reviewed changes
dev/AppNotifications/AppNotifications.idl Outdated Show resolved Hide resolved
dev/AppNotifications/AppNotifications.idl Outdated Show resolved Hide resolved
@satkh satkh requested a review from codendone November 20, 2024 06:46
codendone
codendone approved these changes Nov 20, 2024
View reviewed changes
Copy link
Contributor

@codendone codendone left a comment

Choose a reason for hiding this comment

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

:shipit:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anupriya13 anupriya13 anupriya13 approved these changes

@codendone codendone codendone approved these changes

@jonwis jonwis jonwis approved these changes

@pmpurifoy pmpurifoy Awaiting requested review from pmpurifoy

@EricJohnson327 EricJohnson327 Awaiting requested review from EricJohnson327

@danielayala94 danielayala94 Awaiting requested review from danielayala94

@vivekkhare31 vivekkhare31 Awaiting requested review from vivekkhare31

@JesseCol JesseCol Awaiting requested review from JesseCol

@SatwikKrSharma SatwikKrSharma Awaiting requested review from SatwikKrSharma

Assignees
No one assigned
Labels
api-design Updates to Project Reunion API surfaces area-Notifications Toast notification, badges, Live Tiles, push notifications feature proposal needs-triage
Projects
None yet
Milestone
1.7
Development

Successfully merging this pull request may close these issues.

[Experimental] Adding interfaces for Feature "Enhanced UX Notification for Video and Audio Call"
5 participants
@satkh @jonwis @anupriya13 @danielayala94 @codendone