Web Search Dialog Compose#256
Open
TheTerminatorOfProgramming wants to merge 1 commit intomardous:masterfrom
Open
Web Search Dialog Compose#256TheTerminatorOfProgramming wants to merge 1 commit intomardous:masterfrom
TheTerminatorOfProgramming wants to merge 1 commit intomardous:masterfrom
Conversation
Convert the Web Search Dialog to Compose
Contributor
Reviewer's GuideReplaces the legacy WebSearchDialog with a new Jetpack Compose-based bottom sheet dialog wired through Navigation, including a new nav graph dialog destination, a Compose screen and fragment wrapper, and an updated player action to launch it. Sequence diagram for launching the new Compose web search bottom sheetsequenceDiagram
actor User
participant AbsPlayerFragment
participant NavController
participant WebSearchFragment
participant ComposeWebSearchBottomSheet
participant Browser
User->>AbsPlayerFragment: Tap NowPlayingAction.WebSearch
AbsPlayerFragment->>NavController: navigate(nav_web_search, songDetailArgs(currentSong))
NavController->>WebSearchFragment: Show dialog destination nav_web_search
WebSearchFragment->>WebSearchFragment: Configure BottomSheetDialog behavior
WebSearchFragment->>ComposeWebSearchBottomSheet: Set Compose content with song and context
User->>ComposeWebSearchBottomSheet: Tap search engine button
ComposeWebSearchBottomSheet->>Browser: context.startActivity(song.searchQuery(engine).openWeb().toChooser())
Browser-->>User: Open web search results
Updated class diagram for AbsPlayerFragment and the new WebSearchFragment Compose screenclassDiagram
class AbsPlayerFragment {
<<abstract>>
+onOptionsItemSelected(item)
+handleNowPlayingAction(action)
}
class WebSearchFragment {
+navArgs : WebSearchFragmentArgs
+song : Song
+onCreateView(inflater, container, savedInstanceState) View
}
class WebSearchFragmentArgs {
+extraSong : Song
}
class WebSearchBottomSheetComposable {
<<Composable>>
+WebSearchBottomSheet(song : Song, context : Context) void
}
class WebSearchEngine {
<<enum>>
+entries : Array~WebSearchEngine~
}
class Song
class BottomSheetDialogFragment
class BottomSheetDialog {
+behavior
}
class BottomSheetBehavior {
+isFitToContents : Boolean
+skipCollapsed : Boolean
+peekHeight : Int
+maxHeight : Int
}
class BottomSheetDialogSurfaceComposable {
<<Composable>>
+BottomSheetDialogSurface(content) void
}
AbsPlayerFragment ..> WebSearchFragment : navigate nav_web_search
WebSearchFragment --|> BottomSheetDialogFragment
WebSearchFragment *-- WebSearchFragmentArgs
WebSearchFragment --> Song
WebSearchFragment ..> WebSearchBottomSheetComposable : setContent
WebSearchFragment ..> BottomSheetDialog
BottomSheetDialog *-- BottomSheetBehavior
WebSearchBottomSheetComposable --> Song
WebSearchBottomSheetComposable ..> WebSearchEngine : use entries
WebSearchBottomSheetComposable ..> BottomSheetDialogSurfaceComposable
WebSearchEngine <.. Song : searchQuery extension
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:
- In
WebSearchBottomSheetthesongparameter is nullable but is always accessed withsong!!; either makesongnon-nullable or add an early-return/disabled state to avoid potential NPEs. - The four
Buttonblocks inWebSearchBottomSheetare nearly identical and indexed manually intoengines; consider iterating overengines(orWebSearchEngine.entries) to generate buttons dynamically so the UI stays in sync if the enum changes. - Instead of passing an explicit
ContextintoWebSearchBottomSheet, consider usingLocalContext.currentinside the composable to keep the API more idiomatic for Compose and reduce coupling to the caller.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `WebSearchBottomSheet` the `song` parameter is nullable but is always accessed with `song!!`; either make `song` non-nullable or add an early-return/disabled state to avoid potential NPEs.
- The four `Button` blocks in `WebSearchBottomSheet` are nearly identical and indexed manually into `engines`; consider iterating over `engines` (or `WebSearchEngine.entries`) to generate buttons dynamically so the UI stays in sync if the enum changes.
- Instead of passing an explicit `Context` into `WebSearchBottomSheet`, consider using `LocalContext.current` inside the composable to keep the API more idiomatic for Compose and reduce coupling to the caller.
## Individual Comments
### Comment 1
<location> `app/src/main/java/com/mardous/booming/ui/screen/websearch/WebSearchScreen.kt:35-39` </location>
<code_context>
+import com.mardous.booming.ui.component.compose.BottomSheetDialogSurface
+@OptIn(ExperimentalMaterial3Api::class)
+@Composable
+fun WebSearchBottomSheet(
+ song: Song?,
+ context: Context
+) {
+ val engines = WebSearchEngine.entries.toTypedArray()
+
+ BottomSheetDialogSurface {
</code_context>
<issue_to_address>
**issue (bug_risk):** Nullable `song` combined with `song!!` dereferences introduces a crash risk.
`song` is nullable (`Song?`), but the button handlers use `song!!`, so this will crash if the sheet is ever opened without a song (e.g., missing nav args or future reuse). Either require a non-null `Song` in the function signature and callers, or add null handling (early return / disabled actions) and remove the `!!` usage.
</issue_to_address>
### Comment 2
<location> `app/src/main/java/com/mardous/booming/ui/screen/websearch/WebSearchScreen.kt:39-48` </location>
<code_context>
- override fun onCreateDialog(savedInstanceState: Bundle?): Dialog {
- val song = BundleCompat.getParcelable(requireArguments(), EXTRA_SONG, Song::class.java)!!
-
- val engines = WebSearchEngine.entries.toTypedArray()
- val titles = engines.map { getString(it.nameRes) }.toTypedArray()
-
</code_context>
<issue_to_address>
**suggestion:** Hard-coded enum indices and duplicated button code make the implementation fragile and harder to maintain.
The four buttons are manually duplicated and tied to `engines[0..3]` / `WebSearchEngine.entries[0..3]`, which will break if the enum size or order changes and is easy to miss when updating. Instead, iterate over `engines` (or `WebSearchEngine.entries`) to generate the buttons dynamically, using the enum values directly to avoid magic indices and duplication.
Suggested implementation:
```
import android.content.Context
import androidx.compose.foundation.interaction.MutableInteractionSource
import androidx.compose.foundation.layout.Arrangement
import androidx.compose.foundation.layout.Column
import androidx.compose.foundation.layout.Spacer
import androidx.compose.foundation.layout.fillMaxWidth
import androidx.compose.foundation.layout.height
import androidx.compose.foundation.layout.padding
import androidx.compose.foundation.layout.wrapContentHeight
import androidx.compose.foundation.rememberScrollState
import androidx.compose.foundation.verticalScroll
import androidx.compose.material3.Button
import androidx.compose.material3.ExperimentalMaterial3Api
import androidx.compose.material3.Text
import androidx.compose.runtime.Composable
import androidx.compose.runtime.remember
import androidx.compose.ui.Modifier
import androidx.compose.ui.res.stringResource
import androidx.compose.ui.unit.dp
import com.mardous.booming.extensions.media.searchQuery
import com.mardous.booming.extensions.openWeb
import com.mardous.booming.extensions.toChooser
import com.mardous.booming.ui.component.compose.BottomSheetDialogSurface
import com.mardous.booming.ui.screen.websearch.WebSearchEngine
@OptIn(ExperimentalMaterial3Api::class)
@Composable
fun WebSearchBottomSheet(
song: Song?,
context: Context
) {
val query = song?.searchQuery ?: return
val engines = remember { WebSearchEngine.entries.toList() }
val interactionSource = remember { MutableInteractionSource() }
BottomSheetDialogSurface {
Column(
modifier = Modifier
.fillMaxWidth()
.wrapContentHeight()
.verticalScroll(rememberScrollState())
.padding(horizontal = 16.dp, vertical = 24.dp),
verticalArrangement = Arrangement.spacedBy(12.dp)
) {
engines.forEach { engine ->
Button(
modifier = Modifier.fillMaxWidth(),
onClick = {
// This assumes a WebSearchEngine API similar to:
// fun createUri(query: String): Uri
// Adjust to match the actual enum implementation.
val intent = engine
.createUri(query)
.toChooser(context)
context.openWeb(intent)
},
interactionSource = interactionSource
) {
Text(text = stringResource(id = engine.nameRes))
}
}
Spacer(modifier = Modifier.height(8.dp))
}
}
}
```
I had to reconstruct the body of `WebSearchBottomSheet` based on your comment and the imports, so you will need to:
1. Replace the `onClick` block body with the exact logic you currently have for launching a web search, but using `engine` instead of a hard‑coded `engines[0]` / `WebSearchEngine.entries[0]` etc. For example, if your existing code is:
- `context.openWeb(WebSearchEngine.entries[0].createUri(query).toChooser(context))`
then inside the loop, change it to:
- `context.openWeb(engine.createUri(query).toChooser(context))`
2. Remove any now‑unused imports or parameters that may have been present in the original implementation (e.g. if you were using different button types or layout modifiers).
3. If your design system uses a custom button composable instead of `Button` / `Text`, swap those in while keeping the `engines.forEach { engine -> ... }` pattern.
4. Ensure `WebSearchEngine` is imported from the correct package; if it’s already in the same package you can remove the explicit import I added.
</issue_to_address>
### Comment 3
<location> `app/src/main/res/values/dimens.xml:22` </location>
<code_context>
<dimen name="mini_player_height">70dp</dimen>
<dimen name="bottom_nav_height">80dp</dimen>
<dimen name="queue_height">400dp</dimen>
+ <dimen name="shuffle_height">800dp</dimen>
<dimen name="icon_notification_size">32dp</dimen>
</code_context>
<issue_to_address>
**suggestion:** The hardcoded `800dp` bottom sheet height may not scale well across devices.
Using a fixed 800dp for `shuffle_height` (as the bottom sheet peek/max height) risks exceeding the viewport on small screens and looking disproportionate on large ones. Consider deriving this from the available screen height (or using `MATCH_PARENT`/percent-based behavior) instead of a large hardcoded dp value to keep the layout responsive across devices.
Suggested implementation:
```
<dimen name="queue_height">400dp</dimen>
<!-- Actual shuffle bottom sheet height is calculated at runtime based on the screen height.
This value is only a fallback/default and should remain small. -->
<dimen name="shuffle_height">0dp</dimen>
<dimen name="icon_notification_size">32dp</dimen>
```
To fully implement the suggestion (derive the height from the available screen height), you will also need to:
1. Locate where `R.dimen.shuffle_height` is used (likely in the shuffle bottom sheet layout or where `BottomSheetBehavior` is configured).
2. Replace the direct use of the dimen as a fixed height with runtime logic, for example:
- Obtain the screen height via `Resources.getSystem().displayMetrics.heightPixels` or a `WindowMetrics` API.
- Compute a proportional height, e.g. `val shuffleHeight = (screenHeight * 0.6f).toInt()`.
- Apply this height to the bottom sheet’s layout params or to `BottomSheetBehavior.peekHeight`.
3. Keep `shuffle_height` as a fallback (0dp or a very small value) and for previews, but rely on the runtime calculation for actual devices so the sheet scales appropriately on small and large screens.
</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
+39
to
+48
| val engines = WebSearchEngine.entries.toTypedArray() | ||
|
|
||
| BottomSheetDialogSurface { | ||
| Column( | ||
| modifier = Modifier | ||
| .fillMaxWidth() | ||
| .wrapContentHeight() | ||
| ) { | ||
| Column( | ||
| modifier = Modifier |
Contributor
There was a problem hiding this comment.
suggestion: Hard-coded enum indices and duplicated button code make the implementation fragile and harder to maintain.
The four buttons are manually duplicated and tied to engines[0..3] / WebSearchEngine.entries[0..3], which will break if the enum size or order changes and is easy to miss when updating. Instead, iterate over engines (or WebSearchEngine.entries) to generate the buttons dynamically, using the enum values directly to avoid magic indices and duplication.
Suggested implementation:
import android.content.Context
import androidx.compose.foundation.interaction.MutableInteractionSource
import androidx.compose.foundation.layout.Arrangement
import androidx.compose.foundation.layout.Column
import androidx.compose.foundation.layout.Spacer
import androidx.compose.foundation.layout.fillMaxWidth
import androidx.compose.foundation.layout.height
import androidx.compose.foundation.layout.padding
import androidx.compose.foundation.layout.wrapContentHeight
import androidx.compose.foundation.rememberScrollState
import androidx.compose.foundation.verticalScroll
import androidx.compose.material3.Button
import androidx.compose.material3.ExperimentalMaterial3Api
import androidx.compose.material3.Text
import androidx.compose.runtime.Composable
import androidx.compose.runtime.remember
import androidx.compose.ui.Modifier
import androidx.compose.ui.res.stringResource
import androidx.compose.ui.unit.dp
import com.mardous.booming.extensions.media.searchQuery
import com.mardous.booming.extensions.openWeb
import com.mardous.booming.extensions.toChooser
import com.mardous.booming.ui.component.compose.BottomSheetDialogSurface
import com.mardous.booming.ui.screen.websearch.WebSearchEngine
@OptIn(ExperimentalMaterial3Api::class)
@Composable
fun WebSearchBottomSheet(
song: Song?,
context: Context
) {
val query = song?.searchQuery ?: return
val engines = remember { WebSearchEngine.entries.toList() }
val interactionSource = remember { MutableInteractionSource() }
BottomSheetDialogSurface {
Column(
modifier = Modifier
.fillMaxWidth()
.wrapContentHeight()
.verticalScroll(rememberScrollState())
.padding(horizontal = 16.dp, vertical = 24.dp),
verticalArrangement = Arrangement.spacedBy(12.dp)
) {
engines.forEach { engine ->
Button(
modifier = Modifier.fillMaxWidth(),
onClick = {
// This assumes a WebSearchEngine API similar to:
// fun createUri(query: String): Uri
// Adjust to match the actual enum implementation.
val intent = engine
.createUri(query)
.toChooser(context)
context.openWeb(intent)
},
interactionSource = interactionSource
) {
Text(text = stringResource(id = engine.nameRes))
}
}
Spacer(modifier = Modifier.height(8.dp))
}
}
}
I had to reconstruct the body of WebSearchBottomSheet based on your comment and the imports, so you will need to:
-
Replace the
onClickblock body with the exact logic you currently have for launching a web search, but usingengineinstead of a hard‑codedengines[0]/WebSearchEngine.entries[0]etc. For example, if your existing code is:context.openWeb(WebSearchEngine.entries[0].createUri(query).toChooser(context))
then inside the loop, change it to:
context.openWeb(engine.createUri(query).toChooser(context))
-
Remove any now‑unused imports or parameters that may have been present in the original implementation (e.g. if you were using different button types or layout modifiers).
-
If your design system uses a custom button composable instead of
Button/Text, swap those in while keeping theengines.forEach { engine -> ... }pattern. -
Ensure
WebSearchEngineis imported from the correct package; if it’s already in the same package you can remove the explicit import I added.
| <dimen name="mini_player_height">70dp</dimen> | ||
| <dimen name="bottom_nav_height">80dp</dimen> | ||
| <dimen name="queue_height">400dp</dimen> | ||
| <dimen name="shuffle_height">800dp</dimen> |
Contributor
There was a problem hiding this comment.
suggestion: The hardcoded 800dp bottom sheet height may not scale well across devices.
Using a fixed 800dp for shuffle_height (as the bottom sheet peek/max height) risks exceeding the viewport on small screens and looking disproportionate on large ones. Consider deriving this from the available screen height (or using MATCH_PARENT/percent-based behavior) instead of a large hardcoded dp value to keep the layout responsive across devices.
Suggested implementation:
<dimen name="queue_height">400dp</dimen>
<!-- Actual shuffle bottom sheet height is calculated at runtime based on the screen height.
This value is only a fallback/default and should remain small. -->
<dimen name="shuffle_height">0dp</dimen>
<dimen name="icon_notification_size">32dp</dimen>
To fully implement the suggestion (derive the height from the available screen height), you will also need to:
- Locate where
R.dimen.shuffle_heightis used (likely in the shuffle bottom sheet layout or whereBottomSheetBehavioris configured). - Replace the direct use of the dimen as a fixed height with runtime logic, for example:
- Obtain the screen height via
Resources.getSystem().displayMetrics.heightPixelsor aWindowMetricsAPI. - Compute a proportional height, e.g.
val shuffleHeight = (screenHeight * 0.6f).toInt(). - Apply this height to the bottom sheet’s layout params or to
BottomSheetBehavior.peekHeight.
- Obtain the screen height via
- Keep
shuffle_heightas a fallback (0dp or a very small value) and for previews, but rely on the runtime calculation for actual devices so the sheet scales appropriately on small and large screens.
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 Web Search Dialog to Compose
Summary by Sourcery
Convert the web search dialog to a Compose-based bottom sheet fragment and update navigation to use it instead of the legacy dialog.
New Features:
Enhancements: