Compare result fix

[novachevskyi]

In the lib description it was stated that module registration affects the conflict resolution order: If they are not defined in the same module the one defined in the registry listed first in DeeplinkDelegate will be matched.. In fact that’s not true. When conflict and duplicate was found here then sorting is performed based on comparable here. The logic is broken because it is described that

// Found multiple matches. Sort matches by concreteness: // No variable element > containing placeholders > are a configurable path segment But in fact we have the list sorted in the opposite way. It looks like the logic here sets the wrong value and -1 should be flipped to 1 to work as expected.

[rossbacher]

You are correct that this is If they are not defined in the same module the one defined in the registry listed first in DeeplinkDelegate will be matched.. not really true. Because we sort the duplicates based on concreteness we cannot make any assumption about which one gets picked. So the documentation should be updated to If they are not defined in the same module it is not defined which one will be returned, thus the built in support for finding duplicates should be used to ensure there are no duplicates or something else.

However the code change is wrong, maybe the comment here is also misleading, what is meant is that is should sort the elements into an order give by the example e.g.

            // Found multiple matches. Sort matches by concreteness:
            // listOf(No variable element ... containing placeholders ...  are a configurable path segment)

The code is expecting the more concrete element at the front of the list, we do have a test for that testMoreConcreteMach which fails in your PR, plus some other tests.

You should run all tests locally before submitting a PR.

[novachevskyi]

@rossbacher Thanks for the reply! I'll be thankful if you can elaborate on the topic more.

Indeed, I see that testMoreConcreteMach fails because MainActivity handles the deep link instead of the LibraryActivity. The main activity defines:

@DeepLink("placeholder://host/somePathOne/{param1}/somePathThree")
  public static Intent lessConcreteMatch(Context context, Bundle bundle) {

And the LibraryActivity has more concrete match:

 @DeepLink("placeholder://host/somePathOne/somePathTwo/somePathThree")
  public static Intent moreConcreteMatch(Context context, Bundle bundle) {

For this case the logic works as expected.

However, what if we have the following case: MainActivity:

@DeepLink("placeholder://host/{param1}/{param2}/{param3}")
  public static Intent lessConcreteMatch(Context context, Bundle bundle) {

LibraryActivity:

 @DeepLink("placeholder://host/somePathOne/somePathTwo/{param1}")
  public static Intent moreConcreteMatch(Context context, Bundle bundle) {

In this case the sorting won't work as you expect it to be because it will find that the first deeplink contains dynamic segment placed earlier than the second one. But in my case I'm assuming that the second deeplink is more concrete than the first one.

lessConcreteMatch now doesn't fail with my example.

I assume I used the wrong solution by focusing only on my problematic case. What would be the best way to fix the issue? Or do you think that issue don't exist and I'm misusing the library in some way?

[novachevskyi]

This solution works for both test cases, but I'm still not sure if it doesn't broke anything else.

override fun compareTo(other: DeepLinkMatchResult): Int {
    return when {
        this.firstNonConcreteIndex < 0 &&
                this.firstNonConcreteIndex != other.firstNonConcreteIndex -> -1
        other.firstNonConcreteIndex < 0 &&
                other.firstNonConcreteIndex != this.firstNonConcreteIndex -> 1
        this.firstNonConcreteIndex < other.firstNonConcreteIndex -> 1
        this.firstNonConcreteIndex == other.firstNonConcreteIndex -> {
            if (this.firstNonConcreteIndex == -1 || deeplinkEntry.uriTemplate[firstNonConcreteIndex] == other.deeplinkEntry.uriTemplate[firstNonConcreteIndex]) {
                0
            } else if (deeplinkEntry.uriTemplate[firstNonConcreteIndex] == configurablePathSegmentPrefixChar) {
                -1
            } else 1
        }
        else -> -1
    }
}

[novachevskyi]

This one also works fine with my latest suggestion:

@DeepLink("placeholder://host/somePathOne/{param1}/somePathThree")
  public static Intent moreConcreteMatch(Context context, Bundle bundle) {

@DeepLink("placeholder://host/{param1}/somePathTwo/somePathThree")
  public static Intent lessConcreteMatch(Context context, Bundle bundle) {

[novachevskyi]

So with my recent update MainActivityTest in different variations doesn't fail, while it fails for your master with: MainActivity:

@DeepLink("placeholder://host/{param1}/{param2}/{param3}")
  public static Intent lessConcreteMatch(Context context, Bundle bundle) {

LibraryActivity:

 @DeepLink("placeholder://host/somePathOne/somePathTwo/{param1}")
  public static Intent moreConcreteMatch(Context context, Bundle bundle) {

Other tests are used to check comparison values, so they should be changed respectively to the recent changes I beilieve.

Please let me know if it does make a sense.

[rossbacher]

@novachevskyi you are correct this is a bug as having no placeholder or configurable path segment is -1 and thus always smaller than having a low one.

I looked into fixing this with using Int.MAX_VALUE as this makes the compare code cleaner but i think your solution is more elegant, please add a comment explaining new lines in the compareTo code so that is is clear for what they are. After that i will accept and merge this, thank you for fixing!

[rossbacher]

Can you also male sure that CI passes? Just run ./gradlew check on your local branch and ensure that it passes.

[novachevskyi]

@rossbacher I messed up with some changes and forks initially, but now check is running well locally. Please re-verify and feel free to suggest any possible changes if required.

Also, do you have any plans about updating the release version with current fix?

[rossbacher]

@rossbacher I messed up with some changes and forks initially, but now check is running well locally. Please re-verify and feel free to suggest any possible changes if required.

Also, do you have any plans about updating the release version with current fix?

I'll merge your change today if it passes CI tests, this will trigger publishing of a snapshot

For a release we still need a few other PRs merged and I'm not 100% sure about a timeline for that, but if you want to use the fix you can just use the snapshot.

[rossbacher]

Thank you for the fix!

[novachevskyi]

Thanks 👍