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

Reunion Push Notifications API spec #458

Merged
merged 7 commits into from
Mar 12, 2021
Merged

Reunion Push Notifications API spec #458

merged 7 commits into from
Mar 12, 2021

Conversation

hulumane
Copy link
Member

No description provided.

@ghost ghost added the needs-triage label Feb 19, 2021
@hulumane hulumane changed the title Push spec draft v1 Reunion Push Notifications Public API spec Feb 19, 2021
@hulumane hulumane marked this pull request as ready for review February 19, 2021 00:57
@hulumane hulumane marked this pull request as draft February 19, 2021 00:58
@riverar
Copy link
Contributor

riverar commented Feb 19, 2021

👋 @hulumane, I see this is marked [draft], are you looking for feedback yet?

@hulumane
Copy link
Member Author

@riverar Yes we are looking for active feedback. There are still a few loose ends with the GetActivationArgs pattern that needs to be discussed.

@hulumane
Copy link
Member Author

@andrewleader @danielayala94 @sharath2727 Please note the first Public API draft

Copy link
Contributor

@andrewleader andrewleader left a comment

Choose a reason for hiding this comment

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

Looking nice! We need to change the class name of PushNotificationChannelManager to be something different than the existing inbox class name (otherwise it'll be confusing for developers). I'd suggest simply PushManager for now

@riverar
Copy link
Contributor

riverar commented Feb 19, 2021

Where does this work land, in terms of the Roadmap (https://github.com/microsoft/ProjectReunion/blob/main/docs/roadmap.md) timeline?

specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
@nicuparente
Copy link
Contributor

Where does this work land, in terms of the Roadmap (https://github.com/microsoft/ProjectReunion/blob/main/docs/roadmap.md) timeline?

@riverar - we are tentatively targeting this to land by the end of 2021. As soon as our plans become more solidified, we will get it added to the official roadmap. Of course, this is not final yet, but we plan to keep the community updated with this.

specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
@hulumane hulumane marked this pull request as ready for review February 23, 2021 21:46
@hulumane hulumane self-assigned this Feb 23, 2021
## Registration of the Push Activator for Win32 (Inproc)
If a Win32 app needs to subscribe to Push triggers inproc, the code in Main would follow the pattern below:
* The Registration will take in a CLSID of the COM server as part of the Activator setup. The registration API will simply be a thin wrapper around the BackgroundInfra WinRT APIs and abstract away the COM details from the developer.
* The app will query ApplifeCycle APIs to retrieve an ActivationKind. (Note: This is covered in a seperate AppLifeCycle API spec.)
Copy link
Member Author

Choose a reason for hiding this comment

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

@andreww-msft FYI. Will need a link to your spec to show how we are addressing the new Activation Kinds as discussed in yesterday's meetup. Thanks

@hulumane
Copy link
Member Author

@ShawnSteele This needs to go through the Public API review process

specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
@andrewleader andrewleader added the area-Notifications Toast notification, badges, Live Tiles, push notifications label Feb 25, 2021
@andrewleader andrewleader changed the title Reunion Push Notifications Public API spec Reunion Push Notifications API spec Mar 2, 2021
specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved

// Check to see if the WinMain activation is due to a Push Activator
// Note: This API is currently not present in Reunion but will be included as part of the AppLifecycle Public API spec.
auto activation = AppLifecycle::Activation();
Copy link
Member

Choose a reason for hiding this comment

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

https://osgwiki.com/wiki/Marshal-By-Value_for_ActivatedEventArgs describes the "by value" cases of activation. that works for designs that don't require references.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ChrisGuzak Externals can't see this resource, can you provide an excerpt?

Copy link
Member

Choose a reason for hiding this comment

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

The document describes a common way for in-Windows types to declare "I know how to marshal myself using a valueset rather that COM's marshal-by-reference default." The type dumps its data into a ValueSet which is itself serialized into a binary blob and squirted to the other process where it's reconstituted into a fresh in-target-process instance of the original type. It lets us reduce IPCs between platform and app components for many of the ...ActivatedEventArgs types since many of those are just "here's a string/uri/number/etc to act on." Currently it relies on an undocumented handshake between COM and the marshaling system, which is why it's limited to Windows.* types.

We look forward to bringing this to the platform SDK since it's of general use, especially in the work that @aeloros is doing around activation.

specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
auto pushArgs = args.as<IPushNotificationActivatedEventArgs>();

// Call TakeDeferral to ensure that code runs in low power
pushArgs.GetDeferral();
Copy link
Member

Choose a reason for hiding this comment

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

RECOMMEND: Follow standard deferral pattern from API design patterns repo

specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved

runtimeclass ChannelResult
{
ChannelResult(
Copy link
Member

Choose a reason for hiding this comment

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

DISCUSSION: Start of a pattern - allowing apps to create types they can use for testing or driving their own state machines.

specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
@hulumane hulumane force-pushed the user/pavanh/push-spec branch from d53965d to 4bd173c Compare March 8, 2021 23:34
specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved

**Push Channels**: All the Push APIs are dependent on the caller app having an identity at the minimal. However, even packaged Win32 apps that have an identity cannot use our Public APIs to subscribe to Push Channels because they may not be store provisioned. Sideloaded UWPs that aren't store distributed have exactly the same problem.
**Push Channels**: All the Push APIs are dependent on the caller app having an identity at the minimal. <br/>
Copy link
Member

Choose a reason for hiding this comment

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

"having an identity." (drop "at the minimal.

specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
specs/PushNotifications/PushNotifications-spec.md Outdated Show resolved Hide resolved
@hulumane hulumane dismissed stale reviews from andrewleader and DrusTheAxe March 12, 2021 20:13

The concern with winrt::guid was addressed. The new c++ winrt nuget packages allow us to pass in GUID strings when constructing a guid.

@hulumane hulumane merged commit 2804a10 into main Mar 12, 2021
@hulumane hulumane deleted the user/pavanh/push-spec branch March 12, 2021 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-design Updates to Project Reunion API surfaces area-Notifications Toast notification, badges, Live Tiles, push notifications
Projects
None yet
Development

Successfully merging this pull request may close these issues.