ReVanced / revanced-manager

💊 Application to use ReVanced on Android
https://revanced.app
GNU General Public License v3.0
17.94k stars 738 forks source link

feat: Downloader API proposal #2064

Open oSumAtrIX opened 3 months ago

oSumAtrIX commented 3 months ago

Feature description

Downloader API proposal

A downloader must specify a download function and a strategy. The downloader provides a DSL in which a strategy can be invoked. In this case we want two strategies. An api() and webview() strategy.

API strategy

The API strategy is to use an API just like the name suggests. The strategy has a find function to find a download link from the requested app and version. The function returns a final download link.

The download function is called once the patching process begins. The download link earlier returned by find is used here to download the requested app:

val downloader = downloader {
    api {
        find { app, version -> "https://example.com/$app/$version" }
    }

    download { link-> URL(link).inputStream() }
}

WebView strategy

The WebView strategy is helpful because some sites may be gated behind anti-scraping or captcha systems, or some sites would prefer not to be scraped and instead want to generate revenue

The solution is to find the download link by displaying the user a webview. The user navigates the site until the download button is pressed for example. The download function hooks requests, and once it sees the response for the download request, it can capture the download URL:

val downloader = downloader {
    webView {
        show { app, version -> "https://example.com/$app/$version" }
                capture { request,  response -> response.takeIf { it.url == "https://cdn.com/app.apk" }?.url }
    }

    download { url -> url.inputStream() }
}

As seen, the show function receives the requested app and version and, in turn, builds a link as a starting point for displaying the webview. The user navigates the page, triggering requests. All requests/ responses are hooked by the function capture. Once capture returns a string (the download link), Manager ends the web view and saves the download link. The download earlier returned by capture is used here to download the requested app.

The download function

The argument of download should be generic and specified by the strategies. For example this would bind the generic type to URL:

val downloader = downloader {
    webView<URL> {
        show { app, version -> "https://example.com/$app/$version" }
            capture { request,  response -> response.takeIf { it.url == "https://cdn.com/app.apk" }?.url }
    }

     download { url -> url.inputStream() }
}

This would bind the generic type to String:

val downloader = downloader {
    api<String> {
        find { app, version -> "https://example.com/$app/$version" }
    }

    download { link-> URL(link).inputStream() }
}

This is useful when the downloader may want to share a custom object to download.

Full example

Click me ```kt suspend fun test() { downloader { api { find { app, version -> "$app/$version" } } download { URI.create(it).toURL().openStream() to null } }.get("app", null).download() downloader { webView { show { app, version -> "$app/$version" } capture { _, response -> response.url to response.header["size"]!!.toInt() } } download { result: Pair -> URL(result.first).openStream() to result.second } }.get("app", "1.0").download() } interface Strategy { suspend fun get(app: String, version: String?): T } class ApiStrategy( val find: suspend (app: String, version: String?) -> T ) : Strategy { override suspend fun get(app: String, version: String?) = find(app, version) } class ApiStrategyBuilder { private var find: (suspend (app: String, version: String?) -> T)? = null fun find(find: suspend (app: String, version: String?) -> T) { this.find = find } fun build(): ApiStrategy { return ApiStrategy(find!!) } } fun DownloaderBuilder.api(block: ApiStrategyBuilder.() -> Unit) = strategy(ApiStrategyBuilder().apply(block).build()) class Request class Response(val url: String, val header: Map) class WebViewStrategy( val show: (app: String, version: String?) -> String, val capture: (request: Request, response: Response) -> T? ) : Strategy { override suspend fun get(app: String, version: String?): T { val uri = show(app, version) // Show a webview here and somehow capture requests/responses to get the result return capture(Request(), Response(uri, mapOf("size" to "1024")))!! } } class WebViewStrategyBuilder { private var show: ((app: String, version: String?) -> String)? = null private var capture: ((request: Request, response: Response) -> T?)? = null fun show(show: (app: String, version: String?) -> String) { this.show = show } fun capture(capture: (request: Request, response: Response) -> T?) { this.capture = capture } fun build(): WebViewStrategy { return WebViewStrategy(show!!, capture!!) } } fun DownloaderBuilder.webView(block: WebViewStrategyBuilder.() -> Unit) = strategy(WebViewStrategyBuilder().apply(block).build()) class Downloader( private val strategy: Strategy, private val download: suspend (T) -> Pair ) { class Downloadable( private val downloader: Downloader, private val result: T ) { suspend fun download() = downloader.download(result) } suspend fun get(app: String, version: String?) = Downloadable(this, strategy.get(app, version)) } class DownloaderBuilder { private var strategy: Strategy? = null private var download: ((T) -> Pair)? = null internal fun strategy(strategy: Strategy) { this.strategy = strategy } fun download(download: (T) -> Pair) { this.download = download } fun build(): Downloader { return Downloader(strategy!!, download!!) } } fun downloader(block: DownloaderBuilder.() -> Unit) = DownloaderBuilder().apply(block).build() ```

Questions

An example API could look like this:

val downloader = downloader {
    val api: String by option("API URL", "The API obtained from 'https://example.com/dashboard'")

    webview<URL> {
        show { app, version -> "https://example.com/$app/$version?api=$api" }
        capture { request,  response -> response.takeIf { it.url == "https://cdn.com/app.apk" }?.url }
    }

    download { url -> url.inputStream() }
}

Acknowledgements

Axelen123 commented 3 months ago

I don't think webviews need to be part of this API. A more flexible alternative would be for manager to provide a way to launch activities, which can contain webviews or other authentication mechanisms:

// API code

interface ActivityLaunchPermit {
    // Launches an activity and returns the resulting intent on completion.
    suspend fun launch(intent: Intent): Intent?
}

// Plugin code
val downloader = downloader {
    get {
        val permit = requestUserInteraction() // Suspends until the user allows interaction and returns an ActivityLaunchPermit

        val result = permit.launch(launchWebViewActivity)
        // The result intent can contains the data that the plugin needs
        ...
    }

    download { ... }
}

Utilities for webviews can be implemented in a separate library if it emerges as a common pattern

Ushie commented 3 months ago

For the webview downloader, when do we show the webview? Once the downloader is selected (before the patching process begins) or once the patching process starts?

I believe the most appropriate place would be when it reaches the Download APK step in the patching process, a popup would be shown explaining to the user what they need to do

We may want to configure downloaders. An API design isn't yet available and must be discussed. For example, some APIs need an API key, and we may want to allow the user to enter an API key.

For the API design, we should be able to re-use patch options for this, code for it exists in both the API form and the frontend form so it should simplify the process

oSumAtrIX commented 3 months ago

@Axelen123

I don't think webviews need to be part of this API.

I believe it would be more readable and expressive if you "installed" a strategy via the DSL. What you described as "get" here is the strategy interface. Here is the explanation:

interface Strategy<T> {
    suspend fun get(app: String, version: String?): T
}

As you can see now, you can do the following, which is on par with what you wanted:

downloader<Pair<String, String>> {
        strategy(
            object : Strategy<Pair<String, String>> {
                override suspend fun get(app: String, version: String?): Pair<String, String> {
                    val permit = requestUserInteraction() // Suspends until the user allows interaction and returns an ActivityLaunchPermit

                    val result = permit.launch(launchWebViewActivity)
                    // The result intent can contains the data that the plugin needs

                    return result
                }
            }
        )

       download { (url, size) -> URL(url).openStream() to size.toInt() }
    }

Now this is the part you said we should leave out: This anonymous object is wrapped into a nice API:

class WebViewStrategy<T>(
    val show: suspend (app: String, version: String?) -> String,
    val capture: suspend (request: Request, response: Response) -> T?
) : DownloaderStrategy<T> {
    override suspend fun get(app: String, version: String?): T {
     // show & capture logic
    }
}

We add a small DSL API:


class WebViewStrategyBuilder<T> {
    private var show: ((app: String, version: String?) -> String)? = null
    private var capture: ((request: Request, response: Response) -> T?)? = null

    fun show(show: (app: String, version: String?) -> String) {
        this.show = show
    }

    fun capture(capture: (request: Request, response: Response) -> T?) {
        this.capture = capture
    }

    fun build(): WebViewStrategy<T> {
        return WebViewStrategy(show!!, capture!!)
    }
}

fun <T> DownloaderBuilder<T>.webView(block: WebViewStrategyBuilder<T>.() -> Unit) =
    strategy(WebViewStrategyBuilder<T>().apply(block).build())

and now we can use it:

downloader {
        webView {
            show { app, version -> "$app/$version" }
            capture { _, response -> response.url to response.header["size"]!!.toInt() }
        }

        download { result: Pair<String, Int> -> URL(result.first).openStream() to result.second }
}.get("app", "1.0").download()

I have added a full example to OP for reference and testing.

@Ushie

I believe the most appropriate place would be when it reaches the Download APK step in the patching process; a popup would be shown explaining to the user what they need to do

Manager is agnostic to the implementation of the strategy. All it sees is this:

downloader.get("pkg", "version").download()

This is all it has to work with. So now, if you say we call get, which in turn launches the webview strategy right before the patching process, this means we would also call get for the API strategy right before the patching process. That must be kept in mind.

Ushie commented 3 months ago

Manager is agnostic to the implementation of the strategy. All it sees is this:

downloader.get("pkg", "version").download()

This is all it has to work with. So now if you say we call get which in turn launches the webview strategy, right before the patching process, this means, we would also call get for the API strategy right before the patching process. That mus be kept in mind.

Yes, that is fine, it either continues the patching process undisrupted or requests user interaction, temporarily suspending the patch process until the result is obtained

The API should allow the plugin to customize what is displayed in the popup, as different implementations require different popups and currently there is no limit to what the implementations can be, although this is annoying for localization

oSumAtrIX commented 3 months ago

Yes, that is fine, it either continues the patching process undisrupted or requests user interaction, temporarily suspending the patch process until the result is obtained

I don't understand. The get function is intended for Manager so that manager can show a "This downloader couldn't find a suitable download` before the user starts patching. Otherwise it would be annoying to go back do all the steps again.

Ushie commented 3 months ago

Yes, that is fine, it either continues the patching process undisrupted or requests user interaction, temporarily suspending the patch process until the result is obtained

I don't understand. The get function is intended for Manager so that manager can show a "This downloader couldn't find a suitable download` before the user starts patching. Otherwise it would be annoying to go back do all the steps again.

