-
Notifications
You must be signed in to change notification settings - Fork 344
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
JetStream and Catalog updates #165
Conversation
(cherry picked from commit 4a39926)
…he lambda provides the selected tab index
…p of the screen while navigating to the right
TvMaterialCatalog/app/src/main/java/com/google/tv/material/catalog/AppBar.kt
Outdated
Show resolved
Hide resolved
TvMaterialCatalog/app/src/main/java/com/google/tv/material/catalog/Component.kt
Show resolved
Hide resolved
TvMaterialCatalog/app/src/main/java/com/google/tv/material/catalog/AppBar.kt
Outdated
Show resolved
Hide resolved
TvMaterialCatalog/app/src/main/java/com/google/tv/material/catalog/Component.kt
Show resolved
Hide resolved
.focusRequester(focusRequester) | ||
.focusRestorer(), |
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.
We don't need to make use of focusRequester
modifier to save and restore focus when using focusRestorer
modifier.
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.
Removing this gives me an exception:
java.lang.IllegalStateException:
FocusRequester is not initialized. Here are some possible fixes:
1. Remember the FocusRequester: val focusRequester = remember { FocusRequester() }
2. Did you forget to add a Modifier.focusRequester() ?
3. Are you attempting to request focus during composition? Focus requests should be made in
response to some event. Eg Modifier.clickable { focusRequester.requestFocus() }
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.
The focusRequester
modifier is necessary as the request
focus method is called over the associated FocusRequester
object. Without the modifier, calling requestFocus
method crashes the app.
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.
Actually, focusRestorer doesn't need any focusRequester modifier, to work. If the app is crashing, it could be a bug, most likely it would be because of the 3rd point: "attempting to request focus during composition".
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.
I just noticed that there is a Launched Effect which is requesting focus on the focus requester. If the focusRequester is not initialized, it will crash the app. I think the goal here is to request focus on the first element. In that case, the focusRequester should be assigned to the first item's modifier and not the parent grid.
TvMaterialCatalog/app/src/main/java/com/google/tv/material/catalog/HomeGridCard.kt
Outdated
Show resolved
Hide resolved
@Composable | ||
fun ComponentsGridCard( | ||
component: Component, | ||
modifier: Modifier = Modifier | ||
modifier: Modifier = Modifier, | ||
onClick: (() -> Any)? = 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.
Would you elaborate on this? I believe click event handler does not need to return value, so the type of onClick
parameter should be as follows. The suggested code would simplify the code to call the callback:
onClick: () -> Unit = {}
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.
I've adjusted this slightly to allow the lambda to return Any and make sure the invoker returns it as expected.
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.
The callback should not have a return value, as it is to deliver click events to the outer component.
If we need to handle the result of function calls that potentially fails, we should introduce another component to manage state as recommended in this doc.
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.
LGTM with a minor comment.
import androidx.compose.ui.ExperimentalComposeUiApi | ||
import androidx.compose.ui.Modifier | ||
import androidx.compose.ui.focus.FocusRequester | ||
import androidx.compose.ui.focus.focusRequester |
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.
nit: redundant import
.focusRequester(focusRequester) | ||
.focusRestorer(), |
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.
The focusRequester
modifier is necessary as the request
focus method is called over the associated FocusRequester
object. Without the modifier, calling requestFocus
method crashes the app.
@Composable | ||
fun HomeGrid() { | ||
val focusRequester = remember { FocusRequester() } | ||
val itemClick = { | ||
focusRequester.saveFocusedChild() |
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.
I don't think we need this. If the item gains focus, it automatically gets saved if the parent grid is wrapped with "focusRestorer"
LaunchedEffect(Unit) { | ||
focusRequester.requestFocus() | ||
} |
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 could lead to crashes because of race condition. It is possible that the LaunchedEffect is executed before the items are placed and it would cause a crash. We can make use of onPlaced
or onGloballyPositioned
modifiers to see if the item is available to get focus. Refer b/276738340#comment6
TvMaterialCatalog/app/src/main/java/com/google/tv/material/catalog/HomeGridCard.kt
Outdated
Show resolved
Hide resolved
CardLayoutDefaults.ImageCard( | ||
onClick = { navHostController.navigate(component.routeValue) }, | ||
Card( | ||
onClick = { |
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.
The code to handle click events should be updated as follows:
Card(
onClick = {
onClick()
navHostController.navigate(component.routeValue)
}
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.
I've changed this such that the click handler is provided into ComponentsGridCard, ptal.
latest from upstream
…ayout using BringIntoViewSpec instead of modifier.pivotOffset
JetStream:
TV Material Catalog: