-
Notifications
You must be signed in to change notification settings - Fork 7
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
New Alert builder system #53
Conversation
/** | ||
* Builds an alert using DSL syntax | ||
* | ||
* @param initialize |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documentation
*/ | ||
fun alert(initialize: BaseAlertBuilder.() -> Unit): AlertInterface { | ||
reset() | ||
initialize() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pattern makes the builder non threadsafe.
That might be acceptable, but what will happen if we pop more than one dialog? Will the handlers also be reset? (seems like they are stored in actions).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should probably make a test for this behaviour, regardless of what happens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this might work now. The multithreaded testcase is hard, but the testcase for building two alerts in a row can still be added I think (unless I missed it somewhere).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely
This is ok, some small changes need and some longer term questions. The example and readme should reflect how to use it from common code, and also contain the coroutine based variant. I created #55 for that, so it does not have to be solved in this PR |
Also added dispatcher for fix coroutines executaion on iOS side
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice additions, but let's still cover some edge cases and coroutines consistent with Components
etc. instead of creating a custom implementation for iOS.
*/ | ||
|
||
@UseExperimental(InternalCoroutinesApi::class) | ||
internal class NsQueueDispatcher(private val dispatchQueue: dispatch_queue_t) : CoroutineDispatcher(), Delay { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems based on an old example. Kotlin common has classes like MainScope
and Dispatchers
that can take care of this, as per the current examples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You even use Dispatchers as actual
several times below (e.g. https://github.com/splendo/kaluga/pull/53/files#diff-0b39b48bb18d30351b45fb529d0bb606R24 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also thought that. I tried MainScope().launch
and GlobalScope.launch(Dispatchers.Main)
and expected to run them in main queue on iOS. But in all cases I've got an exception:
kotlin.IllegalStateException: There is no event loop. Use runBlocking { ... } to start one.
Looks like this one Kotlin/kotlinx.coroutines#470 is still open and not included into library, and we have to keep this workaround. Or I missed something...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look at the Components
examples, you need to wrap the launch
in runBlocking
for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we should make a convenience launch
method, because despite of the error message people are confused by this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still not clear how to use it then. We have suspend alert
to build alert and suspend show
to show. If we wrap all with runBlocking
it will never return because show is waiting for user's input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You wrap the launch
inside runBlocking
. runBlocking
will not wait for what happens inside launch
.
Did you check the example of Components?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean example of LocationPrinter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd look at all of them, also to see how common testing is done (#56), but yeah just search launch
or MainScope
and you can see the usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, in AlertFactory
in shared example we have three functions.
Last one works with runBlocking { MainScope().launch { ... }}
fine.
But this approach doesn't work if I have delay
call inside with the same event loop complains. And this doesn't work for CancelableContinuation
(when we call resume
) for the same reasons.
*/ | ||
fun alert(initialize: BaseAlertBuilder.() -> Unit): AlertInterface { | ||
reset() | ||
initialize() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this might work now. The multithreaded testcase is hard, but the testcase for building two alerts in a row can still be added I think (unless I missed it somewhere).
@@ -33,6 +29,10 @@ actual class AlertInterface( | |||
private val context: Context | |||
) : BaseAlertPresenter(alert) { | |||
|
|||
private inline fun <T> T.applyIf(condition: Boolean, block: T.() -> Unit): T = apply { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would also only have this ones if we put it in a good spot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I put it in Components, then Alerts has to bring all dependencies which we have in Components. So it's better to make for example Utils component in dependency free stuff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, Components
is from before the split into modules. It contains a bunch of utils stuff but needs to be split, see #16 You can pick it up if you want, till then I'd say it's ok for your module to depend on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to fulfil all dependencies but end up with circular one. I'd like to leave it here for now, so Alerts will be released and puck up #16 as next one
@Test | ||
fun testConcurrentBuilders() = runBlockingTest { | ||
val builder = AlertBuilder(activityRule.activity) | ||
val alerts: MutableList<AlertInterface> = mutableListOf() | ||
|
||
CoroutineScope(Dispatchers.Main).launch { | ||
for (i in 0 until 10) { | ||
alerts.add(builder.alert { | ||
setTitle("Alert$i") | ||
setPositiveButton("OK$i") | ||
}) | ||
} | ||
for (i in 0 until 10) { | ||
alerts[i].show() | ||
device.wait(Until.findObject(By.text("Alert$i")), DEFAULT_TIMEOUT) | ||
device.findObject(By.text("OK$i")).click() | ||
assertTrue(device.wait(Until.gone(By.text("Alert$i")), DEFAULT_TIMEOUT)) | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still need to resolve dispatchers, but then we're good.
Related to #29
Now you able to create builder once with Context/UIViewController and later instantiate as many alerts as needed using DSL block syntax on both platforms.