ReVanced Manager can loop over all available plugins until it finds a suitable download, if it fails to find any then it will request the user to supply an APK themselves and give them a shortcut to a Google search, similar to the one in ReVanced Manager v1

oSumAtrIX commented 3 months ago

until it finds a suitable download

This implies making calls to get.

So I assume you are talking about get being called after the user selects a non installed app. In that case, sounds good, edit the OP so we can keep track of updates.

Ushie commented 3 months ago

until it finds a suitable download

This implies making calls to get.

So I assume you are talking about get being called after the user selects a non installed app. In that case, sounds good, edit the OP so we can keep track of updates.

No, I'm talking about calling get in the patch process, the user doesn't have to select what provider to download with, they can have multiple

When they reach the patch process, ReVanced Manager loops over the multiple providers installed and either it finds one that includes the app, then proceeds to do its strategy, or it loops through all of them unsuccessfully and offers the user to select an APK from storage

oSumAtrIX commented 3 months ago

calling get in the patch process

I think its better to check if the downloader can get the download after selecting a non installed app. Once get returns non null, ReVanced Manager progresses to the next screen and in the patch process screen it calls download.

Axelen123 commented 3 months ago

@oSumAtrIX what is the point of the strategy interface? We can still support webView blocks even if we just have get. Also, requestUserInteraction is implemented by manager so there needs to be some sort of receiver scope.

oSumAtrIX commented 3 months ago

The strategy is just an abstraction of the get() api. As you can see the api strategy has a readable and to an api domain specific find function. The webview one has a domain specific show and capture function. If you were to remove those and instead just show get() you lose semantics, and make it slightly less readable. The DSL also reduces duplicate code.

Like explained before your suggested get() is the get() inside the strategy interface, just that the interface allows for proper domain specific abstraction

Axelen123 commented 3 months ago

Strategies seem odd here, Manager shouldn't care about the implementation of your plugin and we shouldn't assume specifics about implementations either. Just using get is simpler and doesn't require you to define anonymous objects if whatever you are trying to do isn't served by the existing strategies.

The strategy is just an abstraction of the get() api. As you can see the api strategy has a readable and to an api domain specific find function.

    api {
        find { app, version -> Something() }
    }

Is longer than:

    get { app, version ->
        Something()
    }

The webview one has a domain specific show and capture function. If you were to remove those and instead just show get() you lose semantics, and make it slightly less readable. The DSL also reduces duplicate code.

Webview helpers can be implemented in a library that extends this plugin API, we don't need it here. It is already possible to implement without requiring special treatment because plugins can launch their own activities through the requestUserInteraction API and have the webview there.

oSumAtrIX commented 3 months ago

Manager shouldn't care about the implementation of your plugin and we shouldn't assume specifics about implementations either

That is correct. Downloader only sees: downloader.get("pkg", "version").download(). As you can see, there are no details on the implementation of the strategy behind get.

Just using get is simpler and doesn't require you to define anonymous objects if whatever you are trying to do isn't served by the existing strategies.

In the case of the API strategy, find is basically get. But in the case of webview, we have show and capture instead of get. If you were to replace that with get, every downloader that needs a webview would have to implement the boilerplate which the webView() function already has.

Webview helpers can be implemented in a library that extends this plugin API

Why not ship it to the library? If you outsource it, the strategy setter must be public and thus visible via the API. Right now its internal which is hidden from the public API.

because plugins can launch their own activities through the requestUserInteraction API

I am not aware of such an API. I only know what I proposed so far. If you intend to implement a requestUserInteraction API you have wrong abstraction in the case of an API downloader. In my proposal this is not possible, because if you use the api function you don't have access to the show function and vice versa.

Axelen123 commented 3 months ago

This is how you would use requestUserInteraction:

// These two interfaces are implemented by Manager.
interface GetScope {
    suspend fun requestUserInteraction(): ActivityLaunchPermit
}

interface ActivityLaunchPermit {
    // Launches an activity using [intent] and returns the result intent.
    suspend fun launch(intent: Intent): Intent?
}

// Usage
val downloader = downloader {
    get { packageName, version ->
        // requestUserInteraction() suspends until the user allows or denies the request.
        val permit = requestUserInteraction()
        val result = permit.launch(Intent().apply {
            // This activity shows the webview and returns the URL.
            `package` = "my.downloader.plugin"
            action = "LAUNCH_WEBVIEW"
            putExtra("PACKAGE_NAME", packageName)
            putExtra("VERSION", version)
        })!!

        val url = result.getStringExtra("URL")
        ..
    }
}

A convenient extension function for the get() implementation and a base class for the Webview activity can be provided in another library. The extension function only needs the public API of GetScope so there is no problem there. A major advantage of having the webview APIs in another library is that plugins which don't use them don't have to bring in the webview dependency, which reduces APK size.

oSumAtrIX commented 3 months ago

Can you explain why we give plugins low-level APIs, such as when launching an activity? To me, it sounds like they will never need that.

A small and concise API where you can only do a limited but controlled and overseeable amount of things sounds simpler and more reasonable to me,.

The webView function is an extension to the downloader builder. It is not a direct member:

fun <T> DownloaderBuilder<T>.webView(block: WebViewStrategyBuilder<T>.() -> Unit) =
    strategy(WebViewStrategyBuilder<T>().apply(block).build())

Same for api:

fun <T> DownloaderBuilder<T>.api(block: ApiStrategyBuilder<T>.() -> Unit) =
    strategy(ApiStrategyBuilder<T>().apply(block).build())

As you can see they merely call strategy which is a direct member of DownloaderBuilder.

Plugins which do not need a webview, simply don't call it. I don't think moving these to another library next to the existing plugin library makes sense. Having some readymade extension functions there such as webView and api directly through the plugin library which can be invoked inside the builder scope is minimal and intuitive. They are not too far fetched to be outsourced to a different module but also not directly related to the downloader API which is why they are extension functions and not direct members.

val downloader = downloader {
    webView {
        show { ... }
        capture { ... }
    }
}

The "strategy" API is not exposed to the user. It is code that is abstracted away but makes sense to keep in the plugin code for architectural reasons.

If you think a low-level API access such as launching intents is necessary, next to webView and api, a strategy for downloaders to get the download link via an Intent can be added, too. And similarly to downloaders that do not need a webview, those that do not need that low level access to launch activities, simply don't call this extension.

val downloader = downloader {
    intent {
        launch { app, version -> Intent(app, version) }
        result { intent -> intent.getStringExtra("url") to intent.getLongExtra("size") }
    }

   download { link, size -> URL(link).inputStream() to size }
}
Axelen123 commented 3 months ago

Can you explain why we give plugins low-level APIs, such as when launching an activity? To me, it sounds like they will never need that.

Activities and intents are pretty common on Android. All plugins that require interaction need to use them for UI.

A small and concise API where you can only do a limited but controlled and overseeable amount of things sounds simpler and more reasonable to me,.

The suggested get API is small and you don't need it if you are using webView.

Plugins which do not need a webview, simply don't call it. I don't think moving these to another library next to the existing plugin library makes sense. Having some readymade extension functions there such as webView and api directly through the plugin library which can be invoked inside the builder scope is minimal and intuitive. They are not too far fetched to be outsourced to a different module but also not directly related to the downloader API which is why they are extension functions and not direct members.

val downloader = downloader {
  webView {
      show { ... }
        capture { ... }
  }
}

I am fine with webView being an extension function, but the implementation of it needs to be in a non-compileOnly library because the activity needs to be present inside the plugin APK. Just to clarify, the reason why I believe webView should be an extension function that launches an activity is because in order to show UI, manager and plugins have to cooperate. Activities are a primitive on which basically any UI can be built upon, so manager implementing special support for it seems unnecessary.

If you think a low-level API access such as launching intents is necessary, next to webView and api, a strategy for downloaders to get the download link via an Intent can be added, too. ...

val downloader = downloader {
  intent {
      launch { app, version -> Intent(app, version) }
        result { intent -> intent.getStringExtra("url") to intent.getLongExtra("size") }
  }

   download { link, size -> URL(link).inputStream() to size }
}

I guess I am open to the idea, but what is the benefit of this over using a suspend function? How would the strategy implementation access the required functions to launch activities?

oSumAtrIX commented 3 months ago

Activities and intents are pretty common on Android. All plugins that require interaction need to use them for UI.

But they would be abstracted by the plugin API. The plugins wouldn't need to interact with activities if the downloader API can simply provide concise and domain specific APIs.

The suggested get API is small and you don't need it if you are using webView.

The get API does not convey any semantics about a webView each plugin would have to implement a function that calls get with an implementation that launches a webview and so on. So it is not just get which is small on its own, its get plus a whole implementation for webview. The plugin API providing this by default simplifies that and is thus smaller for the API user.

it needs to be in a non-compileOnly library

I am not well versed with the intrinsic of activities. I do not understand why it must not be a compileOnly dependency. I was imagining the plugin API would provide an extension function:

fun <T> DownloaderBuilder<T>.webView(block: WebViewStrategyBuilder<T>.() -> Unit) =
    strategy(WebViewStrategyBuilder<T>().apply(block).build())

whereas WebViewStrategyBuilder returns a lambda in which your activity stuff would be implemented:

class WebViewStrategy<T>(
    val show: (app: String, version: String?) -> String,
    val capture: (request: Request, response: Response) -> T?
) : Strategy<T> {
    override suspend fun get(app: String, version: String?): T {
        val uri = show(app, version)
        // Show a webview activity here and somehow capture requests/responses to get the result
        return capture(Request(), Response(uri, mapOf("size" to "1024")))!!
    }
}

so manager implementing special support for it seems unnecessary.

I did not understand that sentence. Can you reword it/clarify what you mean.

I guess I am open to the idea, but what is the benefit of this over using a suspend function? How would the strategy implementation access the required functions to launch activities?

If you are asking if launch & result are separate, there's no particular reason. Both can be suspendable (because the underlying strategy get function is also suspendable in which both launch and result are invoked. I just laid out an example for you with nice semantics. Of course an intent strategy builder could be implemented like this as well, i was just giving ideas:

