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

Androidx activityResult refactor #454

Merged
merged 8 commits into from
Jan 10, 2024
Merged

Conversation

IanCrossCD
Copy link
Contributor

Description:
Migrates FaceTrackingManager Camera Permissions to Androidx activityResult. Creates CameraPermissionHelper

Copy link
Collaborator

@PaulKlauser PaulKlauser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the long lead-time here. Overall I like the changes, but I'm wondering if there's a way to simplify the ping-ponging between permissions callbacks and make that read more synchronously. There may not be, but worth poking at imo

app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
@mattttvaughn mattttvaughn force-pushed the androidx-activityresult-refactor branch from 57130da to 870f6c0 Compare December 29, 2023 15:15
@mattttvaughn mattttvaughn force-pushed the androidx-activityresult-refactor branch from 870f6c0 to 7b3f2cc Compare December 29, 2023 15:20
Comment on lines 48 to 55
activity.registerForActivityResult(ActivityResultContracts.RequestPermission()) { isGranted: Boolean ->
if (isGranted) {
enableFaceTracking()
} else {
disableFaceTracking()
permissionSettingsDialog.show()
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all the new stuff in this file is just moved wholesale from CameraPermissionsHelper. The only intended changes are to directly use enableFaceTracking() or disableFaceTracking() instead of calling them via callbacks

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since enable/disable are now called internally, do they still need to be part of the public interface here?

This then feeds into how this class is being tested, which I think is going to inform whether or not your mocking below is sufficient. My gut says it's not, since you're now unable to test the various branching points at line 49

Comment on lines 30 to 33
return FaceTrackingPermissions(
sharedPreferences = sharedPreferences,
activity = mockk(relaxed = true)
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

holler if there's a better approach since I know relaxed mocks are probably a code smell, but an activity is required in this class now and doing interface shenanigans didn't feel worth the time/complexity to me

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can these Fakes go in the test directory?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved!

@mattttvaughn mattttvaughn merged commit 9696316 into main Jan 10, 2024
4 checks passed
@mattttvaughn mattttvaughn deleted the androidx-activityresult-refactor branch January 10, 2024 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants