Bbowman/file activation by bbowman · Pull Request #1100 · microsoft/WinObjC

bbowman · 2016-10-06T21:38:36Z

Adds the ability to activate an islandwood app via a file.

aballway · 2016-10-06T21:52:29Z

+    VERIFY_IS_NOT_NULL(functor, WEX::Common::String().Format(L"The callback for %ls has not been set!", name));
 }
-#elif defined(ASSERT_NE_MSG)
+#elif defined(ASSERT_TRUE_MSG)


is there a better way to split between functional/unit tests than these ifdefs? #WontFix

no

In reply to: 82293757 [](ancestors = 82293757)

this is the easiest way to prefer gtest if present but not break back compat

In reply to: 82296551 [](ancestors = 82296551,82293757)

aballway · 2016-10-06T21:53:35Z

 /////////////////////////////////////////////////////////////
 #define BUILD_MOCK_METHOD_IMPL(METHOD_NAME, PARAMETER_COUNT, IS_CONST, ...)                                                               \
-    \
+                                                                                                                                          \


super nit: get rid of spaces here, just newlines #Closed

aballway · 2016-10-06T21:55:05Z

 #pragma once

 #import <UIKit/UIApplication.h>
+#import <UWP/WindowsFoundation.h>


nit: are these includes needed here? #ByDesign

well you use WMSSpeechRecognitionResult so you were getting lucky with the include ordering before ....

In reply to: 82294162 [](ancestors = 82294162)

IIRC not including them in a header was intentional, something funky happened with the linker because of how our modules take ownership of headers or something to that effect. I'd prefer sticking to forward declarations until that's resolved so someone doesn't accidentally stumble into that wasp nest #ByDesign

I prefer doing the right thing and fixing issues preventing us from doing the right thing rather than leaving scary things hidden for someone else to deal with. If there was "something funky" lets address that rather than running from it. Additionally this is an Internal header and thus shouldn't be subjected to modules.

In reply to: 82297724 [](ancestors = 82297724)

aballway · 2016-10-06T21:57:47Z

 // Use text value to guarantee it is the same argument we create
 @interface CortanaVoiceCommandForegroundTestDelegate : NSObject <UIApplicationDelegate>
-@property (nonatomic, readonly) NSMutableDictionary* methodsCalled;
+@property (nonatomic, readonly) StrongId<NSMutableDictionary> methodsCalled;


<3 #ByDesign

😒 this does not seem ideal #WontFix

sweet emoji. What part seems non ideal? Having a strongID prop seems fine to me. I don't particularly like the methodsCalled doodad but it works and follows AJs pattern

In reply to: 82296469 [](ancestors = 82296469)

But why? I am not seeing what this is actually addressing.

In reply to: 82297024 [](ancestors = 82297024,82296469)

Memory leaks from _methodsCalled = [NSMutableDictionary new]; in the old implementation. There was no corresponding release in the dtor. The options were 1. add a dtor 2. use strong id 3. turn on arc. I chose 2 but @DHowett-MSFT seems less enthused with that choice.

In reply to: 82302619 [](ancestors = 82302619,82297024,82296469)

Since this is test code, I dont have a strong preference. In framework code, we have been doing 1 consistently, we should probably do the same here. (adding -dealloc is quite simple and less intrusive).

In reply to: 82303181 [](ancestors = 82303181,82302619,82297024,82296469)

aballway · 2016-10-06T22:00:53Z

-    _methodsCalled[NSStringFromSelector(_cmd)] = @(YES);
-    return true;
+    [_methodsCalled setObject:@(YES) forKey:NSStringFromSelector(_cmd)];
+    return;


nit: don't need return #WontFix

aballway · 2016-10-06T22:01:12Z

 }

- (BOOL)application:(UIApplication*)application didReceiveProtocol:(WFUri*)uri {
+- (void)application:(UIApplication*)application didReceiveProtocol:(WFUri*)uri {


interesting that this still worked before, thanks #Resolved

aballway · 2016-10-06T22:02:01Z

@@ -0,0 +1,194 @@
+//******************************************************************************
+//
+// Copyright (c) 2016 Microsoft Corporation. All rights reserved.


nit: update copyright #Resolved

I assume I should update your tests too then?

In reply to: 82295347 [](ancestors = 82295347)

Probably <3 #Resolved

aballway · 2016-10-06T22:02:54Z

+
+@implementation FileActivationForegroundTestDelegate
+- (id)init {
+    self = [super init];


extra nit: put in if() #Resolved

I'll update yours too I guess

In reply to: 82295491 [](ancestors = 82295491)

bbowman · 2016-10-06T23:05:40Z

 * #import <UWP/WindowsFoundation.h>
 * #import <UWP/WindowsMediaSpeechRecognition.h>
+ * #import <UWP/WindowsApplicationModelActivation.h>
 */


this is probably what you were thinking of @aballway and this has an todo so we are at least tracking the work here. It likely is due to circular deps between Projections and Foundation/Other frameworks creating the issue which I'd love us to solve at some point.

jaredhms · 2016-10-06T23:17:37Z

 - (void)application:(UIApplication*)application didReceiveRemoteNotification:(NSDictionary*)userInfo;
 - (void)application:(UIApplication*)application didReceiveToastAction:(NSDictionary*)action;
 - (void)application:(UIApplication*)application didReceiveVoiceCommand:(WMSSpeechRecognitionResult*)result;
+- (void)application:(UIApplication*)application didReceiveFile:(WAAFileActivatedEventArgs*)result;


I think I'd much prefer using the existing iOS API surface for this (appdidfinishlaunchingwithoptions); why force the app devs to change their code? #ByDesign

appDidfinishLaunchingWithOptions gets called only the app is transition from a non-foreground state to a foreground state, but in UPW onActivated gets called irrespective of the apps state (it is upto the application to handle this) and if for some reason the application wants to always be notified of an activation the only way itcan do that is by implementing this delegate. The delegate is guarenteed to be called after every appDidfinishLaunchingWithOptions call and also when an activation occurs while the app was already in foreground

In reply to: 82305070 [](ancestors = 82305070)

it seems they are different patterns with the ability for multiple files and Verbs (open vs post vs edit, etc) and the ability to be called while active as Nithish points out. In light of that, I think the pattern here should be used rather than reducing fidelity / trying to cram extra parameters into an options dictionary that probably wouldn't be checked and be incongruous with the other delegate call.

In reply to: 82463028 [](ancestors = 82463028,82305070)

rajsesh · 2016-10-06T23:38:09Z

ms-jihua · 2016-10-07T17:58:35Z

  // object, returns an empty string.
  const char* message() const {
-    return message_.get() != NULL ?  message_->c_str() : "";
+    return success_ ? "" : (message_.get() != NULL ?  message_->c_str() : "");


nit: this is kind of gross

I guess you could change it to (!success_ && message_.get()) ? message_->c_str() : ""; ? does that even look better? #WontFix

ms-jihua · 2016-10-07T18:02:43Z

mnithish · 2016-10-07T20:24:32Z

    UIApplicationActivated(args);
 }

+void App::OnFileActivated(FileActivatedEventArgs^ args)


You need to also edit msvc\vsimporter-templates\WinStore10-App\App.xaml.cpp and msvc\vsimporter-templates\WinStore10-App\App.xaml.h file for this #Resolved

And to provide more context, we updated vsimporter to use App.Xaml.cpp few iterations back so StarboardXaml is not used any more if someone tries to vsimport their iOS project. We still have to maintain both these code paths so we do not have to force existing developer from having to re-import their project

In reply to: 82463737 [](ancestors = 82463737)

mnithish · 2016-10-07T20:25:22Z

🕐

aballway · 2016-10-11T17:50:20Z

-    virtual void OnBackgroundActivated(Windows::ApplicationModel::Activation::BackgroundActivatedEventArgs^ e) override;
+    virtual void OnBackgroundActivated(Windows::ApplicationModel::Activation::BackgroundActivatedEventArgs ^ e) override;
 #endif
+    void OnFileActivated(Windows::ApplicationModel::Activation::FileActivatedEventArgs ^ args) override;


nit: clang format doesn't like c++/cx #Resolved

oops. I fixed it with the .cpp but not .h

In reply to: 82850185 [](ancestors = 82850185)

aballway · 2016-10-11T17:54:30Z

 }

+void App::OnFileActivated(FileActivatedEventArgs^ args)
+{


you need to call main here to go through UIApplicationMain #Resolved

Add FileActivation Handler Description: This changes adds the ability to activate a WinObjC app via a file (typically registered for in the appx manifest. See https://msdn.microsoft.com/en-us/windows/uwp/launch-resume/handle-file-activation for more details on this process). This change wires up the activation to the UIApplicationDelegate. This approach was taken as opposed to the more conventional process described at https://developer.apple.com/library/content/documentation/FileManagement/Conceptual/DocumentInteraction_TopicsForIOS/Articles/OpeningSupportedFileTypes.html#//apple_ref/doc/uid/TP40010412-SW1 because file activation can happen for multiple files, for a particular verb (open, upload, edit, etc) and can happen while the app is already in the foreground. These differences make the experience different enough that co-opting the other handler seemed inappropriate and would lose fidelity. How verified: new tests Reviewed by: aballway, mnithish

bbowman added 4 commits October 6, 2016 10:44

Add FileActivation Handler

8226faf

s

7413812

s

05e8584

s

9d42115

msftclas added the cla-not-required label Oct 6, 2016

bbowman assigned aballway, ms-jihua and mnithish Oct 6, 2016

aballway approved these changes Oct 6, 2016

View reviewed changes

bbowman commented Oct 6, 2016

View reviewed changes

jaredhms reviewed Oct 6, 2016

View reviewed changes

ms-jihua reviewed Oct 7, 2016

View reviewed changes

mnithish reviewed Oct 7, 2016

View reviewed changes

bbowman added 2 commits October 10, 2016 17:53

s

29fb551

s

1651983

aballway suggested changes Oct 11, 2016

View reviewed changes

bbowman added 3 commits October 11, 2016 13:08

s

1fc4781

Merge branch 'develop' into bbowman/FileActivation

2437d50

Merge branch 'develop' into bbowman/FileActivation

a5c2655

bbowman merged commit d22f496 into microsoft:develop Oct 11, 2016

bbowman deleted the bbowman/FileActivation branch October 11, 2016 21:14

rajsesh mentioned this pull request Oct 20, 2016

Add a delegate to handle OnFileActivated from islandwood app #1069

Closed

Conversation

bbowman commented Oct 6, 2016

Uh oh!

aballway Oct 6, 2016 • edited by bbowman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aballway Oct 6, 2016 • edited by bbowman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aballway Oct 6, 2016 • edited by bbowman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aballway Oct 6, 2016 • edited by bbowman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aballway Oct 6, 2016 • edited by bbowman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DHowett-MSFT Oct 6, 2016 • edited by bbowman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aballway Oct 6, 2016 • edited by bbowman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aballway Oct 6, 2016 • edited by bbowman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aballway Oct 6, 2016 • edited by bbowman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aballway Oct 6, 2016 • edited by bbowman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aballway Oct 6, 2016 • edited by bbowman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredhms Oct 6, 2016 • edited by bbowman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mnithish Oct 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajsesh commented Oct 6, 2016

aballway Oct 6, 2016 •

edited by bbowman

Loading

aballway Oct 6, 2016 •

edited by bbowman

Loading

aballway Oct 6, 2016 •

edited by bbowman

Loading

aballway Oct 6, 2016 •

edited by bbowman

Loading

aballway Oct 6, 2016 •

edited by bbowman

Loading

DHowett-MSFT Oct 6, 2016 •

edited by bbowman

Loading

aballway Oct 6, 2016 •

edited by bbowman

Loading

aballway Oct 6, 2016 •

edited by bbowman

Loading

aballway Oct 6, 2016 •

edited by bbowman

Loading

aballway Oct 6, 2016 •

edited by bbowman

Loading

aballway Oct 6, 2016 •

edited by bbowman

Loading

jaredhms Oct 6, 2016 •

edited by bbowman

Loading

mnithish Oct 7, 2016 •

edited

Loading

ms-jihua Oct 7, 2016 •

edited by bbowman

Loading

mnithish Oct 7, 2016 •

edited by bbowman

Loading

mnithish commented Oct 7, 2016 •

edited

Loading

aballway Oct 11, 2016 •

edited by bbowman

Loading

aballway Oct 11, 2016 •

edited by bbowman

Loading