val downloader = downloader {
    intent {
        result { intent -> 
            val intent = Intent(app, version)
            // your suspend stuff goes here
            intent.getStringExtra("url") to intent.getLongExtra("size")
        }
    }

   download { link, size -> URL(link).inputStream() to size }
}
Axelen123 commented 3 months ago

But they would be abstracted by the plugin API. The plugins wouldn't need to interact with activities if the downloader API can simply provide concise and domain specific APIs.

Yes, webView abstracts this away for webview plugins. Developers don't have to call requestUserInteraction() if the plugin does not require user input to function. Plugins only need it if they have to show custom UI and are not using webView.

The suggested get API is small and you don't need it if you are using webView.

The get API does not convey any semantics about a webView each plugin would have to implement a function that calls get with an implementation that launches a webview and so on. So it is not just get which is small on its own, its get plus a whole implementation for webview. The plugin API providing this by default simplifies that and is thus smaller for the API user.

Webview plugins don't need to define a get block because webView does it for them.

it needs to be in a non-compileOnly library

I am not well versed with the intrinsic of activities. I do not understand why it must not be a compileOnly dependency. I was imagining the plugin API would provide an extension function:

fun <T> DownloaderBuilder<T>.webView(block: WebViewStrategyBuilder<T>.() -> Unit) =
    strategy(WebViewStrategyBuilder<T>().apply(block).build())

whereas WebViewStrategyBuilder returns a lambda in which your activity stuff would be implemented:

class WebViewStrategy<T>(
    val show: (app: String, version: String?) -> String,
    val capture: (request: Request, response: Response) -> T?
) : Strategy<T> {
    override suspend fun get(app: String, version: String?): T {
        val uri = show(app, version)
        // Show a webview activity here and somehow capture requests/responses to get the result
        return capture(Request(), Response(uri, mapOf("size" to "1024")))!!
    }
}

The webView library contains an activity class, which is final and the user does not directly interact with it. The library needs to be implementation instead of compileOnly because the activity can't be launched later if it is not included in the plugin APK. The usage of webView could be:

val dl = downloader {
    webView {
        show { URL() }
        capture { .. }
    }

    download { .. }
}

Internally, webView defines a get block that launches the activity I mentioned earlier with the requestUserInteraction API.

so manager implementing special support for it seems unnecessary. I did not understand that sentence. Can you reword it/clarify what you mean.

A dialog is shown to the user asking them if they want to continue with required user interaction or skip the plugin. This is part of the design proposed by @Ushie. In order to implement this, Manager and the plugin need some sort of interface. Implementing it is easier if Manager only needs to worry about activities.

I guess I am open to the idea, but what is the benefit of this over using a suspend function? How would the strategy implementation access the required functions to launch activities?

If you are asking if launch & result are separate, there's no particular reason. Both can be suspendable (because the underlying strategy get function is also suspendable in which both launch and result are invoked. I just laid out an example for you with nice semantics. Of course an intent strategy builder could be implemented like this as well, i was just giving ideas:

val downloader = downloader {
  intent {
        result { intent -> 
          val intent = Intent(app, version)
          // your suspend stuff goes here
          intent.getStringExtra("url") to intent.getLongExtra("size")
      }
  }

   download { link, size -> URL(link).inputStream() to size }
}

I guess this could also be an extension function as well. I would suggest modifying the example at the top of the issue to add the GetScope from my earlier comment so manager can pass on the required APIs.

oSumAtrIX commented 3 months ago

The webView library contains an activity class, which is final and the user does not directly interact with it. The library needs to be implementation instead of compileOnly because the activity can't be launched later if it is not included in the plugin APK. The usage of webView could be:

The webview API classes are in the plugin API module. ReVanced Manager depends on that module. The Android application plugin shades the module classes into the final APK which means the activity for the webview API is shaded into ReVanced Manager meaning plugins that use the webview API also can use the activity just fine.

Manager and the plugin need some sort of interface. Implementing it is easier if Manager only needs to worry about activities.

Now I understand. So for the webview API you add a WebView activity. For some other API you add another respective activity. Both the webview and the other APIs would use the requestUserInteraction API of the internal plugin system then I assume? So the only public API is webview() and co, whereas requestUserInteraction is internal to the plugin API module. Since webview() is implemented in the plugin API module it uses the requestUserInteraction API to display its webview. This sounds fine to me.

Can you give an example of how the internal API design looks like?

Axelen123 commented 3 months ago

The webview API classes are in the plugin API module. ReVanced Manager depends on that module. The Android application plugin shades the module classes into the final APK which means the activity for the webview API is shaded into ReVanced Manager meaning plugins that use the webview API also can use the activity just fine.

I guess that would work. I never considered doing it like that.

Manager and the plugin need some sort of interface. Implementing it is easier if Manager only needs to worry about activities.

Now I understand. So for the webview API you add a WebView activity. For some other API you add another respective activity. Both the webview and the other APIs would use the requestUserInteraction API of the internal plugin system then I assume?

Yes, webview and plugins that need custom UIs both use the requestUserInteraction API. Plugins that use webview don't call requestUserInteraction manually.

So the only public API is webview() and co, whereas requestUserInteraction is internal to the plugin API module.

By "internal", do you mean completely inaccessible to plugins?

Can you give an example of how the internal API design looks like?

// API
interface GetScope {
    suspend fun requestUserInteraction(): ActivityLaunchPermit
}

interface ActivityLaunchPermit {
    suspend fun launch(intent: Intent): Intent?
}

// Usage
val downloader = downloader {
    get {
        // Receiver is "GetScope".
        val result = requestUserInteraction().launch(MyIntent())!!
        val url = result.getStringExtra("URL")!!

        something(url)
    }
}

Keep in mind that requestUserInteraction and the ActivityLaunchPermit need to be implemented by manager in some way if you are going to suggest a replacement.

oSumAtrIX commented 3 months ago

By "internal", do you mean completely inaccessible to plugins?

Yes. I was thinking it was an internal only API so we would provide wrappers to that internal function. webView is public API but calls requestUserInteraction internally. Now, if we want plugins to allow that as well, then we would, just like webView, provide intent for example:

val downloader = downloader {
    intent {
        launch { Intent(app, version) }
        result { it.getStringExtra("url") to it.getLongExtra("size") }
    }

   download { link, size -> URL(link).inputStream() to size }
}
Axelen123 commented 3 months ago

How would cases where interaction is only required conditionally be handled? A plugin that requires login might not need to ask the user to login if they have already logged in before. intent does not handle this.

Also, please provide an idea for how intent would get access to requestUserInteraction() since it needs to be implemented by manager.

oSumAtrIX commented 3 months ago

How would cases where interaction is only required conditionally be handled?

I don't quite get the interaction part. Is this just metadata provided by the plugin for ReVanced Manager so that ReVanced Manager can display a "This plugin needs interaction" dialog or similar? If so:

val downloader = downloader {
    intent(requiresInteraction = false) {
        launch { Intent(app, version) }
        result { it.getStringExtra("url") to it.getLongExtra("size") }
    }

   download { link, size -> URL(link).inputStream() to size }
}
Axelen123 commented 3 months ago

No, the dialog is displayed when requestUserInteraction() is called. The function returns if the user accepts and throws an exception if the user chooses to skip the plugin. In the example I was talking about, the interaction would be something like OAuth or asking the user for credentials. The plugin would use those credentials to do its job. Asking for credentials is unnecessary if the user has already provided them before.

oSumAtrIX commented 3 months ago

Offtopic

On second thought, having a launch and result function is not really necessary. It is readable and allows for flexible transformation grouped by scope though. The intent scope is the strategy on how we can get a download link. Inside this strategy we launch an intent and transform its result into something consumable by the download scope. So inside the intent scope everything regarding the intent is handled and to the outer scope something is nicely digestable is returned (in the current example an URL and Size pair). So each domain has its own nice small purpose. An alternative would be:

val downloader = downloader {
    intent(requiresInteraction = false) {
        handle { Intent(app, version) }
    }

   download { -> URL(it.getStringExtra("url")).inputStream() to it.getLongExtra("size") }
}

Here we simplify the strategy but as a side effect breach the intent scope. The intent now leaks out of the intent scope to the downloader scope. There is no transformation like before using a result function. So I believe having a launch and result function in the strategy is good.

An alternative would be if we can make transformations inside handle:

val downloader = downloader {
    intent {
        handle { 
            val result = Intent(app, version).launch(requiresInteraction = false)

            result .getStringExtra("url") to result.getLongExtra("size")
       }
    }

   download { link, size -> URL(link).inputStream() to size }
}

Here we retain domain-specific logic in its designated scopes by grouping both the previous launch and handle function into one. Might be less readable though, so I would still tend to my initial proposal.

Ontopic

No, the dialog is displayed when requestUserInteraction() is called. The function returns if the user accepts and throws an exception if the user chooses to skip the plugin. In the example I was talking about, the interaction would be something like OAuth or asking the user for credentials. The plugin would use those credentials to do its job. Asking for credentials is unnecessary if the user has already provided them before.

But why do we ask for permission? Just let the plugin show a view? Can requestUserInteraction() be called twice? Can you write a small API proposal of how it would look in your OAuth/Credentials example?

Axelen123 commented 2 months ago

But why do we ask for permission? Just let the plugin show a view?

It is part of Ushies design. Also, if there is no prompt the user might be thrown into a bunch of activities for downloaders they don't want to use for the app.

Activity API

The API for launching activities can be simplified. The new API would look like this: launch { Intent() } instead of: requestUserInteraction().launch(Intent()).

The return value of download

The download function shouldn't be forced to return Pair<InputStream, Long>. It will be really awkward for plugin developers if the libraries they are working with don't use InputStream. HTTP client librariies might not give you the size until you have started downloading and I don't know of any libraries that both return the size and an InputStream for downloading the file. A different idea is necessary. Look at the current implementation in the branch if you want to suggest something else.

The return value of strategies (or whatever we are going to call them)

The return value of strategies must implement Parcelable. This makes downloaders easier to work with inside Manager and encourages plugin developers to write data classes that only contain the information needed to find the APK. In the current implementation, the return value is an instance of App, which looks like this:

open class App(open val packageName: String, open val version: String) : Parcelable

