Set as Ringtone Dialog Compose#254
Open
TheTerminatorOfProgramming wants to merge 1 commit intomardous:masterfrom
Open
Set as Ringtone Dialog Compose#254TheTerminatorOfProgramming wants to merge 1 commit intomardous:masterfrom
TheTerminatorOfProgramming wants to merge 1 commit intomardous:masterfrom
Conversation
Convert the Set as Ringtone Dialog to Compose
Contributor
Reviewer's GuideConverts the legacy XML-based "Set as Ringtone" dialog into a Compose-driven bottom sheet presented via a navigation dialog destination, including a separate permission bottom sheet and wiring it into the existing song menu flow. Sequence diagram for the new set-as-ringtone Compose bottom sheet flowsequenceDiagram
actor User
participant SongMenu
participant HostingFragment
participant NavController
participant RingtoneFragment
participant SystemSettings
participant RingtoneBottomSheet
participant RingtonePermissionBottomSheet
User->>SongMenu: select action_set_as_ringtone
SongMenu->>HostingFragment: onSongMenu(this)
HostingFragment->>NavController: navigate(nav_ringtone, songDetailArgs(song))
NavController->>RingtoneFragment: create with extraSong
RingtoneFragment->>RingtoneFragment: onCreateView(inflater, container, savedInstanceState)
RingtoneFragment->>SystemSettings: canWrite(context)
alt write settings granted
RingtoneFragment->>RingtoneBottomSheet: show for song
User->>RingtoneBottomSheet: toggle use_also_as_alarm_alert
User->>RingtoneBottomSheet: tap action_set_as_ringtone
RingtoneBottomSheet->>song: configureRingtone(context, useAlsoAsAlarm)
else write settings denied
RingtoneFragment->>RingtonePermissionBottomSheet: show for song
User->>RingtonePermissionBottomSheet: tap action_grant
RingtonePermissionBottomSheet->>SystemSettings: startActivity(ACTION_MANAGE_WRITE_SETTINGS)
end
Updated class diagram for the new RingtoneFragment and related typesclassDiagram
class BottomSheetDialogFragment
class RingtoneFragmentArgs
class Song
class RingtoneFragment {
- navArgs: RingtoneFragmentArgs
- song: Song
+ onCreateView(inflater: LayoutInflater, container: ViewGroup, savedInstanceState: Bundle): View
}
BottomSheetDialogFragment <|-- RingtoneFragment
RingtoneFragmentArgs --> Song
RingtoneFragment --> Song
Flow diagram for navigation to the new ringtone Compose bottom sheetflowchart TD
A["Song.onSongMenu handles action_set_as_ringtone"] --> B["HostingFragment.findActivityNavController(R.id.fragment_container)"]
B --> C["NavController.navigate(R.id.nav_ringtone, songDetailArgs(song))"]
C --> D["Nav_graph dialog destination nav_ringtone"]
D --> E["RingtoneFragment receives extraSong argument"]
E --> F{"Settings.System.canWrite(context)?"}
F -->|Yes| G["Show RingtoneBottomSheet(song)"]
F -->|No| H["Show RingtonePermissionBottomSheet(song)"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- Both
RingtoneBottomSheetandRingtonePermissionBottomSheetacceptsong: Song?but immediately usesong!!in string resources; consider makingsongnon-nullable in the composable API and enforcing that at the call sites to avoid potential NPEs. - The
Settings.System.canWrite(requireContext())check is done directly in the composablesetContentblock; consider hoisting this into a value (e.g., viarememberor computed before composition) to avoid calling the platform API during recomposition and to make the logic easier to test. - Using
R.dimen.shuffle_heightas the bottom sheetpeekHeight/maxHeightfor the ringtone dialog couples it to an unrelated dimension; consider introducing a dedicated ringtone bottom sheet height dimension to make the layout intent clearer and easier to maintain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Both `RingtoneBottomSheet` and `RingtonePermissionBottomSheet` accept `song: Song?` but immediately use `song!!` in string resources; consider making `song` non-nullable in the composable API and enforcing that at the call sites to avoid potential NPEs.
- The `Settings.System.canWrite(requireContext())` check is done directly in the composable `setContent` block; consider hoisting this into a value (e.g., via `remember` or computed before composition) to avoid calling the platform API during recomposition and to make the logic easier to test.
- Using `R.dimen.shuffle_height` as the bottom sheet `peekHeight`/`maxHeight` for the ringtone dialog couples it to an unrelated dimension; consider introducing a dedicated ringtone bottom sheet height dimension to make the layout intent clearer and easier to maintain.
## Individual Comments
### Comment 1
<location> `app/src/main/java/com/mardous/booming/ui/screen/ringtone/RingtoneScreen.kt:40-49` </location>
<code_context>
+@OptIn(ExperimentalMaterial3Api::class)
+@Composable
+fun RingtonePermissionBottomSheet(
+ song: Song?
+) {
+ val context = LocalContext.current
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid nullable `Song` with `song!!` and make the parameter non-nullable if it's required.
Here `song` is declared as `Song?` but immediately used as `song!!`. Since navigation always provides a non-null `Song`, change the parameter to `song: Song` and remove the `!!` to avoid potential crashes if this composable is ever called with null in the future.
</issue_to_address>
### Comment 2
<location> `app/src/main/java/com/mardous/booming/ui/screen/ringtone/RingtonePermissionScreen.kt:38-47` </location>
<code_context>
+fun RingtonePermissionBottomSheet(
</code_context>
<issue_to_address>
**issue (bug_risk):** Align `RingtonePermissionBottomSheet` parameter nullability with actual usage instead of using `song!!`.
This composable always needs a `Song` (for `song.title`), so making the parameter non-nullable avoids potential NPEs and keeps the API consistent with how the fragment passes the argument.
</issue_to_address>
### Comment 3
<location> `app/src/main/java/com/mardous/booming/ui/screen/ringtone/RingtoneFragment.kt:31-36` </location>
<code_context>
+ ): View {
+ dialog?.setOnShowListener { dialogInterface ->
+ val bottomSheetDialog = dialogInterface as BottomSheetDialog
+ bottomSheetDialog.behavior.apply {
+ isFitToContents = true
+ skipCollapsed = true
+ peekHeight = dip(R.dimen.shuffle_height)
+ //isDraggable = false
+ maxHeight = peekHeight
+ }
+ }
</code_context>
<issue_to_address>
**suggestion:** Using a large fixed `peekHeight`/`maxHeight` may misbehave on small or rotated screens.
With `isFitToContents = true` and `skipCollapsed = true`, hard-coding both `peekHeight` and `maxHeight` to `dip(R.dimen.shuffle_height)` (800dp) can exceed the available height on some devices, causing layout problems. Consider using `PEEK_HEIGHT_AUTO`, deriving the height from the window size, or clamping this dimension to the screen height at runtime.
Suggested implementation:
```
dialog?.setOnShowListener { dialogInterface ->
val bottomSheetDialog = dialogInterface as BottomSheetDialog
bottomSheetDialog.behavior.apply {
isFitToContents = true
skipCollapsed = true
val desiredPeekHeight = dip(R.dimen.shuffle_height)
val windowHeight = resources.displayMetrics.heightPixels
val clampedPeekHeight = kotlin.math.min(desiredPeekHeight, windowHeight)
peekHeight = clampedPeekHeight
//isDraggable = false
maxHeight = clampedPeekHeight
}
}
```
If your project prefers imports over fully-qualified calls, add:
`import kotlin.math.min`
at the top of `RingtoneFragment.kt` and replace `kotlin.math.min` with `min` in the body.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+40
to
+49
| song: Song? | ||
| ) { | ||
| var checkedState by remember { mutableStateOf(false) } | ||
| val context = LocalContext.current | ||
|
|
||
| BottomSheetDialogSurface { | ||
| Column( | ||
| modifier = Modifier | ||
| .fillMaxWidth() | ||
| .wrapContentHeight() |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Avoid nullable Song with song!! and make the parameter non-nullable if it's required.
Here song is declared as Song? but immediately used as song!!. Since navigation always provides a non-null Song, change the parameter to song: Song and remove the !! to avoid potential crashes if this composable is ever called with null in the future.
Comment on lines
+38
to
+47
| fun RingtonePermissionBottomSheet( | ||
| song: Song? | ||
| ) { | ||
| val context = LocalContext.current | ||
|
|
||
| BottomSheetDialogSurface { | ||
| Column( | ||
| modifier = Modifier | ||
| .fillMaxWidth() | ||
| .wrapContentHeight() |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Align RingtonePermissionBottomSheet parameter nullability with actual usage instead of using song!!.
This composable always needs a Song (for song.title), so making the parameter non-nullable avoids potential NPEs and keeps the API consistent with how the fragment passes the argument.
Comment on lines
+31
to
+36
| bottomSheetDialog.behavior.apply { | ||
| isFitToContents = true | ||
| skipCollapsed = true | ||
| peekHeight = dip(R.dimen.shuffle_height) | ||
| //isDraggable = false | ||
| maxHeight = peekHeight |
Contributor
There was a problem hiding this comment.
suggestion: Using a large fixed peekHeight/maxHeight may misbehave on small or rotated screens.
With isFitToContents = true and skipCollapsed = true, hard-coding both peekHeight and maxHeight to dip(R.dimen.shuffle_height) (800dp) can exceed the available height on some devices, causing layout problems. Consider using PEEK_HEIGHT_AUTO, deriving the height from the window size, or clamping this dimension to the screen height at runtime.
Suggested implementation:
dialog?.setOnShowListener { dialogInterface ->
val bottomSheetDialog = dialogInterface as BottomSheetDialog
bottomSheetDialog.behavior.apply {
isFitToContents = true
skipCollapsed = true
val desiredPeekHeight = dip(R.dimen.shuffle_height)
val windowHeight = resources.displayMetrics.heightPixels
val clampedPeekHeight = kotlin.math.min(desiredPeekHeight, windowHeight)
peekHeight = clampedPeekHeight
//isDraggable = false
maxHeight = clampedPeekHeight
}
}
If your project prefers imports over fully-qualified calls, add:
import kotlin.math.min
at the top of RingtoneFragment.kt and replace kotlin.math.min with min in the body.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Convert the Set as Ringtone Dialog to Compose
Summary by Sourcery
Convert the legacy "set as ringtone" dialog to a Compose-based bottom sheet flow with navigation support.
New Features:
Enhancements: