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

Mute Audio Default Setting #268

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Conversation

Kimblebee
Copy link
Collaborator

@Kimblebee Kimblebee commented Sep 30, 2024

Persistent setting to start video recordings muted or with audio.

notable changes:

  • injectable PermissionChecker helper class. It has access to application context for the sole purpose of verifying that a permission has been granted.
  • The permission to record audio is used as a constraint for the Mute Audio setting -- It will be disabled if the permission is not granted
  • Same process of adding a new setting...

@temcguir
Copy link
Collaborator

temcguir commented Oct 8, 2024

In the currently used version of CameraX, you can now start a recording muted with PendingRecording.withAudioEnabled(initialMuted = true). Maybe you can add that to this PR?

https://developer.android.com/reference/androidx/camera/video/PendingRecording#withAudioEnabled(kotlin.Boolean)

.idea/androidTestResultsUserPreferences.xml Outdated Show resolved Hide resolved
.idea/inspectionProfiles/Project_Default.xml Outdated Show resolved Hide resolved
.idea/other.xml Outdated Show resolved Hide resolved
gradle/libs.versions.toml Outdated Show resolved Hide resolved

/**
* [ViewModel] for [SettingsScreen].
*/
@HiltViewModel
class SettingsViewModel @Inject constructor(
private val settingsRepository: SettingsRepository,
private val permissionChecker: PermissionChecker,
Copy link
Collaborator

Choose a reason for hiding this comment

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

So I tried to research this a little, and from what I can tell, the permissions are supposed to be fully owned by the UI layer. We should treat them as if they are user interaction events, like pressing a button. With that in mind, we should check permissions in the UI (Activity or Screen level) and send them down to the ViewModel, where the ViewModel can generate the UiState that depends on them.

So the SettingsViewModel should have something like:

class SettingsViewModel {
    fun setAudioPermissionGranted(granted: Boolean)
}

or if you need multiple permissions to be communicated to the ViewModel:

class SettingsViewModel {
    fun setGrantedPermissions(permissions: Set<String>)
}

This will align with the guidance that:
image

whereas currently, this code is storing a Context (albeit an Application context) in the ViewModel, which violates this guidance.

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