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

Fixes intent handling and adds sample app, test for it. #238

Merged
merged 1 commit into from
Apr 14, 2017
Merged

Fixes intent handling and adds sample app, test for it. #238

merged 1 commit into from
Apr 14, 2017

Conversation

rjrjr
Copy link
Collaborator

@rjrjr rjrjr commented Apr 13, 2017

@rjrjr rjrjr requested a review from loganj April 13, 2017 22:36
@rjrjr
Copy link
Collaborator Author

rjrjr commented Apr 13, 2017

Test is ugly in one respect: it creates a new instance of the activity. Worth fixing? I suppose I could make the activity single instance, but that's not really required.

@rjrjr
Copy link
Collaborator Author

rjrjr commented Apr 13, 2017

Ideally we'd test both getting an intent to an existing activity and creating a new activity with an intent, since those code paths are so damn different. Maybe I should make two activities with different manifests.

rjrjr
rjrjr commented Apr 13, 2017
View reviewed changes
flow-sample-intents/src/main/res/layout/hello_screen.xml Outdated
~ limitations under the License.
-->

<flow.sample.intents.HelloView
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops, copy pasta

@rjrjr rjrjr force-pushed the ray/is-it-safe-211 branch 2 times, most recently from 69841c4 to 9efc93e Compare April 13, 2017 22:43
rjrjr
rjrjr commented Apr 13, 2017
View reviewed changes
flow-sample-intents/src/main/AndroidManifest.xml Outdated
@@ -0,0 +1,36 @@
<?xml version="1.0" encoding="utf-8"?>
<!--
~ Copyright 2016 Square Inc.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

2017

rjrjr
rjrjr commented Apr 13, 2017
View reviewed changes
flow-sample-intents/src/main/java/flow/sample/intents/StringParceler.java
@@ -0,0 +1,19 @@
package flow.sample.intents;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Missing copyright

rjrjr
rjrjr commented Apr 13, 2017
View reviewed changes
flow-sample-intents/src/androidTest/java/flow/sample/basic/IntentsSampleTest.java
@@ -0,0 +1,42 @@
package flow.sample.basic;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@loganj None of the test classes or gradle files have copyright blocks. Is that intentional?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah.

@rjrjr
Copy link
Collaborator Author

rjrjr commented Apr 13, 2017

Cool, if I make the activity singleInstance your built in intent handling breaks. Investigating.

@rjrjr
Copy link
Collaborator Author

rjrjr commented Apr 13, 2017

Fixed, moar tests.

@loganj
Copy link
Collaborator

loganj commented Apr 14, 2017

LG!

@rjrjr rjrjr merged commit c4730dd into master Apr 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@loganj loganj Awaiting requested review from loganj

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@rjrjr @loganj