Skip to content

Fix scenario copy cancellation#847

Open
vibhor1102 wants to merge 1 commit into
Nain57:masterfrom
vibhor1102:codex/fix-scenario-copy-cancellation
Open

Fix scenario copy cancellation#847
vibhor1102 wants to merge 1 commit into
Nain57:masterfrom
vibhor1102:codex/fix-scenario-copy-cancellation

Conversation

@vibhor1102
Copy link
Copy Markdown

Summary

  • keep the copy scenario dialog open while validating the new scenario name
  • start the copy only after validation succeeds
  • run the copy work in a non-cancellable context so dismissing the dialog does not cancel the repository copy mid-flight

Why

The dialog previously used the default setPositiveButton behavior, so pressing OK dismissed the dialog immediately. Since the copy coroutine is owned by the dialog view model, that could cancel the copy work before it completed.

Validation

  • :smartautoclicker:compileFDroidDebugKotlin

@Nain57
Copy link
Copy Markdown
Owner

Nain57 commented May 27, 2026

Hi and thank you for your contribution.
Thank mostly a good fix, I have one remark regarding the threading. I have proposed something that seems cleaner.

Let me know if its ok with you. I can also do the changes myself if needed :)

withContext(NonCancellable) {
if (isSmart) smartRepository.addScenarioCopy(scenarioId, name)
else dumbRepository.addDumbScenarioCopy(scenarioId, name)
}
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

While this works, i don't think that's the proper way to fix this.
A better approach would be to delegate the threading responsability to the smart & dumb repositories, allowing to tie the copy to the app lifecycle.

So basically, by adding the following parameter to both repositories:

@Singleton
class DumbRepository @Inject constructor(
    @Dispatcher(IO) ioDispatcher: CoroutineDispatcher,
    private val dumbScenarioDataSource: DumbScenarioDataSource,
) : IDumbRepository {

    private val coroutineScopeIo: CoroutineScope =
        CoroutineScope(SupervisorJob() + ioDispatcher)

    fun addDumbScenarioCopy(scenarioDbId: Long, copyName: String, onCopyCompleted: () -> Unit): Long? {
        coroutineScopeIo.launch {
            dumbScenarioDao.getDumbScenariosWithAction(scenarioDbId)?.let { scenarioWithActions ->
                addDumbScenarioCopy(scenarioWithActions, copyName)
                onCopyCompleted()
            }
        }
    }
}

@vibhor1102 vibhor1102 force-pushed the codex/fix-scenario-copy-cancellation branch from f7daf69 to 1196b75 Compare May 28, 2026 07:53
@vibhor1102
Copy link
Copy Markdown
Author

Thanks for the review. I pushed an update that follows your suggested direction: the scenario copy work is now launched from repository-owned IO scopes for both smart and dumb scenarios, and the ViewModel only requests the copy and handles completion back on the main dispatcher.

I have not performed device/build testing after this amendment.

I am also completely fine with either this amended approach or with you making further changes directly to the PR if you prefer a slightly different implementation style.

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