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

Migration to Jetpack Compose. #179

Merged
merged 5 commits into from
Dec 12, 2023
Merged

Conversation

pierredelisle
Copy link
Contributor

See https://developer.android.com/codelabs/jetpack-compose-migration#0 for background information.

Step 1: very simple migration as a first step.
bottom-up approach where we simply migrate the Home screen view subtree that is shown when no devices are currently commissioned to the ecosystem.

Also, some gradle upgrades:

  • compileSdk: 33 -> 34
  • Gradle plugin: 8.1.0 -> 8.1.1
  • Kotlin plugin: 1.9.0 -> 1.9.20
  • material: 1.9.0 -> 1.10.0

See https://developer.android.com/codelabs/jetpack-compose-migration#0 for background information.

Step 1: very simple migration as a first step.
bottom-up approach where we simply migrate the Home screen view subtree that is shown when no devices are currently commissioned to the ecosystem.

Also, some gradle upgrades:
- compileSdk: 33 -> 34
- Gradle plugin: 8.1.0 -> 8.1.1
- Kotlin plugin: 1.9.0 -> 1.9.20
- material: 1.9.0 -> 1.10.0
Copy link
Collaborator

@tunjid tunjid left a comment

Choose a reason for hiding this comment

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

Just the minor change about the ViewModel being directly passed to the composable.

@JolandaVerhoef
Copy link

LGTM! Could you add me as a reviewer on next PRs so I can check them from a Compose perspective? Happy to answer any questions when they arise :)

3p-ecosystem/build.gradle.kts Outdated Show resolved Hide resolved
3p-ecosystem/build.gradle.kts Show resolved Hide resolved
gradle/libs.versions.toml Outdated Show resolved Hide resolved
build.gradle.kts Outdated Show resolved Hide resolved
3p-ecosystem/build.gradle.kts Outdated Show resolved Hide resolved
3p-ecosystem/build.gradle.kts Outdated Show resolved Hide resolved
3p-ecosystem/build.gradle.kts Show resolved Hide resolved
3p-ecosystem/build.gradle.kts Show resolved Hide resolved
Copy link

@javadude javadude left a comment

Choose a reason for hiding this comment

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

LGTM, but should be reviewed by a Compose person to make sure

- The plugins block should be using the version catalog.
- specify 'composeBom' version in version catalog
- all deps should be in version catalog and referenced via libs.xxxxx...
- Should be using the version catalog for plugins
- The HomeScreen composable should take only lambdas and state if possible.
- Should update the versionCode and versionName
- Delete the compileOptions and kotlinOptions blocks
@pierredelisle
Copy link
Contributor Author

pierredelisle commented Dec 12, 2023

LGTM! Could you add me as a reviewer on next PRs so I can check them from a Compose perspective? Happy to answer any questions when they arise :)

Thanks Jolanda, will do. As mentioned via chat, it's not clear to me what's missing, but I cannot see the Composable previews in Android Studio. Note that I do have the following in my build.gradle.kts:

    implementation(libs.androidx.compose.ui.tooling.preview)
    debugImplementation(libs.androidx.compose.ui.tooling)

I'm using Giraffe | 2022.3.1 Patch 1.
Thanks for any insight on this.


Update:
Filed #183.
We can address that issue in a subsequent PR.

@pierredelisle
Copy link
Contributor Author

Just the minor change about the ViewModel being directly passed to the composable.

@tunjid I looked at the code you referred to, and the ViewModel does get passed to InterestsRoute.
See InterestsRoute.

@Composable
internal fun InterestsRoute(
    onTopicClick: (String) -> Unit,
    modifier: Modifier = Modifier,
    viewModel: InterestsViewModel = hiltViewModel(),
) {
    val uiState by viewModel.uiState.collectAsStateWithLifecycle()

    InterestsScreen(
        uiState = uiState,
        followTopic = viewModel::followTopic,
        onTopicClick = onTopicClick,
        modifier = modifier,
    )
} 

Not sure how I can get rid of it since in the composable we extract uiState from it.

@tunjid
Copy link
Collaborator

tunjid commented Dec 12, 2023

@pierredelisle in the interests snippet, there's a parent Composable; the InterestsRoute. Its exists only to get the appropriately scoped ViewModel from hilt. It grabs the ViewModel then extracts the uiState from it before passing it to the actual InterestsScreen Composable.

The InterestsScreen Composable is the actual screen and can be used with the @Preview annotation since its state is easily constructed inline and tested.

Your latest changes seem to already follow this approach. I made a few changes:

  • HomeScreen takes the devicesUiModel as an argument. Idiomatically, the HomeScreen is now solely a function of the devicesUiModel.
  • Moved logging to a effect of the devicesUiModel changing. If two devicesUiModel changes between compositions, it will be logged. Prior, anything that triggers recomposition would cause a log.
  // This composable is a wrapper composable for extracting data from the ViewModel before passing it to the
  // actual screen composable
 @Composable
  private fun HomeRoute(homeViewModel: HomeViewModel) {
    // Observes values coming from the VM's devicesUiModelLiveData
    val devicesUiModel by homeViewModel.devicesUiModelLiveData.observeAsState()
    HomeScreen(devicesUiModel)
  }

  @Composable
  private fun HomeScreen(devicesUiModel: DeviceUiModel?) {
    val noDevices = devicesUiModel == null || devicesUiModel.devices.isEmpty()
    if (noDevices) {
      NoDevices()
    }

    LaunchedEffect(devicesUiModel) {
        Timber.d("HomeRoute [${devicesUiModel}]")
    }
  }

@javadude
Copy link

Build changes look good.

- Pass DevicesUiModel to HomeScreen (instead of just a noDevices boolean)
@pierredelisle
Copy link
Contributor Author

Your latest changes seem to already follow this approach. I made a few changes:

Got it. Thanks for the clarifications! Made the changes.

@pierredelisle pierredelisle merged commit 62d5935 into main Dec 12, 2023
4 checks passed
@pierredelisle pierredelisle deleted the pierred/jetpack-compose-1 branch December 12, 2023 21:01
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.

4 participants