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

fix(ios): check if app is booted before sending notifications #10565

Merged
merged 4 commits into from Jan 7, 2019

Conversation

hansemannn
Copy link
Collaborator

@build build added this to the 8.0.0 milestone Jan 2, 2019
@build build requested a review from a team January 2, 2019 19:25
@build
Copy link
Contributor

build commented Jan 2, 2019

Messages
📖

💾 Here's the generated SDK zipfile.

📖 🎉 Another contribution from our awesome community member, hansemannn! Thanks again for helping us make Titanium SDK better. 👍
📖

✅ All tests are passing
Nice one! All 2987 tests are passing.

Generated by 🚫 dangerJS against 0a4386a

@jquick-axway
Copy link
Contributor

@hansemannn, in another PR, I'm looking into adding a new Ti.App event "started" which gets fired after the "app.js" gets executed. This intends to solve the same problem. Please see...
https://github.com/appcelerator/titanium_mobile/pull/10519/files#diff-722bd9f159d5a9feed5efb028c22ab04

I think your code change is fine. I just wanted to let you know that I plan on transitioning the code to use the above solution in the future. It's especially needed when we start adding bootstrap scripts which will display UI before the "app.js" gets executed.

What do you think?

@hansemannn
Copy link
Collaborator Author

Hey @jquick-axway! In general, I am fine renaming it. But - especially for iOS - the handleurl method is also used for URL-handling outside the deep-linking scope, e.g. for handling links from a web-view manually. So I'd not deprecate this but see the started event as an addition. Although the functionality may be duplicate on iOS unless it does something else as well. I'll need to go through your proposed changed to check that.

@jquick-axway
Copy link
Contributor

@hansemannn, I think we might be talking about 2 different things. Sorry about the confusion.

The Ti.App event "started" is a new feature that I'm adding. I only added it because the system needs some kind of clue that the "app.js" has been executed and has had a chance to add its event listeners... because we don't want to fire events until the "app.js" has had a chance to add listeners for them. I added the "started" event to my intent PR because my code change also needed to defer showing a "JSActivity" until after the root activity has been displayed. This is an Android only issue I'm solving and the new "started" event provides a guaranteed means for doing this.

Now here's a potential issue that can happen on iOS in the future. If we did set up a bootstrap script to show UI on startup (let's say to show a dialog), the "app.js" won't be loaded until after that bootstrap UI has been cleared. This means that a "handleurl" event fired while a bootstrap UI is displayed won't be received by the "app.js" listeners since the "app.js" hasn't been loaded yet. That's the potential future issue. For "handleurl", perhaps this behavior is okay, but this behavior might not be okay for notification handling. That's the potential issue that the "started" event solves.

@vijaysingh-axway
Copy link
Contributor

@hansemannn Changes looks good to me. Can you please give a test case for QE to verify. Please fix linking issue. Thanks!

@hansemannn
Copy link
Collaborator Author

hansemannn commented Jan 4, 2019

You mean linting? Can you post the command to lint again? clang-format -style=file -i <file> seems to not pickup the .clang-format we've put into /iphone/ back then.

Test case:

  1. Create a simple app.js with the following content:
// Before, this event only fired if the app was opened before. On a cold
// start, it was ignored.
Ti.App.iOS.addEventListener('handleurl', function (event) {
    Ti.API.warn(event);
});

var win = Ti.UI.createWindow({ backgroundColor: '#fff' });
win.open();
  1. Add the following to your plist section:
<key>CFBundleURLTypes</key>
<array>
    <dict>
        <key>CFBundleURLName</key>
        <string>com.yourdomain.yourappprefix</string>
        <key>CFBundleURLSchemes</key>
        <array>
            <string>myapp</string>                           
        </array>
    </dict>
</array>
  1. Trigger the URL scheme by typing myapp://test123 in Safari.

@vijaysingh-axway
Copy link
Contributor

vijaysingh-axway commented Jan 7, 2019

@hansemannn Yes. Linting :).
I believe you are running command -
clang-format -i -style=file TitaniumKit/TitaniumKit/Sources/API/TiApp.m at location '/iphone' in this case.
I have run the same command and its working. I have fixed this linting issue. Thanks!

Copy link
Contributor

@vijaysingh-axway vijaysingh-axway left a comment

Choose a reason for hiding this comment

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

CR passed.

Copy link
Contributor

@ssjsamir ssjsamir left a comment

Choose a reason for hiding this comment

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

FR Passed Using a variation of test case provided above.

var label1 = Ti.UI.createLabel({
  color: '#900',
  font: { fontSize:48 },
  shadowColor: '#aaa',
  shadowOffset: {x:5, y:5},
  shadowRadius: 3,
  text: 'A simple label',
  textAlign: Ti.UI.TEXT_ALIGNMENT_CENTER,
  top: 30,
  width: Ti.UI.SIZE, height: Ti.UI.SIZE
});

Ti.App.iOS.addEventListener('handleurl', function (event) {
    Ti.API.warn(event);
    win.add(label1);
});

var win = Ti.UI.createWindow({ backgroundColor: '#fff' });
win.open();

Test Environment

iPhone 6 Sim (12.1)
APPC CLI: 7.0.9
Operating System Name: Mac OS Mojave
Operating System Version: 10.14.2
Node.js Version: 8.9.1
Xcode 10.1

@ssjsamir ssjsamir merged commit 53e39f5 into tidev:master Jan 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants