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

JetStream and Catalog updates #165

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

Conversation

pflammertsma
Copy link
Member

@pflammertsma pflammertsma commented Apr 26, 2024

JetStream:

  • added adaptive banner

TV Material Catalog:

  • minor UI tweaks
  • resolved bug causing recomposition
  • remember and restore focus in HomeGrid

@pflammertsma pflammertsma changed the title added adaptive banner JetStream and Catalog updates Apr 26, 2024
Comment on lines +44 to +45
.focusRequester(focusRequester)
.focusRestorer(),
Copy link
Contributor

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.

Copy link
Member Author

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() }

Copy link
Contributor

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.

Copy link
Contributor

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".

Copy link
Contributor

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.

@Composable
fun ComponentsGridCard(
component: Component,
modifier: Modifier = Modifier
modifier: Modifier = Modifier,
onClick: (() -> Any)? = null
Copy link
Contributor

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 = {}

Copy link
Member Author

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.

TvMaterialCatalog/gradle/libs.versions.toml Outdated Show resolved Hide resolved
Copy link
Contributor

@chikoski chikoski left a 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
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: redundant import

Comment on lines +44 to +45
.focusRequester(focusRequester)
.focusRestorer(),
Copy link
Contributor

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()
Copy link
Contributor

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"

Comment on lines +30 to +32
LaunchedEffect(Unit) {
focusRequester.requestFocus()
}
Copy link
Contributor

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

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.

None yet

3 participants