The type can be used as-is if the plugin only needs the package name and version for the download. Plugin developers can create a data class that extends App if more data is necessary.

Loading plugins

Plugins are managed as Android apps instead of being handled by manager. Manager looks for installed applications that have this uses-feature tag in the manifest:

<uses-feature android:name="app.revanced.manager.plugin.downloader" />

The class containing the downloader plugin is declared using a meta-data tag. See the example plugin manifest for a full example. ReVanced Manager will only load plugins if the user marks them as trusted.

oSumAtrIX commented 2 months ago

It is part of Ushies design. Also, if there is no prompt the user might be thrown into a bunch of activities for downloaders they don't want to use for the app.

So, is this an opt-in API for downloaders? Can they launch intents if they want to without requesting? Is this something we want to provide as an API at all?

API would look like this: launch { Intent() } instead of: requestUserInteraction().launch(Intent()).

I think a better way would be an extension function Intent().requestLaunch() to imply you are requesting something. Just launch doesn't imply you are requesting, which is the important part of this API.

The extension function would set the internal-only lambda similar to how launch { } would. On a related note, is "launch" terminology we invented or something Android uses for starting intents? I would use whatever Android uses, for example, "requestStart" if start is used.

The return value of download

It should not be Pair<InputStream, Long> but Pair<InputStream, Long?> (nullable). An InputStream is the only correct choice here. A file system API is unnecessary for the downloader API and for the downloader implementation. You should use the type that fits the current situation correctly. You wouldn't use String instead of Integer either for counting something. Every downloader will open a stream at some point in time. If you are working with a library that only allows you to download to a file, you will either need to use a different library, as it clearly decided that the File type is necessary instead of an input stream and thus unsuitable to be used for a downloader implementation, or fork the library to suit the situation.

That being said, an input stream should be requested from a downloader. A file would be the incorrect type for the purpose. Regardless, a file-like API may be useful to a downloader. Why? Because the downloader may want to cache something to avoid consuming too much memory, for example. Given this proposal, the downloader can even use inappropriate APIs, abuse the cache to write to a file, and then open an input stream, thus satisfying the return type of the downloader API.

The reason I said "file-like" is because even in the case of caching, the type File is still wrong in the current domain. A more appropriate solution to solve the requirement "Caching" would be to introduce a controlled DSL such as:

cache("name") { stream -> }

cache is an API that, given a name, opens a closeable stream. Downloaders can use it to store something on disk for the lifespan of the downloader. They can access it given the same name at any time during the lifespan.

This is a much cleaner, smaller, and simpler API that precisely fulfills the requirement for caching without breaching the small and controlled domain with big, mostly useless for the current domain APIs provided by File. Due to this, a cache cannot be abused anymore to use libraries that require a File API, as such a requirement makes them automatically unsuitable for use as a library for a downloader implementation (technically, they can create and abuse a temp file using the Java APIs to still use the library even when unfit).

The return value of strategies must implement Parcelable.

What if I want to return a stream? I don't think that's parcelable. The constraint maybe makes sense outside of the downloader, somewhere deep in the internal code of ReVanced Manager, but that is a technicality that should not impair the implementation of a downloader. The API of a downloader should be predictable, minimal, and simple. Asking for Parcelable raises the question for the user, why the return type must be parcelable. The answer to that would be something the user should not even know about just to implement a downloader. A compromise would be to require an inner member of the type Parcelable. So the API would allow any type to be used but with an upper constraint that requires the type to implement Parcelable. This parcelable would be the "parcelizable" representation of the type it is inside and its purpose would be for the Manager. Like I said, this is a compromise and not a solution because it still does not solve the issue I mentioned regarding creating constraints on the downloader API when the constraint does not even remotely concern the implementor.

The class containing the downloader plugin is declared using a meta-data tag.

Can multiple downloaders be provided, each via one uses-feature tag?

Axelen123 commented 2 months ago

Can they launch intents if they want to without requesting?

Broadcasts and services can be sent/bound with no problems using the provided Context. Starting an activity without the API is not useful because the plugin does not get to handle the result.

Is this something we want to provide as an API at all?

Yes, because custom UIs are useful and it serves as a base for implementing webView { }.

I think a better way would be an extension function Intent().requestLaunch() to imply you are requesting something. Just launch doesn't imply you are requesting, which is the important part of this API.

The extension function would set the internal-only lambda similar to how launch { } would. On a related note, is "launch" terminology we invented or something Android uses for starting intents? I would use whatever Android uses, for example, "requestStart" if start is used.

I have no problem with changing the usage/signature. The Android API function for launching activities using an intent is called startActivity.

It should not be Pair<InputStream, Long> but Pair<InputStream, Long?> (nullable). An InputStream is the only correct choice here.

Give me the name of an HTTP client library that returns an InputStream and the size of the file being downloaded. If no such library exists, everyone will be forced to implement InputStream manually or resort to workarounds such as saving to a temporary file (which causes the indicators in the UI to not accurately reflect the actual download progress). Neither outcome is desirable.

cache is an API that, given a name, opens a closeable stream. Downloaders can use it to store something on disk for the lifespan of the downloader. They can access it given the same name at any time during the lifespan.

Java provides an API for creating temporary files, so providing a dedicated API for it seems unnecessary.

What if I want to return a stream? I don't think that's parcelable.

You don't return a stream, you store whatever you use to get the stream instead. This could be a file path, URL or an ID for a remote resource.

The constraint maybe makes sense outside of the downloader, somewhere deep in the internal code of ReVanced Manager, but that is a technicality that should not impair the implementation of a downloader.

The root cause of the constraint comes from the fact that the lifecycle of the Jetpack Compose composition is tied to an Activity, so users need to save and restore state to avoid losing it. This constraint is propagated until it reaches the downloaders. We don't have any control over this.

The API of a downloader should be predictable, minimal, and simple. Asking for Parcelable raises the question for the user, why the return type must be parcelable. The answer to that would be something the user should not even know about just to implement a downloader.

Parcelable may have been a very annoying interface to implement 10 years ago, but this is not the case anymore. Parcelable is very common in the Android ecosystem and its appearance here would not be considered out of place by anyone who knows what the interface is for. ReVanced Manager is made for Android so asking for basic Android development knowledge is not wrong here.

A compromise would be to require an inner member of the type Parcelable. So the API would allow any type to be used but with an upper constraint that requires the type to implement Parcelable. This parcelable would be the "parcelizable" representation of the type it is inside and its purpose would be for the Manager. Like I said, this is a compromise and not a solution because it still does not solve the issue I mentioned regarding creating constraints on the downloader API when the constraint does not even remotely concern the implementor.

I don't understand this. What do you mean by "inner member"?

Can multiple downloaders be provided, each via one uses-feature tag?

You would not need to specify the manifest attributes multiple times. Manager could simply load multiple downloaders from a single class instead. Regardless, I don't think supporting multiple downloaders in a single APK is necessary right now. It can still be implemented in the future.

oSumAtrIX commented 2 months ago

using the provided Context

Is it necessary to pass the entire context down? Context for the purpose of launching intents can already be done internally via a public facing API.

Is this something we want to provide as an API at all?

Yes, because custom UIs are useful and it serves as a base for implementing webView { }.

I meant is giving entire context to the API something we want? Creating some constrained APIs which make calls to context APIs would simplify the API because the API would not need to expose the downloader implementation to the entire context APIs.

The Android API function for launching activities using an intent is called startActivity

How about requestStartActivity? We are passing an intent though and not an activity, so i don't know why Android calls it activity. I guess the intent carries the activity we want to start.

Give me the name of an HTTP client library that returns an InputStream and the size of the file being downloaded

Ktor client provides an API to open a stream. It can also read the headers, pause the stream and pass down the same open stream for reading the body. Since the header carries the size, it can provide the size for the file being downloaded. Another alternative is Java's native URL. It has a default handler for the HTTP protocol to open a stream.

everyone will be forced to implement InputStream manually or resort to workarounds such as saving to a temporary file (which causes the indicators in the UI to not accurately reflect the actual download progress). Neither outcome is desirable

Nobody is forced to implement input stream. File is an abstraction of input stream not the other way round. You would have to use a File when just a stream is necessary, but not the other way round. If just a stream is necessary, you do not need a File. How downloaders handle providing a stream is effectively their responsibility. It only makes sense to ask for an input stream. The file API is wrong as depicted with the String/Integer example in my previous comment.

Java provides an API for creating temporary files, so providing a dedicated API for it seems unnecessary.

The API breaches the domain. It provides access to an entire File API as explained in my previous comment. I have followed with a proposal for this issue.

You don't return a stream, you store whatever you use to get the stream instead. This could be a file path, URL or an ID for a remote resource.

Well, what I return is not really your concern. I return it for my purpose.

The root cause of the constraint comes from the fact that the lifecycle of the Jetpack Compose composition is tied to an Activity, so users need to save and restore state to avoid losing it. This constraint is propagated until it reaches the downloaders. We don't have any control over this.

Thats detailed intrinsics and technicality which now burdes the implementor of the downloader. Like explained earlier, this is a black-box constraint for them and is anything but comprehensible, unless they themselves delve deep in this technicality. Can the activity not be reused or something shared?

Parcelable is very common in the Android ecosystem and its appearance here would not be considered out of place

Can you make Stream parcelable? Or a lambda?

I don't understand this. What do you mean by "inner member"?

abstract class Data<T: Parcelable> {
    abstract val parcelableData: T
}

parcelableData is a member of Data, which is what I meant.

You would not need to specify the manifest attributes multiple times. Manager could simply load multiple downloaders from a single class instead

Downloaders are instance referenced by properties. I could declare them in multiple files so Kotlin would generate multiple classes. On a related note, since the outer class is generated, how would the user reference it in the manifest? They could guess the generated name, but that's kinda unintuitive. Why not load all public accessible properties of type downloader? The user wouldn't need to declare them in manifest and manager could load them from any class.

Axelen123 commented 2 months ago

using the provided Context

Is it necessary to pass the entire context down? Context for the purpose of launching intents can already be done internally via a public facing API.

Is this something we want to provide as an API at all?

Yes, because custom UIs are useful and it serves as a base for implementing webView { }.

