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

Update sample with more documentation and remove Toasts #1116

Merged
merged 1 commit into from
Jun 10, 2024

Conversation

kul3r4
Copy link
Contributor

@kul3r4 kul3r4 commented Jun 5, 2024

  • remove Toasts and create 2 separate screens for showing nodes
  • Update ReadMe with more details and point to Horologist for specific use cases

@kul3r4 kul3r4 requested review from yschimke and garanj June 5, 2024 16:03
@kul3r4 kul3r4 force-pushed the datalayer-version2 branch 3 times, most recently from b2c85ce to f44e771 Compare June 5, 2024 16:26
* This form is easier to work with when trying to operate upon all [Node]s.
*/
private suspend fun getCapabilitiesForReachableNodes(): Map<Node, Set<String>> =
capabilityClient.getAllCapabilities(CapabilityClient.FILTER_REACHABLE)
Copy link
Contributor

Choose a reason for hiding this comment

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

This call can fail, so we should probably catch the ApiExceptions and log?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Under which circumstances can fail?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not likely, more about correct code.

https://developers.google.com/android/reference/com/google/android/gms/wearable/WearableStatusCodes

I think any call can technically fail with TIMEOUT, REMOTE_EXCEPTION, INTERNAL_ERROR.

#tioli

Copy link
Contributor

Choose a reason for hiding this comment

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

We should consider these a remote call to another process on the device.

fun MainScreen(
onShowNodesList: () -> Unit,
onShowCameraNodesList: () -> Unit,
image: Bitmap?,
Copy link
Contributor

Choose a reason for hiding this comment

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

Bitmap might not be a good type on a Composable function. We should probably introduce some data class and mark it Stable?

setContent {
MainApp(
events = clientDataViewModel.events,
image = clientDataViewModel.image,
onQueryOtherDevicesClicked = ::onQueryOtherDevicesClicked,
onQueryMobileCameraClicked = ::onQueryMobileCameraClicked
nodesViewModel = nodesViewModel
Copy link
Contributor

Choose a reason for hiding this comment

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

This API feels a bit weird. you have state from clientDataViewModel and nodesViewModel directly.

Should we remove clientDataViewModel here and move that logic into some MainAppViewModel?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, at the moment the clientDataViewModel is extending the various listeners (message, capability etc.) and overrides the onchanged but we also have a DataLayerListenerService that implements the WearableListenerService and there again the onchanged methods are overriden. Perhaps we should try to keep only one place where those are overriden?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved the nodesViewModel creation, e.g.

fun CameraNodesScreen(
    viewModel: NodesViewModel = viewModel(factory = NodesViewModel.Factory)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the other part of the comment related to creation of the ViewModel, tracking it in #1119

Copy link
Contributor

@yschimke yschimke left a comment

Choose a reason for hiding this comment

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

Nothing blocking

@kul3r4 kul3r4 force-pushed the datalayer-version2 branch 3 times, most recently from 6b1f5c7 to a879492 Compare June 7, 2024 15:45
@kul3r4
Copy link
Contributor Author

kul3r4 commented Jun 10, 2024

Related to #1039

@kul3r4 kul3r4 merged commit acaee14 into android:main Jun 10, 2024
3 checks passed
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.

2 participants