New Alert builder system #53
Conversation
avdyushin
added
enhancement
| /** | ||
| * Builds an alert using DSL syntax | ||
| * | ||
| * @param initialize |
| */ | ||
| fun alert(initialize: BaseAlertBuilder.() -> Unit): AlertInterface { | ||
| reset() | ||
| initialize() |
There was a problem hiding this comment.
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.
we should probably make a test for this behaviour, regardless of what happens.
There was a problem hiding this comment.
This is good point
There was a problem hiding this comment.
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).
|
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 |
thoutbeckers
added
component:alerts
and removed
✏️architecture
Also added dispatcher for fix coroutines executaion on iOS side
| */ | ||
|
|
||
| @UseExperimental(InternalCoroutinesApi::class) | ||
| internal class NsQueueDispatcher(private val dispatchQueue: dispatch_queue_t) : CoroutineDispatcher(), Delay { |
There was a problem hiding this comment.
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.
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.
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.
Look at the Components examples, you need to wrap the launch in runBlocking for now.
There was a problem hiding this comment.
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.
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.
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.
You mean example of LocationPrinter?
There was a problem hiding this comment.
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.
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.
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.
We would also only have this ones if we put it in a good spot.
There was a problem hiding this comment.
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.
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.
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)) | ||
| } | ||
| } | ||
| } |
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.