Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch Export Settings to Activity Result API #8740

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

marcRDZ
Copy link
Contributor

@marcRDZ marcRDZ commented Jan 13, 2025

Change related to #7758

Migrate create document result to new API on settings export

@@ -195,7 +193,6 @@ class SettingsExportViewModel(

companion object {
private const val MIN_PROGRESS_DURATION = 1000L
private const val SETTINGS_MIME_TYPE = "application/octet-stream"
Copy link
Member

Choose a reason for hiding this comment

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

Why did you move this to the fragment? Deciding which MIME type to use is not something that should be done in view code.

Besides that, I also don't see how this is related to switching to the activity result API. Please try to keep pull requests as focused as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh well, the reason was that the usage of wildcards on ActivityResultContracts.CreateDocument() is deprecated so you have to specify MIME Type on constructor and you cannot use registerForActivityResult at runtime. My options were make that constant public or move it to the place of usage instead of passing by action events. I'll revert it and use the first option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

Choose a reason for hiding this comment

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

I see. This feels like a design bug in the CreateDocument() API. We could fix that by using our own implementation:

class CreateDocument : ActivityResultContract<CreateDocument.Arguments, Uri?>() {
    override fun createIntent(context: Context, input: Arguments): Intent {
        return Intent(Intent.ACTION_CREATE_DOCUMENT)
            .setType(input.mimeType)
            .putExtra(Intent.EXTRA_TITLE, input.title)
    }

    override fun getSynchronousResult(context: Context, input: Arguments): SynchronousResult<Uri?>? = null

    override fun parseResult(resultCode: Int, intent: Intent?): Uri? {
        return intent.takeIf { resultCode == Activity.RESULT_OK }?.data
    }

    data class Arguments(
        val title: String,
        val mimeType: String,
    )
}

Alternatively, expose the MIME type via a SettingsExportViewModel property. We don't want to introduce tight coupling between the fragment and the view model. So when changing SettingsExportFragment, pretend SettingsExportViewModel was an interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh you're right, I should expose through a property and not by changing visibility. Anyway the first option seems to me suitable for future cases, isn't it?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think the first option would be better. It does raise the question of where to put the code, though. I'd say in the package app.k9mail.core.android.common.activity.

@wmontwe: Are we fine with adding a dependency on androidx.activity to :core:android:common?

@marcRDZ marcRDZ force-pushed the task-7758_create-doc-result branch from e9d6a0a to e5b0a81 Compare January 13, 2025 14:11
@marcRDZ marcRDZ force-pushed the task-7758_create-doc-result branch from e5b0a81 to dff82bf Compare January 13, 2025 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants