airbnb / DeepLinkDispatch

A simple, annotation-based library for making deep link handling better on Android
http://nerds.airbnb.com/deeplinkdispatch/
4.38k stars 405 forks source link

Auto AndroidManifest entry #31

Open tasomaniac opened 9 years ago

tasomaniac commented 9 years ago

Why do we have to enter the Manifest Activity manually? Can't you just make it happen within the library thanks to Manifest merger.

felipecsl commented 9 years ago

As far as I know, it is not possible to dynamically generate an android manifest file to be picked up by the manifest merger. This is something we've been discussing ideas on how to solve but I can't think of a way to do it automatically right now. If you have any pointers or know how, please feel free to submit a pull request or provide more infos

MichaelEvans commented 9 years ago

@tasomaniac It's not possible to do for one reason - there's no way to dynamically generate the intent-filter with the data attribute you need. (Otherwise, yes, this would be possible). You can use an activity tag with node:tools="replace" in your manifest to add the data attribute yourself, but at that point, are you really saving any headache?

lordcodes commented 9 years ago

From the @DeepLink / @DeepLinks annotation you know the scheme and the URL. You could use that to generate an intent filter data attribute with

android:scheme="{scheme from URL in annotation}" android:host="{host from URL in annotation}" android:pathPrefix="{rest of URL in annotation}"

You can generate one of these for each deep link annotation.

I.e.

@DeepLink("https://www.company.com/shop")
public class ShopActivity extends Activity { ... }

@DeepLink("https://www.company/com/basket")
public class BasketActivity extends Activity { ... }

Would generate

 <intent-filter>
    <action android:name="android.intent.action.VIEW" />

        <category android:name="android.intent.category.DEFAULT" />
        <category android:name="android.intent.category.BROWSABLE" />

        <data
                    android:scheme="https"
                    android:host="www.company.com"
                    android:pathPrefix="shop" />

        <data
                    android:scheme="https"
                    android:host="www.company.com"
                    android:pathPrefix="basket" />

    </intent-filter>

You could simplify the generation of manifest by including an AndroidManifest that is ready (with no data tags), then something to indicate where to insert all of the data tags. Then a gradle task that runs when the app is built and inserts all of the data tags.

romainpiel commented 8 years ago

I think it is possible to achieve with a manifestPlaceholder:

        <activity
            android:name="com.airbnb.deeplinkdispatch.DeepLinkActivity"
            android:theme="@android:style/Theme.NoDisplay">
            <intent-filter>
                <action android:name="android.intent.action.VIEW" />
                <category android:name="android.intent.category.DEFAULT" />
                <category android:name="android.intent.category.BROWSABLE" />
                ${manifestDataPlaceholder}
            </intent-filter>
        </activity>

And generate manifestDataPlaceholder from the gradle plugin on the fly.

See https://github.com/castorflex/manifestreplace-plugin for a similar approach.

felipecsl commented 8 years ago

@RomainPiel but can the annotation processor trigger the gradle plugin when apt is running in order to insert the correct intent filter?

romainpiel commented 8 years ago

Not sure about that, I never played with apt extensively. Worth a try you think?

romainpiel commented 8 years ago

Maybe @hvisser can help on that? Hey Hugo, do you think there's a way to modify the gradle defaultConfig when apt is running? I'm also wondering if apt is running before the manifest merger. (if not any modification to the config wouldn't do anything)

hvisser commented 8 years ago

You can pass in arguments to the annotation processor, based on the variant which would also hold the applicationId if that is what you are asking? Not sure what you are asking I guess. You can't update any plugin configuration after it is configured.

hvisser commented 8 years ago

One way would maybe add string resources, reference those in the manifest and have the project override that. But it's not that nice either and I'm not sure if that would work for fields like the url scheme.

romainpiel commented 8 years ago

Alright, let's summarise (@felipecsl correct me if I'm wrong):

So I'm not sure how to "reach" the gradle plugin from the annotation processor. The string res thing is actually not silly. It would be a way to link them, not optimally clean though :/ I'm starting to think that the main blocker for this issue is that urls are spread out in the app. I wish the definition of the routes was a bit more centralised like that:

intentFilter {
    data(url: 'airbnb://', activity:'com.airbnb.MainActivity')
    data(url: 'airbnb://blah', activity:'com.airbnb.BlahActivity')
    data(url: 'airbnb://blah/{id}', activity:'com.airbnb.BlahActivity')
}

That way you would have a plugin launching apt and then generating path in the manifest. But well obviously, it's not in the scope of this ticket. Just my 2cts :)

hvisser commented 8 years ago

You can't call back up to Gradle from the processor, the processor doesn't know about that at all, it's called by javac or by the jack compiler on Android. What you could do it generate some file in the processor and write a Gradle plugin to do something with that, after compiling the sources. It sounds like a lot of hassle for the problem it's solving imho.

felipecsl commented 8 years ago

yeah doesn't sounds like it's worth doing...

romainpiel commented 8 years ago

Hmmm yeah, discussable. At the moment, if a url is not handled, the deep link will never be launching the browser. Only way is to fallback to a web view but it doesn't fit the android way of doing things IMHO. But I agree, the quantity of work to achieve might not be worth the effort. On 10 Nov 2015 9:18 p.m., "Felipe Lima" notifications@github.com wrote:

yeah doesn't sounds like it's worth doing...

— Reply to this email directly or view it on GitHub https://github.com/airbnb/DeepLinkDispatch/issues/31#issuecomment-155569833 .

felipecsl commented 8 years ago

hmm I suppose you're talking about http(s) deep links specifically? In this case maybe there's a way to reroute it and startActivity with an explicit intent to launch the browser? Maybe if you specify the Chrome package name? Of course it wouldn't work if you don't have Chrome installed but could be a simple fallback

romainpiel commented 8 years ago

Ah yeah sorry I should have been more specific, I'm talking about http links. Defaulting to Chrome sounds a bit hacky to me. I'm going to manually add the paths in the manifest for now and I'll try to make an alternative solution when I have some time. Thanks for your help anyway. I think we've been around the problem now, feel free to close the issue :+1:

felipecsl commented 8 years ago

I'll keep it open in case someone has a better idea or a new solution comes up :)