I meant is giving entire context to the API something we want? Creating some constrained APIs which make calls to context APIs would simplify the API because the API would not need to expose the downloader implementation to the entire context APIs.

Binding to services or retrieving localized string resources for toast/error messages are examples of valid use-cases for Context. I am sure there are more. Third-party libraries used by plugins could also require a Context.

The Android API function for launching activities using an intent is called startActivity

How about requestStartActivity? We are passing an intent though and not an activity, so i don't know why Android calls it activity. I guess the intent carries the activity we want to start.

Maybe startActivityRequest or requestActivity sound better? The Android function is called startActivity because it starts an activity using the provided Intent (references which activity to start and the "arguments" for that activity).

Give me the name of an HTTP client library that returns an InputStream and the size of the file being downloaded

Ktor client provides an API to open a stream. It can also read the headers, pause the stream and pass down the same open stream for reading the body. Since the header carries the size, it can provide the size for the file being downloaded. Another alternative is Java's native URL. It has a default handler for the HTTP protocol to open a stream.

In the case of Ktor, you cannot return a stream if you are using the prepareX and execute API. The other functions offered by Ktor store the response in memory so any streams created using them will not reflect the actual download. I am not familiar with the Java API. How do you set request headers and read response headers?

everyone will be forced to implement InputStream manually or resort to workarounds such as saving to a temporary file (which causes the indicators in the UI to not accurately reflect the actual download progress). Neither outcome is desirable

Nobody is forced to implement input stream. File is an abstraction of input stream not the other way round. You would have to use a File when just a stream is necessary, but not the other way round. If just a stream is necessary, you do not need a File. How downloaders handle providing a stream is effectively their responsibility. It only makes sense to ask for an input stream. The file API is wrong as depicted with the String/Integer example in my previous comment.

The signature would make sense for an extension function, but it does not seem flexible enough for real world use-cases. Look at the code for the play store plugin. It downloads splits, merges them and then writes the result to the output. The plugin would not be able to accurately report download progress since the UI indicators are tied to the InputStream which cannot exist until all the splits have been downloaded and merged. A more flexible API is required to support this. The play store plugin can and does correctly report download progress and must still be able to do so even after API changes. The new API does not necessarily have to use files, it can use OutputStream or something, but it needs to separate download progress tracking from the process of outputting the APK. The Pair<InputStream, Long?> API can still be provided using extension functions. We could even create an extension function where you just return a URL.

Java provides an API for creating temporary files, so providing a dedicated API for it seems unnecessary.

The API breaches the domain. It provides access to an entire File API as explained in my previous comment. I have followed with a proposal for this issue.

We don't get to decide which standard library APIs can and can't be used. That is up to plugin developers since they are the ones writing that code. We do not need to provide an API for caching because plugins can implement caching themselves through the Java API or a third-party library.

You don't return a stream, you store whatever you use to get the stream instead. This could be a file path, URL or an ID for a remote resource.

Well, what I return is not really your concern. I return it for my purpose.

Manager does not care about the implementation of your plugin. Manager only sees App. You can use the class as-is or extend it to store more data if needed.

The root cause of the constraint comes from the fact that the lifecycle of the Jetpack Compose composition is tied to an Activity, so users need to save and restore state to avoid losing it. This constraint is propagated until it reaches the downloaders. We don't have any control over this.

Thats detailed intrinsics and technicality which now burdes the implementor of the downloader. Like explained earlier, this is a black-box constraint for them and is anything but comprehensible, unless they themselves delve deep in this technicality.

Activity recreation and system-initiated process death are well documented and every Android app developer has to account for them. It is impossible to write a usable Android app if you are constantly losing your state.

Can the activity not be reused or something shared?

State stored inside ViewModels or global variables are not affected by activity recreation, but any state stored inside them will still be lost on system-initiated process death. Apps handle process death by reloading state from a database, the filesystem, etc (Manager does this with patch bundles for example) or by saving and restoring it with the parcel system. We cannot prevent the system from killing the app process.

Parcelable is very common in the Android ecosystem and its appearance here would not be considered out of place

Can you make Stream parcelable? Or a lambda?

Streams are not parcelable. Lambdas are if they implement java.io.Serializable through the @JvmSerializableLambda annotation or you use a fun interface with Serializable as a superinterface. You do not need to return those types from get/similar if you implement your plugin sensibly by storing URLs, files, etc instead. Regardless, your reply does not have anything to do with my original statement that using Parcelable here is not out of place.

The Parcelable requirement isn't there to force developers to write their plugins in a specific way, it is only present for technical reasons.

I don't understand this. What do you mean by "inner member"?

abstract class Data<T: Parcelable> {
    abstract val parcelableData: T
}

parcelableData is a member of Data, which is what I meant.

How does having parcelableData help here? You would not be able to restore the Data instance in case of process death since Data is not Parcelable.

You would not need to specify the manifest attributes multiple times. Manager could simply load multiple downloaders from a single class instead

Downloaders are instance referenced by properties. I could declare them in multiple files so Kotlin would generate multiple classes. On a related note, since the outer class is generated, how would the user reference it in the manifest? They could guess the generated name, but that's kinda unintuitive.

Generated class names are very easy to predict and can be fully controlled through annotations such as @JvmName and @JvmMultifileClass if desired. This is no different from referencing a main function. The plugin template also shows you how to predict the class name, so developers don't need to figure that out themselves.

Why not load all public accessible properties of type downloader? The user wouldn't need to declare them in manifest and manager could load them from any class.

Classloaders do not tell you which classes exist.

Talking about supporting multiple plugins inside a single app seems unnecessary since it is not something we are going to implement on release.

oSumAtrIX commented 2 months ago

Binding to services or retrieving localized string resources for toast/error messages are examples of valid use-cases for Context. I am sure there are more. Third-party libraries used by plugins could also require a Context.

If localization is the only thing we want downloaders to have, why not provide a concise DSL for it instead of just giving it context? Every downloader would have to write the same boilerplate the DSL would have implemented.

Third-party libraries used by plugins could also require a Context.

Why would a downloading library need an Android context?

Maybe startActivityRequest

Well, this would mean we start a request, but instead we request to start an activity.

or requestActivity sound better?

Likewise, we do not request an activity, we request to start an activity.

In the case of Ktor, you cannot return a stream if you are using the prepareX and execute API. The other functions offered by Ktor store the response in memory so any streams created using them will not reflect the actual download. I am not familiar with the Java API. How do you set request headers and read response headers?

I am not sure what you are referring to, but I am able to open an input stream to read the request body. The headers can be read regardless of that, after all its a HTTP library. The functionality to get the stream is rarely missing in a HTTP library because

A. HTTP libraries have nothing to do with Files so it would be odd to ask for File in such a library B. Writing the body to byte[] would be inefficient

Which is why, basically all HTTP libraries will respond with a stream (optionally byte[] as a helper function)

Look at the code for the play store plugin.

I don't think I understand your problem. The play store plugin can request the files, read the header for the size, stream the body (apk files) to cache, and then open a stream from it to read the separate files and merge them. The merged file can be written to cache again and a stream can be opened for it to return in download.

Working with streams here works just fine. Caching is not impaired and the entire File API does not poison the DSL.

We don't get to decide which standard library APIs can and can't be used

Yes we do. We are writing the API for the downloader and thus are the ones in charge of controlling which APIs are accessible. By creating concise functions we can create a very small and concise API perfectly cut for the current domain requirements. A File API provides much more than the domain asks for. What the plugin does inside the domain on the other hand is not something we get to decide. They are free to ab-use the File API even though, the current domain does not ask for one. Why ab-use? Because at no point in time does the domain ask for a File API. It only ever needs a stream. If you are using a library that somehow needs File, it therefore is automatically an unsuitable library for a plugin implementation because it leads to ab-using the File API. The correct course of action when being in such a situation is to either use a library that supports streams and therefore is a valid choice in the current domain (No File APIs are needed) or fork the library to not use File APIs.

Manager does not care about the implementation of your plugin. Manager only sees App. You can use the class as-is or extend it to store more data if needed.

So the abstract Strategy classes abstract get function returns App instead of T? Sure, but sounds a bit restricting. T is simpler because for example the strategy api could return a URL as T. The function download would receive it and open a stream. With the restriction App we would have to

  1. Create an implementing class
  2. Add a field to store the URL
  3. Break Liskov because we don't have anything else besides an URL and an URL is not an App

Activity recreation and system-initiated process death are well documented and every Android app developer has to account for them. It is impossible to write a usable Android app if you are constantly losing your state.

Well, I am writing a downloader. Why as someone who is perfectly capable of writing a downloader, now I have to delve into Android intrinsic? Yes, there's a little Android boilerplate such as setting up the project, or adding stuff to the manifest, that's not possible to mitigate, but what is mitigatable is breaching the downloader DSL with stuff that only concerns Android and not downloaders.

State stored inside ViewModels or global variables are not affected by activity recreation, but any state stored inside them will still be lost on system-initiated process death. Apps handle process death by reloading state from a database, the filesystem, etc (Manager does this with patch bundles for example) or by saving and restoring it with the parcel system. We cannot prevent the system from killing the app process.

We'll Manager is one process. Unless Manager is killed why would we lose the state of downloader instances?

How does having parcelableData help here? You would not be able to restore the Data instance in case of process death since Data is not Parcelable.

You can also not parcelize streams. Choose your poison.

Classloaders do not tell you which classes exist

At least with JAR files you can iterate through the entries and get all classes that way, but I suppose this would be difficult for dex files (possible via dexlib)

Axelen123 commented 2 months ago

Binding to services or retrieving localized string resources for toast/error messages are examples of valid use-cases for Context. I am sure there are more. Third-party libraries used by plugins could also require a Context.

If localization is the only thing we want downloaders to have, why not provide a concise DSL for it instead of just giving it context? Every downloader would have to write the same boilerplate the DSL would have implemented.

Like I said, there are several valid uses for Context (sending broadcasts, bound services, resources, assets). Why should we reinvent all of these APIs when we can easily provide Context instead? Context already has documentation and examples. Developers that know how to use Context also would not need to learn new APIs.

It is possible to block access to functions such as startActivity if desired by using ContextWrapper and overriding startActivity. This would stop the DSL breaches you mentioned.

Third-party libraries used by plugins could also require a Context.

