-
Notifications
You must be signed in to change notification settings - Fork 581
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
Uses Horologist DataHelpers in DataLayer sample #1111
Conversation
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'm torn.
There is a nice simplicity of using the raw APIs, but also it ignores the real complexities of those APIs, and real world practices.
@@ -29,7 +29,7 @@ android { | |||
applicationId "com.example.android.wearable.datalayer" | |||
versionCode 1 | |||
versionName "1.0" | |||
minSdk 21 | |||
minSdk 23 |
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.
should we comment why this is? bumping the minSdk on mobile seems discussion worthy.
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 came as a consequence of using Horologist on phone side.
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.
Yep - let me confirm why that is, so we can at least tell developers the root cause, why horologist is 23.
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.
Testing on google/horologist#2249
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.
will update when we release next Horologist version / or can wait to land until we do a Horologist release
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.
Maybe a Todo? And land now?
DataLayer/Wearable/src/main/java/com/example/android/wearable/datalayer/MainActivity.kt
Outdated
Show resolved
Hide resolved
DataLayer/Wearable/src/main/java/com/example/android/wearable/datalayer/MainActivity.kt
Outdated
Show resolved
Hide resolved
displayNodes(nodes) | ||
var nodeList: Set<Node>? = null | ||
if (wearDataLayerAppHelper.isAvailable()) { | ||
wearDataLayerAppHelper.connectedAndInstalledNodes.collect { nodes -> |
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.
Does this flow.collect ever return?
// Increment the count to send next time | ||
count++ | ||
// Increment the count to send next time | ||
count++ |
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 think this example is problematic. The count isn't stored or saved, so if there are reasons why the activity gets recreated it will revert to 0.
I think either
a) explain that the count is local to the specific instance of this activity and may reset
b) we use the DataClient as the current value and read and update the value.
The horologist sample might look more complicated with proto, and DataStore. But it's actually fundamentally a more real world example.
Closing this as decided to not use DataHelpers and demonstrate Wearable DataLayer library. Improvement for the sample is in #1116 |
No description provided.