-
Notifications
You must be signed in to change notification settings - Fork 268
New Sample for ActivityResultContracts.RequestPermission() #8
Conversation
@@ -0,0 +1,8 @@ | |||
## This file must *NOT* be checked into Version Control Systems, |
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.
Please don't check this file into source control. :)
*/ | ||
class MainActivity : AppCompatActivity(), ActivityCompat.OnRequestPermissionsResultCallback { | ||
|
||
private lateinit var layout: 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.
Please convert the sample to use view binding.
} | ||
|
||
companion object { | ||
private val TAG = "CameraPreview" |
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 should be const
(and you could consider moving it to be a top level constant at the top of the file).
* Implementation is based directly on the documentation at | ||
* http://developer.android.com/guide/topics/media/camera.html | ||
*/ | ||
class CameraPreview @JvmOverloads constructor( |
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's probably no reason for the @JvmOverloads
here, since this class isn't being consumed by any Java programming language callers.
// Create the Preview view and set it as the content of this Activity. | ||
val cameraPreview = CameraPreview(this, null, | ||
0, camera, cameraInfo, displayRotation) | ||
findViewById<FrameLayout>(R.id.camera_preview).addView(cameraPreview) |
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.
Please convert the class to use view binding.
* A safe way to get an instance of the Camera object. | ||
*/ | ||
private fun getCameraInstance(cameraId: Int): Camera? { | ||
var c: Camera? = null |
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 use a better name than c
for the variable.
<manifest xmlns:android="http://schemas.android.com/apk/res/android" | ||
package="com.example.android.basicpermissions"> | ||
|
||
<!-- BEGIN_INCLUDE(manifest) --> |
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.
Consider removing this comment (along with the END_INCLUDE
one below)
status: PUBLISHED | ||
technologies: [Android] | ||
categories: [Permissions] | ||
languages: [Java] |
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.
Sorry, I forgot to note this: Please change this to [Kotlin]
:)
Thanks for the contribution! I know a bunch of the comments actually also apply to the existing Kotlin sample, but it would be nice to improve this one not only to use the activity result library but also other improvements :) |
Hey @nic0lette thanks for the review. One question, since this camera API is even the new one. If yes, I will remove and address the other comments |
I'm not sure. Yes, the goal is to show the permission flow, but it's nice to also see the result of it. Let's leave the deprecated Camera API usage for now, and then update it to use CameraX in the future. (Even if you'd like to do that part, I think we should do it in a separate PR) (The name for the sample (directory) is also a bit intense? How would you feel about something like |
@nic0lette thanks for the tips. sorry for the long time until I address it. Change the folder to Thanks for pushing the view binding, I had not give the time before to check it and now I understand why is better compared with Kotlin Synthetic Just add the new code, let me know anything! |
Hey @codingjeremy sorry to disturb, not sure the best way of helping here. First time committing in the Android Repo, something I could do or need to do to help this PR? |
@tiembo @nic0lette @codingjeremy I add the changes I saw in #11 let me know if something is off ^^ |
Hey people any news? =D Sorry to disturb @tiembo @codingjeremy |
Sent from Yahoo Mail on Android
On Sel, Nov 17, 2020 at 5:38 PTG, Victor Canato<notifications@github.com> wrote:
Hey people any news? =D
Sorry to disturb @tiembo @codingjeremy
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
Merge pull request #1 from android/master
Added latest changes into my branch/PR |
Good day and almost merry Christmas Any news here? |
Good morning and happy new year =D Let me know if need more changes or we are waiting for the final release, |
Thanks @themar7777 ! Just to let you know I cannot merge 😭
Hahaha so feel free to merge it when you can =) |
Hey @tiembo any news for this PR? =D |
Thank you for the sample. :) |
close #7
Description
appCompat
version1.3.0-alpha02
androidx
librariesCreate to give a previous sample using the code from Request App Permissions
Obs:
MainActivity.kt
if compare with the other kotlin module.1.3
stable.Open for folder name suggestion and/or any other code suggestion
Disclaimer
Already sign the license as noted on CONTRIBUTING.md. Waiting for answer