Why would a downloading library need an Android context?

An IPC library for communicating with an app store not made by the plugin developer would ask for a Context.

In the case of Ktor, you cannot return a stream if you are using the prepareX and execute API. The other functions offered by Ktor store the response in memory so any streams created using them will not reflect the actual download. I am not familiar with the Java API. How do you set request headers and read response headers?

I am not sure what you are referring to, but I am able to open an input stream to read the request body. The headers can be read regardless of that, after all its a HTTP library.

Show me Ktor code that does this.

Look at the code for the play store plugin.

I don't think I understand your problem. The play store plugin can request the files, read the header for the size, stream the body (apk files) to cache, and then open a stream from it to read the separate files and merge them. The merged file can be written to cache again and a stream can be opened for it to return in download.

The problem here is that the play store plugin is capable of tracking the download progress of the splits, but cannot report that progress to the UI with the suggested API. In the suggested API, the indicators are updated when data is read from the returned InputStream. The InputStream cannot exist until the splits have been merged, and the splits cannot be merged until every split has been downloaded.

We don't get to decide which standard library APIs can and can't be used

Yes we do. We are writing the API for the downloader and thus are the ones in charge of controlling which APIs are accessible. By creating concise functions we can create a very small and concise API perfectly cut for the current domain requirements. A File API provides much more than the domain asks for. What the plugin does inside the domain on the other hand is not something we get to decide. They are free to ab-use the File API even though, the current domain does not ask for one. Why ab-use? Because at no point in time does the domain ask for a File API. It only ever needs a stream. If you are using a library that somehow needs File, it therefore is automatically an unsuitable library for a plugin implementation because it leads to ab-using the File API. The correct course of action when being in such a situation is to either use a library that supports streams and therefore is a valid choice in the current domain (No File APIs are needed) or fork the library to not use File APIs.

There is no way to prevent someone from calling the tempfile APIs or any other standard library API even if we tell them not to. This is why I said we don't get to decide here. We don't have to use File in the DSL because alternate solutions are possible.

Manager does not care about the implementation of your plugin. Manager only sees App. You can use the class as-is or extend it to store more data if needed.

So the abstract Strategy classes abstract get function returns App instead of T? Sure, but sounds a bit restricting. T is simpler because for example the strategy api could return a URL as T. The function download would receive it and open a stream. With the restriction App we would have to

1. Create an implementing class

2. Add a field to store the URL

3. Break Liskov because we don't have anything else besides an URL and an URL is not an App

Creating an implementing class is really easy, just look at the play store plugin. The library can provide an App implementation for the API strategy if the strategy requires users to return a URL. An alternative would be to make the API strategy generic and allow the user to return an App or their own implementation. The packageName and version fields are used by manager, which is why App exists. Plugins do not need to create their own class if they only need the package name and version inside the download block.

Activity recreation and system-initiated process death are well documented and every Android app developer has to account for them. It is impossible to write a usable Android app if you are constantly losing your state.

Well, I am writing a downloader. Why as someone who is perfectly capable of writing a downloader, now I have to delve into Android intrinsic? Yes, there's a little Android boilerplate such as setting up the project, or adding stuff to the manifest, that's not possible to mitigate, but what is mitigatable is breaching the downloader DSL with stuff that only concerns Android and not downloaders.

Saving state is a requirement imposed on Manager by Android. Manager cannot fulfill this requirement unless downloader state can be saved, so mitigation is not possible here. You don't need to know how to implement Parcelable manually since the implementation can be generated by the parcelize plugin.

State stored inside ViewModels or global variables are not affected by activity recreation, but any state stored inside them will still be lost on system-initiated process death. Apps handle process death by reloading state from a database, the filesystem, etc (Manager does this with patch bundles for example) or by saving and restoring it with the parcel system. We cannot prevent the system from killing the app process.

We'll Manager is one process. Unless Manager is killed why would we lose the state of downloader instances?

The system will kill the process of apps that are not being used, even if they are still in the task list. You have to account for this by saving your state, which is what Parcelable is for. A correctly written Android app will restore the state and bring the user back to the most recent screen when the user navigates back after the process was killed by the system.

How does having parcelableData help here? You would not be able to restore the Data instance in case of process death since Data is not Parcelable.

You can also not parcelize streams. Choose your poison.

This does not answer my question.

oSumAtrIX commented 2 months ago

Like I said, there are several valid uses for Context (sending broadcasts, bound services, resources, assets). Why should we reinvent all of these APIs when we can easily provide Context instead? Context already has documentation and examples. Developers that know how to use Context also would not need to learn new APIs.

It does more than we need. Why would a downloader send a broadcast, bind services, use resources or assets? It should simply download a file.

An IPC library for communicating with an app store not made by the plugin developer would ask for a Context.

Which app store?

Show me Ktor code that does this.

https://ktor.io/docs/client-responses.html#streaming

The problem here is that the play store plugin is capable of tracking the download progress of the splits, but cannot report that progress to the UI with the suggested API.

Why not? Like i said before, it can read the headers then stream the body when downloading the separate splits.

There is no way to prevent someone from calling the tempfile APIs or any other standard library API

Refer to what I said:

image

We don't have to use File in the DSL because alternate solutions are possible

Well, that's what I am saying. The API should ask for an input stream. It's the most primitive form of receiving a downloaded file. No File API, no Context or anything that poisons the concise DSL, just a stream.

An alternative would be to make the API strategy generic

That's exactly how it is proposed right now. It is generic. What you said before is that the upper bound should be App and Parcelable on top.

return an App or their own implementation

Back to what you quoted:

image

The packageName and version fields are used by manager

What is this used for? Manager is supposed to retrieve the return value of Strategy#get as Any? when it is querying a downloader that can download the requested app, assuming the ideal case of no upper bound, so a black-box. Then it simply passes that black-box to Downloader#download once the user decides to start the download.

Saving state is a requirement imposed on Manager by Android

I don't know how Android works, but you should be able to keep a reference for the entire duration of the entire app process if you want to. Meaning, that you do not have to save and restore anything. Just reference Strategy#get until you get to Downloader#download. So far you haven't mentioned anything that prevents that.

The system will kill the process of apps that are not being used, even if they are still in the task list. You have to account for this by saving your state, which is what Parcelable is for. A correctly written Android app will restore the state and bring the user back to the most recent screen when the user navigates back after the process was killed by the system.

If I exit the app and open it again, the process was not killed. Assuming it is killed, then you simply go back to the downloader selector screen and restore whatever you can restore, but repeat the process where Strategy#get has to be called. The efficiency decline here is negligible, because for one, we are talking about an edge case scenario and second, the dev implementing the plugin does not need to worry about some arbitrary (to them) constraint

Axelen123 commented 2 months ago

Like I said, there are several valid uses for Context (sending broadcasts, bound services, resources, assets). Why should we reinvent all of these APIs when we can easily provide Context instead? Context already has documentation and examples. Developers that know how to use Context also would not need to learn new APIs.

It does more than we need. Why would a downloader send a broadcast, bind services, use resources or assets? It should simply download a file.

Context is not used to download anything, it can be used to implement plugins. The play store plugin shows a real world use of Context.

An IPC library for communicating with an app store not made by the plugin developer would ask for a Context.

Which app store?

It was an example. There is still no good reason to reinvent the APIs here.

Show me Ktor code that does this.

ktor.io/docs/client-responses.html#streaming

The httpResponse is not valid outside of the execute block like I said before. You cannot return an InputStream with this unless you put the body in a ByteArrayInputStream or something.

The problem here is that the play store plugin is capable of tracking the download progress of the splits, but cannot report that progress to the UI with the suggested API.

Why not? Like i said before, it can read the headers then stream the body when downloading the separate splits.

You don't merge the splits by concatenating the downloaded APK files, there is a processing step before you get the final APK. How would you make the UI indicator reflect the download progress of the splits? The current API fully supports this, but the InputStream API does not.

There is no way to prevent someone from calling the tempfile APIs or any other standard library API

Refer to what I said:

image

We cannot prevent plugins from creating tempfiles, so why do we need to provide it ourselves? How is calling a standard library function different and bad compared to something that downloaders would definitely use like an HTTP client library? What real impact does it have?

We don't have to use File in the DSL because alternate solutions are possible

Well, that's what I am saying. The API should ask for an input stream. It's the most primitive form of receiving a downloaded file. No File API, no Context or anything that poisons the concise DSL, just a stream.

Yet the InputStream API alone cannot properly support things like the play store plugin. We need to provide something that can. Refer to the OutputStream suggestion in this comment.

return an App or their own implementation

Back to what you quoted:

image

For 1 and 2, refer to my previous comment. What is number 3 and how is it a problem?

The packageName and version fields are used by manager

Both are used to assert that the downloader provided the proper package and version as requested by Manager. They will be used in the UI as well.

What is this used for? Manager is supposed to retrieve the return value of Strategy#get as Any? when it is querying a downloader that can download the requested app, assuming the ideal case of no upper bound, so a black-box. Then it simply passes that black-box to Downloader#download once the user decides to start the download.

This would have worked if the things in App were not required.

Saving state is a requirement imposed on Manager by Android

I don't know how Android works, but you should be able to keep a reference for the entire duration of the entire app process if you want to. Meaning, that you do not have to save and restore anything. Just reference Strategy#get until you get to Downloader#download. So far you haven't mentioned anything that prevents that.

The system will kill the process of apps that are not being used, even if they are still in the task list. You have to account for this by saving your state, which is what Parcelable is for. A correctly written Android app will restore the state and bring the user back to the most recent screen when the user navigates back after the process was killed by the system.

If I exit the app and open it again, the process was not killed. Assuming it is killed, then you simply go back to the downloader selector screen and restore whatever you can restore, but repeat the process where Strategy#get has to be called. The efficiency decline here is negligible, because for one, we are talking about an edge case scenario and second, the dev implementing the plugin does not need to worry about some arbitrary (to them) constraint

Navigating away from the last screen is bad UX because you are violating the user expectation that the app will be in the same state you left it in when you exited the app. Going through plugin interactions again is not good UX either. Parcelable exists, so there is no good technical reason for it either.

Parcelable does not not prevent anyone from implementing plugins because developers can resort to hacks like storing non-parcelable data in a global Map to transport it from get to download. The Map would be reset if the Manager process dies, which is why I wouldn't recommend doing it but that is not the point.

