I'm still coming up to speed, so I'll take general...

# tornadofx

[carlw]

I'm still coming up to speed, so I'll take general Kotlin and TornadoFX comments. There are a few ways to reorganize this -- like pushing the login code into an event hander -- but I think they'll all need a lot of nesting.

What I hope to see with coroutines is a code organization closer to the 4-step pseudo code where what is asynchronous and what is on the FX thread isn't noticable.

/end

[elizarov]

That is how I was able to simplify your code using coroutines without adding any new utility functions to TornadoFx: https://gist.github.com/elizarov/926456a9774b8e12b098f05be390634b

The basic idea, is that you convert all the long-running rest-using functions (preLoginInit, login and postLoginInit) into suspending functions using run(CommonPool) { … } idiom, so that they don’t block the thread they are invoked from.

Now, I’ve extracted all your login logic into a separate function named launchLogin. It uses launch(JavaFx) {…} to start a coroutine in the JavaFx application thread and all the logic can be expressed sequentially inside. Every call to a suspending function will suspend it, making sure that the JavaFx app thread is not blocked. All the suspending invocations are nicely marked by the IDEA on the left-hand side.

The benefit is that you can now centralize all the error handling, by using a regular try { … } catch { … }.

You don’t need to do anything special about getLoginInfo, because it uses Dialog.showAndWait which does not block the JavaFx app thread (it has its own event loop inside).

It would be way more convenient, if tornadoxfx.Rest turns all its blocking functions into suspending functions (the same run(pool) { … ] idiom can be used), so that you never have to worry about accidentally blocking your UI by invoking some blocking function on Rest object.

[edvin]

Nice! If we modify tornadofx.Rest to always return suspending functions, will this have any negative impact if it's called outside of a launch block?

[edvin]

This means that we would introduce an explicit dependency to the couroutines lib as well, so this would need to be in Maven central or else everybody would need to update their build scripts. Is it just in bintray for now?

[elizarov]

If all the blocking functions in Rest are turned into suspend fun then you will not be able to accidentally use them from UI thread at all. The compiler will not let you just call them, which is exactly what you want. You will have to always invoke them from inside of launch or from other kind of coroutine.

[elizarov]

coroutines library is in bintray jcenter now. We can push it to maven central, too, if you think it is imporant.

[ron]

if we make the coroutines a dependency on tornadoFX pom then it will have no impact on the user of tornadoFX

[edvin]

@elizarov Oh, that's actually an added bonus then. Yes, it's imperative for us that it is in Maven Central, or builds would break when people upgrade to the TornadoFX version that includes coroutines. However, if we make this change, our runAsync must use launch, or all existing code would fail. The issue then becomes that runAsync operates on a Task, so removing this would break compatibility. If we can still utilize a Task for this however, this would be a seamless migration.

[elizarov]

Changing the signature of runAsync, so that it has func: suspend FXTask<*>.() -> T) is quite straightforward. It also good opportunity to migrate from Thread(this).start() in the code of task to using a pool of threads (common pool shall be fine). However, unfortunately, it does not make it automatically fully backwards compatible, because the old code could have been doing something like:

fun foo() { 
    runAsync { bar() }
}

fun bar() { 
    api.get(”something") 
}

It will not “just work” if an “api.get” is a suspending function. The “bar” will have to be marked with “suspend” modifier, too. IDEA will helpfully provide a quick-fix for that, but still.

Probably, the best approach is to leave “Rest” as is (and maybe deprecate it?), but provide some alternative class with suspending functions that can be migrated to.

[edvin]

Ah, that's right. Maybe we could introduce a coroutines enabled Rest client as a first step. We might even tag it experimental in the beginning to get some more feedback before committing to this approach fully. If it works we could deprecate the old version or just swap out the old for the new in a major release.

[elizarov]

Makes sense.

[carlw]

This looks perfect @roman. How do I get to kotlinx.coroutines if i have a plain intellij project (no gradle or maven)? I've tried a few repository strings involving bintray.com but can't get them indexed or to resolve the package.

[elizarov]

You can download jars from bintray. There is a download link at the top of readme page that directly sends to the last version. It is on the “Files” tab: https://bintray.com/kotlin/kotlin-eap-1.1/kotlinx.coroutines/0.13#files/org/jetbrains/kotlinx

[elizarov]

You’ll need a jar from kotlinx-coroutines-core and kotlinx-coroutines-javafx modules

[carlw]

ok. thx

[ron]

maybe we should call it coRest?

[ron]

untill we remove the old one?

[edvin]

Nah, we need a sexier name.

[ron]

sexyCoRest?

[edvin]

Nice try 🙂

[Ruckus]

@elizarov

That is how I was able to simplify your code using coroutines without adding any new utility functions to TornadoFx...

Are there any utility functions you can think of off the top of your head that would make supporting coroutines easier (for this case or in general)?