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

VMDButton.setAction() execution overhead #33

Closed
antoinelamy opened this issue Feb 21, 2022 · 2 comments · Fixed by #37
Closed

VMDButton.setAction() execution overhead #33

antoinelamy opened this issue Feb 21, 2022 · 2 comments · Fixed by #37
Labels
viewmodels-declarative Issue related to Trikot.viewmodels-declarative subproject

Comments

@antoinelamy
Copy link
Contributor

Because setAction perform a subscribe on a publisher, setting an action on a large number of items becomes an expensive operation. If possible, refactor this so there is no unnecessary subscription for buttons that are not tapped.

fun setAction(action: () -> Unit) {
    actionPublisher
        .observeOn(VMDDispatchQueues.uiQueue)
        .subscribe(
            cancellableManager,
            onNext = {
                action()
            }
        )
}
@antoinelamy antoinelamy added the viewmodels-declarative Issue related to Trikot.viewmodels-declarative subproject label Feb 21, 2022
@marcantoinefortier
Copy link
Contributor

marcantoinefortier commented Feb 21, 2022

I would even go for removing entirely the publisher around viewModel's action. We should only expose a single action block to be called from the UI, and in that block we can do asynchronous work if needed.

interface ViewModel {
    val action: Publisher<ViewModelAction>
}

val myViewModel = object : ViewModel {
    override val action = myPublisher.map {
        ViewModelAction {
            // do work
        }
    }
}

interface ViewModel {
    val action: ViewModelActionBlock
}

val myViewModel = object : ViewModel {
    override val action = {
        myPublisher.first().subscribe(...) {
            // do work
        }
    }
}

@antoinelamy
Copy link
Contributor Author

@marcantoinefortier yeah that's exactly the solution I was aiming to. 👍🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
viewmodels-declarative Issue related to Trikot.viewmodels-declarative subproject
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants