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

Misc considerations for SplashActivity #106

Open
AllanWang opened this issue Jan 20, 2019 · 2 comments
Open

Misc considerations for SplashActivity #106

AllanWang opened this issue Jan 20, 2019 · 2 comments

Comments

@AllanWang
Copy link
Collaborator

Going along with good practices, if you have functions that should never run on the main thread, or functions that should always run from the main thread but aren't always called from a main thread, it would be better to make them suspend functions and wrap them in their expected context. This way, the caller doesn't have to worry about the coroutine context, and it will be much easier to maintain as the specifications occur only once.

For instance, showNextScreen modifies views but is sometimes called in the background, It would be nicer to make it suspended with a ui context, and use launch { showNextScreen() } in onActivityResult rather than withContext(uiDispatcher) { showNextScreen() } from the background caller.

In the case of login, you can see that it is specified to begin with the ui dispatcher, yet is called from launch(bgDispatcher) { login(true) } by showNextScreen. While this works, this is an incorrect context switch by the caller.

There are other parts, such as

// currently in bgDispatcher
if (downloadEverything) {
    try {
        userDownloader.execute()
    } catch (e: IOException) {
        // If there was an exception, stop here
        // Hide the container
        progressContainer.isVisible = false

        showLoginScreen(e)
        return@withContext
    }
} else {
    userDownloader.start()
}

withContext(uiDispatcher) {
    // Hide the container
    progressContainer.isVisible = false

    // Connection successful: home page
    openHomePage()
}

where the call within downloadEverything is not in the right context. As showLoginScreen assumes it starts in ui, I believe that will crash, as well as the progressContainer.isVisible = false before it

@AllanWang
Copy link
Collaborator Author

A few other things:

Result is not a part of coroutines. While you can't create one yourself, you can use runCatching to output one.

If you wish to keep using your own Result, then EmptySuccess can probably be an object rather than a class

@jguerinet
Copy link
Owner

I agree with all of this. I hate the current SplashActivity, it's very convoluted and does way too much in too little functions. The coroutines are a mess, they were just set up to work essentially, and are not optimized. I want to revisit this entirely, especially after refactoring the way data is downloaded.

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

No branches or pull requests

2 participants