oSumAtrIX commented 2 months ago

The play store plugin shows a real world use of Context.

How is it using Context?

There is still no good reason to reinvent the APIs here.

The good reason is that it retains the point of using a DSL in the first place.

You cannot return an InputStream with this unless you put the body in a ByteArrayInputStream or something.

You can but ktor suggests against it. Ktor was just an example to you asking:

image

Likewise any other HTTP library as mentioned previously can open an input stream including URL#openStream().

You don't merge the splits by concatenating the downloaded APK files, there is a processing step before you get the final APK. How would you make the UI indicator reflect the download progress of the splits? The current API fully supports this, but the InputStream API does not.

Split APKs aren't a concept. Manager only knows regular APK files and therefore expects a stream for it. How you get that stream is a black-box to Manager. Manager expects a stream rightfully because it expects one APK by design and therefore is rightfully in charge of taking the task from the plugin to handle downloading. If it did not, every plugin developer would have to implement the downloading task which is incorrect abstraction because you would put a common task in the impl.

Regardless, Manager expecting a stream is still not preventing you from working with splits in your downloader, because what you do in the implementation is a black-box to Manager. Manager inteprets this black box as "Get APK download input stream". Your implementation involves:

  1. Downloading the splits
  2. Merging the splits
  3. Returning the final APK input stream

Technically speaking:

Like previously explained your implementation is a black box to Manager, which manager calls "Get APK download input stream". Therefore, once Manager starts reading the input stream, the black box implementation for that input stream would:

  1. Stream the download of the splits
  2. Stream the merge of the splits
  3. Return the final APK stream

As you can see, all three tasks are interpretable as "Get APK download input stream". Thus, the "pre-processing" of handling splits becomes integrated into the download process, aligning with the Manager's design and expectations.

As established earlier, Manager is rightfully in charge of handling the download of an APK file, in terms of abstraction. You currently inject a callback to download though, so the plugin implementation must handle downloading when Manager is supposed to. You justify this requirement by arguing with a design that Manager does not support. Therefore the question arises, which scenarios aside from unsupported ones such as splits, call for a callback to handle downloading by a plugin instead of Manager? Unless there's a requirement in alignment with the design of Manager, Manager must rightfully handle downloading in stead of the plugin due to abstraction.

Let's assume you found a reason and indeed abstraction wise and in alignment with Managers design it makes sense to require downloaders to handle the downloading part. Then we add a callback like it currently exists so the downloader can take charge of reporting the download progress. Yet even now, this does not change that Manager should expect an input stream. Sure, the downloader can be in charge of downloading split APK files now since we found a reason for downloaders to handle downloading instead of Manager, but at the end of the day we simply return a stream of the merged APK file to Manager. A File, regardless of your concern regarding split apks or not, is not required by Manager and thus an input stream should be returned. That said, if we want downloaders to handle downloading instead of Manager, we need a reason that does not violate Managers design (like splits). If you have one, we can incorporate a callback, but still require returning an input stream.

download { progress
 while (isDownloading) progress += current to final
 return cache["file"].inputStream()
}

We cannot prevent plugins from creating tempfiles, so why do we need to provide it ourselves? How is calling a standard library function different and bad compared to something that downloaders would definitely use like an HTTP client library? What real impact does it have?

We provide an API because a File API is not needed by the DSL. The plugin API expects plugins to cache though so it should not expect plugins to rely on concepts that are not needed but instead provide APIs to satisfy the requirement.

Calling a std lib function is not bad but it should not be used if not required in a DSL. A File API is not required in the DSL. Without providing an API the user is now forced to use a File API. Therefore we provide the DSL-compliant API.

Yet the InputStream API alone cannot properly support things like the play store plugin. We need to provide something that can. Refer to the OutputStream suggestion in https://github.com/ReVanced/revanced-manager/issues/2064#issuecomment-2263252285 comment.

The InputStream API does not prevent the play store plugin. Like explained above, if a requirement for outsourcing downloading to a plugin implementation has been found under alignment with Managers design, a callback can be added, but an input stream should still be asked. So as you can see if we were to add a callback (need a reason), the play store plugin would work with an input stream API.

For 1 and 2, refer to my previous comment. What is number 3 and how is it a problem?

1 and 2 aren't a problem but 3 is. Basically an implementation of App must be App semantically. Lets assume our strategy returns an URL. An URL is not an App so creating an implementation of the upper bound "App" via URL would violate Liskov, as URL is not an App. This problem is visible for example by not being able to implement the package name property of App, because we only have an URL.

Both are used to assert that the downloader provided the proper package and version as requested by Manager. They will be used in the UI as well.

&

This would have worked if the things in App were not required.

This is a new undiscussed requirement, but no problem, this can be incorporated. Instead of returning T as proposed, we return Package to T. A strategy must always return that pair. Component1 (Package) would satisfy the new requirement , which is asserting T is indeed what we want to download whereas Component2 (T) is something Manager would pass to download once the assertions about Package have been made. To me this doesn't sound like something we need to implement though. If Manager calls downloader.get(pkg, version) and downloader returns some different pkg and version , the downloader is simply broken and needs to be fixed. Also the downloader API gets simpler, no App upper bound, no parcelizable, no Pair of Package to . Everything remains as intuitive as it is currently proposed by removing a precautios assertion that only handles a very small edge case scenario.

Navigating away from the last screen is bad UX because you are violating the user expectation that the app will be in the same state you left it in when you exited the app. Going through plugin interactions again is not good UX either. Parcelable exists, so there is no good technical reason for it either.

Well the process was killed wasn't it? The user can expect some progress being lost just fine, especially if its little progress. Lets assume the user enters the download screen, navigates through the webview and clicks download. They then exit the app and do something else for a while, now the system kills the app. When the user comes back to it, they would see the last restorable state which is for example where the strategy would be called again (which displays the webview). Like I said previously, quite the edge case scenario which if handled would restrain the plugin implementor to an upper constraint Parcelizable which renders returning streams or anything similar useful that cant be parcelized useless. So I would suggest to not handle a very slim edge case scenario, as a compromise restore the last restorable state in favour of greatly simplifying and empowering the plugin API.

Parcelable does not not prevent anyone from implementing plugins because developers can resort to hacks like storing non-parcelable data in a global Map to transport it from get to download. The Map would be reset if the Manager process dies, which is why I wouldn't recommend doing it but that is not the point.

Well, if the process dies in either way a stream for example is lost. If we remove parcelable, the state is lost, if we don't remove it, the global Map you mentioned is lost so I don't see the point of this suggestion.

Axelen123 commented 2 months ago

Plugin interface DSL

The problem of implementing a downloader is not specific enough to be solved purely through a DSL. Plugins will need to use things that are not part of the DSL. The only goal of the plugin DSL should be to construct instances and provide an interface between the plugin and plugin host. We don't need cache or reinvented Context APIs here, and their absence makes the DSL simpler. The get DSL I am suggesting does not constrain plugins with strategies and is still flexible enough to support desirable extensions like webView.

Http/InputStream

You cannot return an InputStream with this unless you put the body in a ByteArrayInputStream or something.

You can but ktor suggests against it.

The HttpResponse is not valid outside execute. You cannot properly return an InputStream with that API. If I am wrong, send a code snippet that uses Ktor to properly return an InputStream.

Likewise any other HTTP library as mentioned previously can open an input stream including URL#openStream().

How do you set headers with Java's URL API?

(This is not the entire statement, more parts are relevant but the original is huge) The InputStream API does not prevent the play store plugin. Like explained above, if a requirement for outsourcing downloading to a plugin implementation has been found under alignment with Managers design, a callback can be added, but an input stream should still be asked. So as you can see if we were to add a callback (need a reason), the play store plugin would work with an input stream API.

Yes, the InputStream API does not prevent the implementation of the play store plugin since you could still mostly use files to pull it off, the problem is/was with the progress indicators. Downloading the file is definitely the responsibility of the plugin because only the plugin knows how the APK source works, it is not the same for everyone. This is the reason why having download is justified. We need an API that can guarantee no plugin is forced to deal with incorrect UI indicators, which would be bad UX. Our opinions on design or what is and isn't "supported" should not be an obstacle to implementing plugins or achieving proper UX.

Why do we need InputStream when OutputStream can be used instead? You could still utilize InputStreams in an OutputStream-based API, since functions like copyTo exist. An OutputStream-based API would be easier to work with in situations where the libraries don't have or use InputStream. It would also make cleanup easier since you wouldn't have to make an object : InputStream and wrap an existing stream or something just to override close.

Parcelable

Navigating away from the last screen is bad UX because you are violating the user expectation that the app will be in the same state you left it in when you exited the app. Going through plugin interactions again is not good UX either. Parcelable exists, so there is no good technical reason for it either.

Well the process was killed wasn't it? The user can expect some progress being lost just fine, especially if its little progress. Lets assume the user enters the download screen, navigates through the webview and clicks download. They then exit the app and do something else for a while, now the system kills the app. When the user comes back to it, they would see the last restorable state which is for example where the strategy would be called again (which displays the webview). Like I said previously, quite the edge case scenario which if handled would restrain the plugin implementor to an upper constraint Parcelizable which renders returning streams or anything similar useful that cant be parcelized useless. So I would suggest to not handle a very slim edge case scenario, as a compromise restore the last restorable state in favour of greatly simplifying and empowering the plugin API.

&

Parcelable does not not prevent anyone from implementing plugins because developers can resort to hacks like storing non-parcelable data in a global Map to transport it from get to download. The Map would be reset if the Manager process dies, which is why I wouldn't recommend doing it but that is not the point.

Well, if the process dies in either way a stream for example is lost. If we remove parcelable, the state is lost, if we don't remove it, the global Map you mentioned is lost so I don't see the point of this suggestion.

