Skip to content
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

Merged
merged 3 commits into from
Apr 8, 2021
Merged

Reduce amount of code #443

merged 3 commits into from
Apr 8, 2021

Conversation

GeorgCantor
Copy link
Contributor

Reduced the amount of code using Kotlin Synthetic

@nic0lette
Copy link
Contributor

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.

@GeorgCantor
Copy link
Contributor Author

Ok

Copy link
Contributor

@nic0lette nic0lette left a 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)
Copy link
Contributor

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) }
Copy link
Contributor

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
Copy link
Contributor

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?

Copy link
Contributor

@nic0lette nic0lette left a 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!

@nic0lette nic0lette merged commit 99e44c1 into android:main Apr 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants