deep linking first pass #171

eoji · 2017-10-24T18:42:16Z

what

First pass of deep linking on Android. This adds deep links to the discovery page and project pages.

views

ktman · 2017-10-24T20:39:52Z

hey @eoji what discovery queries are included here?

eoji · 2017-10-24T21:06:19Z

@ktman Deep linking to the Discovery screen was already present.

In this PR, if the user clicks a link to https://www.kickstarter.com/projects, they're taken to Discovery

ktman · 2017-10-24T22:09:57Z

ohhh interesting. mind testing what happens to queries like https://www.kickstarter.com/discover/advanced?woe_id=23424796&sort=magic&seed=2514803&page=2

want to make sure we're bouncing the queries we don't support back to web.

swoopej

coooooool 😎 couple of small questions but looks good!

swoopej · 2017-10-25T14:52:06Z

app/src/main/java/com/kickstarter/ui/activities/DeepLinkActivity.java

+  }
+
+  private void inputPackageManager(final String url) {
+    if (!TextUtils.isEmpty(url)) {


could we add a .filter(url -> !TextUtils.isEmpty(url)) on the output here so that we won't get empty values emitted? https://github.com/kickstarter/android-oss/pull/171/files#diff-2f428d6bea56ac66d4ec1a8567629475R105

unless I'm missing something and we actually want those

swoopej · 2017-10-25T14:57:41Z

app/src/main/java/com/kickstarter/viewmodels/DeepLinkViewModel.java

+
+      uriFromIntent
+        .filter(uri -> KSUri.isProjectUri(uri, uri.toString()))
+        .map(Uri::toString)


could we just keep this output as an Observable<Uri> since we're converting back to a Uri in the view holder?

luoser

some lil comments! awesome woooorrrkkkkk

luoser · 2017-10-25T18:33:07Z

app/src/main/java/com/kickstarter/ui/activities/DeepLinkActivity.java

+@RequiresActivityViewModel(DeepLinkViewModel.ViewModel.class)
+public class DeepLinkActivity extends BaseActivity<DeepLinkViewModel.ViewModel> {
+  @Override
+  protected void onCreate(@Nullable final Bundle savedInstanceState) {


we like to put our annotations right next to what it's annotating, e.g. final @Nullable Bundle savedInstanceState

luoser · 2017-10-25T18:33:17Z

app/src/main/java/com/kickstarter/ui/activities/DeepLinkActivity.java

+import static com.kickstarter.libs.rx.transformers.Transformers.observeForUI;
+
+@RequiresActivityViewModel(DeepLinkViewModel.ViewModel.class)
+public class DeepLinkActivity extends BaseActivity<DeepLinkViewModel.ViewModel> {


class can be final

luoser · 2017-10-25T18:33:33Z

app/src/main/java/com/kickstarter/ui/activities/DeepLinkActivity.java

+    finish();
+  }
+
+  private void startProjectActivity(final String url) {


@NonNull, L65 & 74 as well

luoser · 2017-10-25T18:37:36Z

app/src/main/java/com/kickstarter/ui/activities/DeepLinkActivity.java

+  }
+
+  private void startProjectActivity(final String url) {
+    final Uri uri = Uri.parse(url);


alternatively, we could output the Uri rather than Url since we only need the Uri to start the activity

luoser · 2017-10-25T18:38:24Z

app/src/main/java/com/kickstarter/ui/activities/DeepLinkActivity.java

+  }
+
+  private void startBrowser(final List<Intent> targetIntents) {
+    final Intent chooserIntent = Intent.createChooser(targetIntents.remove(0), "");


maybe we can add a lil comment to explain why we need to remove the first targetIntent ?

luoser · 2017-10-25T18:45:43Z

app/src/main/java/com/kickstarter/viewmodels/DeepLinkViewModel.java

+              intent.setData(pair.second);
+              return intent;
+            })
+            .toList();


luoser · 2017-10-25T18:46:09Z

app/src/main/java/com/kickstarter/viewmodels/DeepLinkViewModel.java

+        .map(Uri::toString)
+        .compose(bindToLifecycle())
+        .subscribe(this.requestPackageManager::onNext);
+


can nix this new line and add one btwn L109-110

luoser · 2017-10-25T18:46:46Z

app/src/main/java/com/kickstarter/viewmodels/DeepLinkViewModel.java

+    }
+
+    @Override
+    public Observable<String> requestPackageManager() {


these Observable functions can all be annotated with a @NonNull return type

luoser · 2017-10-25T18:47:45Z

app/src/main/java/com/kickstarter/libs/Koala.java

@@ -493,4 +493,11 @@ public void trackOpenedExternalLink(final @NonNull Project project, final @NonNu

    this.client.track(KoalaEvent.OPENED_EXTERNAL_LINK, props);
  }
+
+  // DEEP LINK
+  public void trackUserActivity() {


we tend to name these Koala event functions exactly the event they're tracking; since UserActivity is kind of vague, maybe we can be Xtra and name this trackContinueUserActivityAndOpenedDeepLink or something of the sorts!

luoser · 2017-10-25T18:48:33Z

app/src/test/java/com/kickstarter/viewmodels/DeepLinkViewModelTest.java

+    return new Intent()
+      .setData(Uri.parse(url));
+  }
+}


siqqqqq tests!

swoopej

looks good to meee just a couple non blocking questions

swoopej · 2017-11-01T21:57:06Z

app/src/main/java/com/kickstarter/ui/activities/DeepLinkActivity.java

+  }
+
+  private void startBrowser(final @NonNull List<Intent> targetIntents) {
+    if (!targetIntents.isEmpty()) {


just to confirm we want to call this method even if the targetIntents are an empty list? If we don't need to call this then we could add a filter on the end of the output and have it not emit (but maybe we want finish() to get called here?)

Yeah, we want to finish the DeepLinkActivity!

swoopej · 2017-11-01T21:57:44Z

app/src/main/java/com/kickstarter/ui/activities/DeepLinkActivity.java

+      when we add the EXTRA_INITIAL_INTENTS intents. */
+      final Intent chooserIntent = Intent.createChooser(targetIntents.remove(0), "");
+      chooserIntent.putExtra(Intent.EXTRA_INITIAL_INTENTS,
+        targetIntents.toArray(new Parcelable[targetIntents.size()]));


could any of this logic get pushed into the ViewModel? not sure if it relies on any context from the activity...

I think only startActivity, I really just need the chooserIntent.

luoser

lil nits im sry but logic and important things LGTM!

luoser · 2017-11-06T20:14:46Z

app/src/main/java/com/kickstarter/ui/activities/DeepLinkActivity.java

+import static com.kickstarter.libs.rx.transformers.Transformers.observeForUI;
+
+@RequiresActivityViewModel(DeepLinkViewModel.ViewModel.class)
+final public class DeepLinkActivity extends BaseActivity<DeepLinkViewModel.ViewModel> {


public final

luoser · 2017-11-06T20:25:32Z

app/src/main/java/com/kickstarter/ui/activities/DeepLinkActivity.java

+  protected void onCreate(final @Nullable Bundle savedInstanceState) {
+    super.onCreate(savedInstanceState);
+
+    this.viewModel.outputs.startDiscoveryActivity()


can also alphabetize these outputs

…-linking # Conflicts: # app/src/main/java/com/kickstarter/libs/KoalaEvent.java

eoji · 2017-11-17T22:16:35Z

@swoopej @luoser could i please have your 👀 on this again since it's been a while

swoopej

amazingggg 👌

eoji added 10 commits October 13, 2017 15:07

First pass of deep linking on android! Still needs tests.

6adf367

Added tracking and tests for DeepLinkViewModel

ce8fdcd

start of refactoring deep link browser stuff

bd5b468

Updated tests but I don't like them

95e00a7

Updated tests and clean up.

4a519c9

Merge remote-tracking branch 'oss/master' into deep-linking

af1143a

Merge remote-tracking branch 'oss/master' into deep-linking

5ce9242

Commented out email links in android manifest

4e94d69

Removing intent filter for emails and logic from view model.

4e34be8

Clean up, clean up

a328e8f

eoji requested review from swoopej and luoser October 24, 2017 18:42

swoopej reviewed Oct 25, 2017

View reviewed changes

luoser reviewed Oct 25, 2017

View reviewed changes

eoji added 4 commits October 31, 2017 18:09

PR feedback

fa8df14

got got by checkstyle

62bd882

Fixing broken test 👀

e8b92f1

renaming test

63ba649

swoopej approved these changes Nov 1, 2017

View reviewed changes

luoser approved these changes Nov 6, 2017

View reviewed changes

eoji added 2 commits November 17, 2017 16:22

Lisa PR feedback

638038e

Merge branch 'master' of github.com:kickstarter/android-oss into deep…

ec837cf

…-linking # Conflicts: # app/src/main/java/com/kickstarter/libs/KoalaEvent.java

swoopej approved these changes Nov 20, 2017

View reviewed changes

eoji merged commit 6d6eb99 into master Nov 20, 2017

eoji deleted the deep-linking branch November 20, 2017 16:01

Rcureton pushed a commit that referenced this pull request Jul 31, 2018

deep linking first pass (#171)

dba26ca

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deep linking first pass #171

deep linking first pass #171

eoji commented Oct 24, 2017

ktman commented Oct 24, 2017

eoji commented Oct 24, 2017

ktman commented Oct 24, 2017

swoopej left a comment

swoopej Oct 25, 2017

swoopej Oct 25, 2017

luoser left a comment

luoser Oct 25, 2017

luoser Oct 25, 2017

luoser Oct 25, 2017

luoser Oct 25, 2017

luoser Oct 25, 2017

luoser Oct 25, 2017

luoser Oct 25, 2017

luoser Oct 25, 2017

luoser Oct 25, 2017

luoser Oct 25, 2017

swoopej left a comment

swoopej Nov 1, 2017 •

edited

Loading

eoji Nov 1, 2017

swoopej Nov 1, 2017

swoopej Nov 1, 2017

eoji Nov 1, 2017

luoser left a comment

luoser Nov 6, 2017

luoser Nov 6, 2017

eoji commented Nov 17, 2017

swoopej left a comment

deep linking first pass #171

deep linking first pass #171

Conversation

eoji commented Oct 24, 2017

what

views

ktman commented Oct 24, 2017

eoji commented Oct 24, 2017

ktman commented Oct 24, 2017

swoopej left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luoser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swoopej left a comment

Choose a reason for hiding this comment

swoopej Nov 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luoser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eoji commented Nov 17, 2017

swoopej left a comment

Choose a reason for hiding this comment

swoopej Nov 1, 2017 •

edited

Loading