The point of what I wrote was to prove that the Parcelable requirement doesn't prevent you from doing anything that would otherwise have been possible without it. This includes smuggling an InputStream from get to download. The user does not know anything about system-initiated process death, which means they don't expect any state loss. Introducing UX issues is unnecessary and having Parcelable is definitely beneficial because it enables us to correctly handle system-initiated process death when possible. Properly implementing Parcelable isn't hard either because it can be generated and common types are supported. You can put URLs, primitives, Intents, collections and more inside parcelables, so most plugins will be able to properly implement it without any issue and doing so is not a burden. You don't need to put InputStreams inside an App since the download happens inside download, not get. Devices with low battery or weaker hardware will kill processes more aggressively, so system-initiated process death is not an edge-case. Every app is expected to handle it and Manager is no exception.

One very easy approach to satisfy the Parcelable requirement is to simply check if the requested package is available inside get and if it is, return App(packageName, version), otherwise return null. You can then find the file you need to download based on the packageName and version from the App. This approach works regardless of whatever non-parcelable types you need to work with in the download and get blocks. The Parcelable requirement is not a problem for webView plugins since the relevant types can be parcelized.

Other

Basically an implementation of App must be App semantically. Lets assume our strategy returns an URL. An URL is not an App so creating an implementation of the upper bound "App" via URL would violate Liskov, as URL is not an App. This problem is visible for example by not being able to implement the package name property of App, because we only have an URL.

Fair enough, although its not an unsolvable problem. Lets assume the URL does not contain the package name and version. When searching for apps, Manager provides a package name and optionally a version. The plugin can temporarily trust that the URL matches the expected package name and version until it is time to download it. The plugin can verify that the APK has the correct version and package name after the download is complete. We can provide a utility function for it. This requires making the version nullable inside App but that is definitely possible. It would also be possible to implement this validation inside Manager instead of leaving it to the plugin. I am open to suggestions here if you have alternative solutions.

The play store plugin shows a real world use of Context.

How is it using Context?

The plugin uses Context to bind to a service in the plugin APK. This service provides credentials for the play store API and is only accessible by manager thanks to custom permissions. The user will be asked to login using requestUserInteraction() if there are no credentials available.

oSumAtrIX commented 2 months ago

and their absence makes the DSL simpler

These APIs for which we don't introduce concise domain-specific APIs now poison the DSL. So instead of a clean controlled API specific to the domain, now the plugin is exposed to so many more APIs that are mostly unnecessary. The domain is tainted.

The problem of implementing a downloader is not specific enough to be solved purely through a DSL

I do not think so. A downloader simply gets the task of downloading an APK file and providing the final download stream back to ReVanced Manager. This requirement can be translated to a DSL. If there are additional requirements, I will need to know them. A cache for example was introduced as a requirement after the proposal was made which was referring to requirements prior to the cache so it is important to specify the requirements and then the DSL for that. I still do not understand the need of activities for downloaders, but apart from that the current proposal neatly fits the requirements.

The get DSL I am suggesting does not constrain plugins with strategies and is still flexible enough to support desirable extensions like webView.

The strategies are extension functions that call the public API get. Get is part of the DSL so I do not know what you mean here. The strategies were added because they are expected to be used for pretty much every downloader we are currently imagining and in the future, if other strategies seem to be common between different downloaders, a new extension can be added.

The HttpResponse is not valid outside execute. You cannot properly return an InputStream with that API. If I am wrong, send a code snippet that uses Ktor to properly return an InputStream.

Luckily this is just a design. I can keep the instance alive while I leak the stream out of the scope. APIs like URL provide a stream without a scope. Regardless, the point remains that HTTP libraries primarily support opening streams for bodies. Streams are the least abstract form of a file. It's simply a stream of bytes. The file API on the other hand is very high level and abstract, created to support a handful of scenarios and is the opposite of a concise and controlled DSL.

How do you set headers with Java's URL API?

You can use the APIs of HttpURLConnection.

Downloading the file is definitely the responsibility of the plugin because only the plugin knows how the APK source works, it is not the same for everyone.

Why? In my previous comment I explained how it is the responsibility of Manager and that, even in the case of splits, a design that is unsupported by Manager but due to black-box to the impl of the plugin allowed for downloaders regardless. You retrieve the sizes of all APKs, create a custom input stream which downloads them separately each merges the files and returns the bytes of the final APK. The complexity here is high but that's natural given you are trying to fit a design somewhere that is not supported. If splits were supported, Manager would ask for a response of one or more streams and in that case, as you can see the complexity fades away. Still want to cram unsupported design into the system, then you are met with complexity like in this case. Either way, Manager downloaded the file, just as expected.

We need an API that can guarantee no plugin is forced to deal with incorrect UI indicators

So, we are assuming every downloader implementation works under the design of ReVanced Manager. If they do, they will always ever download a single APK file. Which means they can pass the stream to Manager. In my previous message I have asked you to provide an example in line with Manages current design which still asks for an download API for plugins so they can download themselves. Unless you got a real example like that, every scenario I can imagine leads to the current proposed design. I motivated the reason with abstraction in my previous comment as well.

Our opinions on design or what is and isn't "supported" should not be an obstacle to implementing plugins or achieving proper UX.

If you design a car and you try to provide means to fit an elephant, you did not design a proper car. If you design a car with the expectancy to fit people in there, everything works out as intended. ReVanced Manager is an application to consume and patch an APK file. The downloader is a component to download such an APK file for consumption. Therefore the downloader, by design! expects an APK file from downloaders. Now downloaders can do whatever they want, but the requirement is clear. Manager wants an APK. Your downloader downloads splits? Well, that's your problem you will have to solve in your own black-box. Manager can not justify an incorrectly designed car (download API) just because a downloader tries to fit an elephant in it (download splits).

Why do we need InputStream when OutputStream can be used instead

Very simple. If Manager asks for an Output stream, each downloader will have to implement the code that writes to the stream. As you can see we violate proper OOP principles as a common task (writing to an output stream) should be moved outside of the API. With an input stream, each downloader does the mere necessary which is providing the stream. That done, and Manager can implement read() for every downloader. Notice how none of the downloaders need to implement the same code anymore and instead Manager can do read() for all?

An OutputStream-based API would be easier to work with in situations where the libraries don't have or use InputStream

I explained earlier, that libraries that only support File only automatically are unsuitable APIs. The domain clearly does not require File for downloading (even caching only needs streams). Thats like invoking .bat files to move a file from a to b in Java. It simply isn't the right tool for the current situation. The domain is "Download an APK file". Downloading implies opening a socket. Lets abstract and open a HTTP connection. Now we can use that connection to forward a stream back to Manager. Like explained earlier, every API that is dealing with similar domains has the means to open streams. May it be Ktor or Java URL or any other HTTP library for example. It would be very weird, if a HTTP library would only support Files because the domain of HTTP libraries themselves doesn't ask for Files. Why would a library that works around the HTTP protocol suddenly need file system APIs. If at all, such matter is handled as helper functions, extensions or similar, as means to simplify writing to a file which is a common task, so the user doesn't have to do the chore "File().write(response.stream)" all the time.

It would also make cleanup easier since you wouldn't have to make an object : InputStream and wrap an existing stream or something just to override close.

Creating a custom input stream is only something I suggested because you tried to download splits, an unsupported system. If you try to fit an elephant into a car, you will be met with issues and constrains, in this case "object : InputStream". On the other hand, downloaders which do follow the design and as intended fit humans into cars, neatly open an input stream to ann HTTP connection, or whatever similar and pass it down to manager.

requirement doesn't prevent you from doing anything that would otherwise have been possible without it.

Maybe, but it makes it ugly to work with. You were criticizing input stream, albeit motivated by improper design, so this should make sense to you.

The user does not know anything about system-initiated process death, which means they don't expect any state loss.

That is correct, which is why the parcelable constraint would confuse them. They don't know the reason it exists. Besides that, they don't and shouldn't expect state loss. Manager should go back to the last recoverable state which as explained in my previous comment would be simply restarting the downloader, so the download is always restarted or continued if not killed and thus state loss doesn't exist or at least isn't a problem during the lifetime of the downloader.

Introducing UX issues is unnecessary and having Parcelable is definitely beneficial because it enables us to correctly handle system-initiated process death when possible.

Is it not possible to make the annotation optional? For example, if it is parcelable, ou can reset to the last usable state, but if it is not annotated, you restart the downloader. This would be a good compromise.

You don't need to put InputStreams inside an App since the download happens inside download, not get

Creating an input stream doesn't change that. You don't start to read off the stream in get, that is done at first in download/by manager. Imagine the scenario, you query a download page. The download page expires in a short time. So you need to open a stream quickly, before the time expires. You keep the stream alive until manager finally downloads it. Quite the exaggerated scenario but something to consider. The point is that, Parcelable restricts the developer. It makes the API slightly less intuitive, even if minimal. If I were to implement a downloader for example, I would have to accept this constraint without understanding why it exists. It would be annoying for me, and I can expect the same to happen to many other people that are comfortable with the easy DSL but are put off due to the random occurrence of an Android specific limitation that has nothing to do with the downloader itself.

The plugin can temporarily trust that the URL matches the expected package name and version until it is time to download it. The plugin can verify that the APK has the correct version and package name after the download is complete. We can provide a utility function for it. This requires making the version nullable inside App but that is definitely possible. It would also be possible to implement this validation inside Manager instead of leaving it to the plugin. I am open to suggestions here if you have alternative solutions.

My point was that this verification is not really necessary. You are trying to cover a very unlikely edge-case scenario which is the downloader not downloading the correct APK. In some cases a plugin can't even provide the means to verify. Say, the plugin uses the API strategy, so it asks an API to download the app, the API returns the URL and that's it. Besides, if the plugin API is supposed to provide the package name and version requested to download, it could also simply compare it with what manager passed in get so once again the fields in App don't make sense. Any as the upper bound and treating what get returns as a black box and pas that to download on the other hand is much simpler and doesn't constrain the plugin dev to any upper bound which can happen to be unsuitable quite quickly such as in the case of the API strategy for example.

The plugin uses Context to bind to a service in the plugin APK. This service provides credentials for the play store API and is only accessible by manager thanks to custom permissions. The user will be asked to login using requestUserInteraction() if there are no credentials available.

So to generalize, the entire reason why Context is provided is so that plugins can display UIs. Why not instead simply provide an API prompt(view) where the plugin can prompt the user for input? This DSL would get rid of the need of Context poisoning the DSL by introducing a special API that fits the need of the domain and simply uses Context there.

oSumAtrIX commented 2 months ago

After a discussion some conclusions are: