-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
Reduce amount of code #443
Conversation
Hi @GeorgCantor ! Thanks for the PR. Since Kotlin Android Extensions are deprecated, we don't want to use Kotlin synthetics here. If you were able to migrate these to use view binding, then I'd be able to accept the PR. |
Ok |
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.
Thanks for the update! Just a few small things but otherwise looks good =D
override fun onViewCreated(view: View, savedInstanceState: Bundle?) { | ||
super.onViewCreated(view, savedInstanceState) | ||
binding = QrSignInBinding.bind(view) |
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 you make binding
a local variable then it wouldn't be nullable (so you could get rid of many of the ?
s) and you wouldn't need to clear it in onDestroy()
/** | ||
* Sets the QR code rendered on this sign in screen. | ||
* | ||
* @param code The QR code to display. | ||
*/ | ||
private fun setQrCode(url: String) { | ||
Glide.with(this).load(url).into(qr_code) | ||
binding?.qrCode?.let { Glide.with(this).load(url).into(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.
Could you just pass the view in with the url (or, perhaps even better, just move it into onViewCreated()
)?
|
||
/** | ||
* This class exposes application settings | ||
* for integration with MediaCenter in Android Automotive. | ||
*/ | ||
class SettingsActivity : AppCompatActivity() { | ||
|
||
private lateinit var binding: ActivitySettingsBinding |
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.
Does this need to be a member? It looks like everything is set in onCreate
?
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.
Thanks for the PR!
Reduced the amount of code using Kotlin Synthetic