Closed mrlacey closed 6 years ago
We were looking at the user activity docs and we could do the following:
We can set up a poc, to see if uri scheme needs any adjustments to support this.
We are working on an ActivityFeed PoC. ActivityFeed.zip
The PoC contains funcionallity that could result in two different templates:
The PoC contains changes some modifications to the current UriScheme template:
Update SchemeActivationHandler: We added SchemeActivationData to resolve the page type and parameters from the Uri.
protected override async Task HandleInternalAsync(ProtocolActivatedEventArgs args)
{
// Create data from activation Uri in ProtocolActivatedEventArgs
var data = SchemeActivationData.CreateFromUri(args.Uri);
if (data.IsValid)
{
NavigationService.Navigate(data.PageType, data.Parameters);
}
else if (args.PreviousExecutionState != ApplicationExecutionState.Running)
{
// If the app isn't running and not navigating to a specific page based on the URI, navigate to the home page
NavigationService.Navigate(typeof(Views.MainPage));
}
await Task.CompletedTask;
}
Any comments or suggestion on this PoC?
As the use of adaptive cards is optional when creating an activity record, was any consideration made to providing a solution that doesn't use them?
The ActivityFeedService
has nothing WTS specific in it and could be helpful to a wider range of people if part of the Community Toolkit. Has any consideration been given to this option? (Possibly related to https://github.com/Microsoft/WindowsCommunityToolkit/issues/1182)
SchemaActivationData
seems like a confusing mix of functionality from different features. The use of a ViewsNamespace
magic string feels very bad. The need for the SchemeActivationData.IsValid
feels like it's hiding what should be an exception from the constructor.
Is there a realistic argument or use-case for creating timeline entries without having the app support deep links?
As this really needs an example page to show launching the app in different scenarios. Can we use this as an opportunity to review the way that different features handle launching and example pages in different ways so that we have consistency, remove duplication/differences, and provide something that works out of the box regardless of the features selected?
We could remove the Sample page from the UriScheme Template and explaing in the documentation what developers could do with the activation data passed from HandleInternalAsync to Page OnNavigatedTo event.
I'd much rather have something in the generated code that works and shows an example of use rather than separate docs. Doing this means that we can more easily test the generated apps, and removes possible issues with people trying to follow/understand the docs just to get to a point where something works. It also means we need to write and maintain less documentation.
The ActivityFeedService has an option to create UserActivity without AdaptiveCard (first overload of AddUserActivityAsync method). The service is currently receiving ActivationData, which would be WTS specific, but we can replace that by passing the uri and add it to the CommunityToolkit, if they're interested.
The idea of SchemaActivationHandler and SchemaActivationData was to handle all activations from uri in the same way with PageType and parameters.
Regarding the magic string: An alternative would be adding a method that returns all the possible pages for activation, and compare the page from the uri to those returned from the method Something like this:
private static IEnumerable<Type> GetActivationPages()
{
//TODO WTS: Add your pages for activation here.
//i.e.: yield return typeof(MainPage);
yield break;
}
Another alternative would be removing the SchemaActivationData an adding a switch on the SchemaActivationHandler to navigate to different pages.
Regarding the samples, I'm not sure what's better. I'll open another issue so we can have the discussion there, as this is related to various templates.
Activity feed and deep linking go hand in hand. This was the primary reason why we added in this feature as so we could do timeline launching. Deeplinking is useful for other reasons as well but this was the primary usecase.
Changes in PoC:
I just tried this from downloading and running, i can't get anything to run. VS launches but no window actually launches.
I forgot a checked property "Do not launch" while I was debugging and before create the Zip file.
Here is the same project with this property unchecked.
how do you test the functionality? i ran it but nothing popped up on the timeline and just has a main page. Unclear what steps need to happen to test this.
UserActivity is added to TimeLine in MainPage.xaml.cs OnNavigatedTo method. You can debug the App activation on SchemeActivationHandler HandleInternalAsync method.
so just running the app should have had something show up in my timeline, correct?
Yes
@crutkas Did you get this working on your machine? If not have you tried to uninstall all instances of the "ActivityFeed" poc app from your machine and then retry?
ok. feedback here
We were applying your feedback changes:
What do you think?
@mvegaca when trying the application out, i clicked the timeline button and nothing happened. I would imagine i would see some type of validation something happened with a todo for the dev to take the appropriate action that shows briefly how to handle the data. I'm fine sending it to the sample page like we do with deep linking to be consistant.
Will happily try out a new version
Working on it ;)
Updates in PoC.
SchemeActivationSamplePage
.SchemeActivationSamplePage
will be added on SchemeActivationData GetActivationPages
method.ActivationService StartupAsync
method.SchemeActivationSamplePage
like on the old UriSchemeExample.Zip with updated PoC ActivityFeed.zip
General
After much reflection and investigation into what we should include in the generated code (and why), I have the following proposal.
There are four scenarios relating to adding an entry into the timeline.
Scenarios 2, 3, & 4 are highly dependent upon the functionality of the app and so aren't scenarios we can include by default. There's an argument for including code to handle scenario two if the media page is also included. However, we don't yet have any other functionality that we add based on a combination of selected items. We could add this, but I don't think this warrants it. To handle this in right-click>Add scenarios would also add complications that would best be avoided for now based on the other refactoring that's happening for 3.0. Adding documentation that explains how to add code to handle scenarios 2 & 3 would, I believe, be a warranted addition. The precise requirements for this documentation would need defining. Scenario 4 is so rare I think we can ignore it.
The POC
AdaptiveCardsService.Sample.cs
seems unnecessary. Why add an extra file when the single helper method it contains could have been a private method in the class where it's used?MainPage.xaml.cs
for a piece of code should only appear in the documentation, but that code is then used later in the function. It's not clear which code is being proposed as for the documentation and which should be generated.
//// Fill the UserActivity with Title (mandatory), Description (optional) and BackgroundColor (optional)
ActvitityFeedService
contains what looks like generic helper functions but it includes as mandatory, parameters that the comments list as optional. This inconsistency is potentially confusing at best and should be avoided.OnNavigatedTo
method without consideration for the direction of navigation. If the use of the app means that a person would normally navigate back and forth between the main page and others, this will lead to the code being executed multiple times when this is not necessary.SchemeActivationData
and the POC as a whole seems over-engineered and tries to bundle together a lot of functionality that may not be truly necessary. If an app might only ever open a single page via a deep link, all that is added with SampleActivationData adds up to a lot of extra code.ActivityFeedService
, this feels like an incomplete attempt to create a general-purpose timeline entry helper. As we don't know what may be appropriate with the generated app, rather than provide two options, I think it would be simpler to add the code for creating the entry without the AdaptiveCard and then we can point to docs for how to use an AdaptiveCard. The documentation would allow the space to better show what was possible with AdaptiveCards rather than just a single coded example. Only including one option in the generated code by default would mean the less code is generated that will never be used and a dependency that may not be needed is not included.@mrlacey, from some of your comments I'm not sure if you were looking at the latest version of ActivityFeed PoC.
There are various versions of the Poc in this thread and I like the idea that you can follow the evolution of those, but we have to make sure everybody knows which is the last one.
I'll update the first item in the issue to have a link to the latest version. We did some changes in the latest version, you can see them here: ActivityFeed.zip
Regarding the sample page: We've modified the sampleactivation page to show all parameters that it receives, to make it work for UriScheme and UserActivity features.
Regarding the scenarios: I think it's a good idea to add the user activity on app launch. We've added a comment that this is a sample and developers should move the code to where they consider convenient in their app, we can also point to a document on how to implement scenarios 1-3
Regarding the adaptiveCard/text and description: I think we should add both options to the code. Adaptive Cards seem to me more powerfull/recommended option. We've changed the UserActivityService to make the difference between the two options clearer.
Regarding the uris: We modified the SchemeActivationHandler to use a dictionary to map between pages and url to avoid exposing internal data. We also simplified the SchemeActivationData class a bit and separated configuration in its own SchemeActivationConfig.
I took the zip file from the latest comment with a link that's different to the one you linked. If only we had a system for sharing source code that made it easy to see the changes.....
The refactored UserActivityService
does now make a better case for providing support for both plain text and AdaptiveCard options. However
AddAsync
is not a descriptive method name, especially in a static context. Have alternatives been explored?UserActivityData
that are optional are not clear.The use of SchemeActivationConfig
adds a lot more complexity than I feel is necessary.
It would still be good to see a comment in the code that shows a URL that will work with the code that is generated.
For the text that's included in the sample activity using an AdaptiveTile, it would be better to show text and formatting that doesn't lead to unnecessary truncation of text. It may only be a sample but that doesn't mean basic design considerations can be ignored.
This feature is supposed to work on machines running the Fall Creators Update, but when run on such a machine it just fails silently. This is the default behavior of the underlying functionality but if using this app on a machine running FCU it could be interpreted as meaning that something is broken or has gone wrong. I understand that in the future activities created on an FCU machine may one-day show up in the timeline on another device but they don't yet. For now it could easily appear that the functionality is broken. I understand the desire to get this functionality into the templates as quickly as possible but wonder if it's worth waiting until 1803 is the min target or support for entries appearing on other devices is enabled. If not, then a suitable warning should be included in the code comments.
@mrlacey, I added the PoC to a github repo, to be able to track changes, you can find it here: https://github.com/sibille/ActivityFeedPoC
I applied the following changes:
Regarding activation there are basically two options:
The first option is implemented in ActivityFeed project, I've added a second project called ActivityFeed.Switch with an implementation for the second.
I think what you need depends on the complexity of your project, if you only go to one page as in the sample, you don't need that mapping, if you have different options I like the SchemaActivationData approach better than the switch.
@crutkas, what do you think?
We are going to start working on UserActivity templates based on with activation config approach (page mapping), we also have the OK from the product team.
When activating an SplitView app from the timeline, the title is not showing correctly as the page is not added to the menu.
Docs need some polishing too
The NavigationView Header is bind to Selected Item, and SchemActivationPage is not available on NavigationView menu items. We could add this activation page to the menu items but we are not sure if we have to do that, because this page is a removable page to let user known how to parse and use activation parameters, it is not a section in the app.
Here is the result of adding the page to the menu items in NavigationView.
Updated docs, I think we should not add the sample page to the NavView to fix the header as it is not really part of the app and this means more code to delete. If you change the activation to go to any page added to the NavView the header will show fine. @mrlacey, @crutkas what do you think?
can we recap the exact issue here end to end @sibille ? Thanks! Long thread here and want to be sure we best understand the issue and the proposed solution
@crutkas, sure, sorry. Actually I'll open a new one, to discuss this apart, as the issue is about deep linking and navigation view and happens with the current deeplinking implementation in production too. X-Ref: #2528
Verified in dev-nightly: Templates version: 0.18.18242.2 Wizard version: 0.18.18242.1
Add a new feature that includes the ability to easily add items to the user's activity feed (as used by timeline)
Need to identify what this functionality should be and if/how it should be encapsulated.
Review renaming the current 'URI Scheme' feature to make it clearer that it relates to deep linking into the app. This can also relate to a protocol launch without any deep linking though.
Updating with Action plan:
Latest version of the PoC: https://github.com/sibille/ActivityFeedPoC