-
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
Changes from 1 commit
5cf9d45
bff29a9
8fe2f73
afe040d
4ec7084
10df591
10b8071
9836d77
1530979
88e4c8c
15a3109
9689a53
977ba0e
0f78c8b
97c3a32
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -112,9 +112,9 @@ abstract class BaseAlertPresenter(private val alert: Alert) : AlertActions { | |
dismissAlert(animated) | ||
} | ||
|
||
protected abstract fun dismissAlert(animated: Boolean = true) | ||
internal abstract fun dismissAlert(animated: Boolean = true) | ||
|
||
protected abstract fun showAlert( | ||
internal abstract fun showAlert( | ||
animated: Boolean = true, | ||
afterHandler: (Alert.Action?) -> Unit = {}, | ||
completion: () -> Unit = {} | ||
|
@@ -189,13 +189,34 @@ abstract class BaseAlertBuilder { | |
*/ | ||
fun addActions(actions: List<Alert.Action>) = apply { this.actions.addAll(actions) } | ||
|
||
/** | ||
* Builds an alert using DSL syntax | ||
* | ||
* @param initialize | ||
* @return | ||
*/ | ||
fun alert(initialize: BaseAlertBuilder.() -> Unit): AlertInterface { | ||
reset() | ||
initialize() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Definitely |
||
return create() | ||
} | ||
|
||
/** | ||
* Adds an [action] to the alert | ||
* | ||
* @param action The action object | ||
*/ | ||
private fun addAction(action: Alert.Action) = apply { this.actions.add(action) } | ||
|
||
/** | ||
* Reset builder into initial state | ||
*/ | ||
private fun reset() = apply { | ||
thoutbeckers marked this conversation as resolved.
Show resolved
Hide resolved
|
||
this.title = null | ||
this.message = null | ||
this.actions.clear() | ||
} | ||
|
||
/** | ||
* Creates an alert based on [title], [message] and [actions] properties | ||
* | ||
|
@@ -214,7 +235,7 @@ abstract class BaseAlertBuilder { | |
* | ||
* @return The AlertInterface object | ||
*/ | ||
abstract fun create(): AlertInterface | ||
internal abstract fun create(): AlertInterface | ||
} | ||
|
||
expect class AlertBuilder : BaseAlertBuilder |
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