-
Notifications
You must be signed in to change notification settings - Fork 15
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
Added widget for WearOS device battery level #195
base: master
Are you sure you want to change the base?
Conversation
Looks good for first check. But, I do not merge at the moment. Maybe next year, as there is no time left. This year. |
That's fine, thanks for looking.. have a great xmas! |
@@ -28,15 +30,17 @@ enum class WidgetType(val cls: Class<*>) { | |||
GLUCOSE_TREND_DELTA(GlucoseTrendDeltaWidget::class.java), | |||
GLUCOSE_TREND_DELTA_TIME(GlucoseTrendDeltaTimeWidget::class.java), | |||
GLUCOSE_TREND_DELTA_TIME_IOB_COB(GlucoseTrendDeltaTimeIobCobWidget::class.java), | |||
OTHER_UNIT(OtherUnitWidget::class.java); | |||
OTHER_UNIT(OtherUnitWidget::class.java), | |||
BATTERY_LEVEL(BatteryLevelWidget::class.java); |
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 you can not use this base class, as these widget will not be updated for Battery-Level change.
You have to use an own class with parent NotifierInterface and register for NotifySource.NODE_BATTERY_LEVEL.
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.
Also for NotifySource.CAPILITY_INFO, this will be called, if wear is connected or disconnected...
The widget Notifier should only be active, if there is at least one Widget active.
The object/class must be initialized via GlucoDataServiceMobile::onCreate():
GlucoDataHandler/mobile/src/main/java/de/michelinside/glucodatahandler/GlucoDataServiceMobile.kt
Line 165 in f546973
GlucoseBaseWidget.updateWidgets(applicationContext) |
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'll investigate this
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.
Have moved this into a separate class now
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.
Then remove all changes to this class.
|
||
|
||
if (hasBatteryLevel) { | ||
var deviceNameStr = "No WearOS Device" |
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.
No static text! Check, if there is already one. For example "activity_main_disconnected_label".
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.
My bad, will add new string
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.
Fixed
|
||
if (WearPhoneConnection.nodesConnected) { | ||
if (WearPhoneConnection.connectionError) { | ||
deviceNameStr = "Connection error" |
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.
No static text here.
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.
My bad, will add new string
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.
Fixed
|
||
if (hasBatteryLevel) { | ||
var deviceNameStr = "No WearOS Device" | ||
var levelStr = "?%" |
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.
Either ?% or --%
deviceNameStr = "Connection error" | ||
} | ||
else { | ||
WearPhoneConnection.getNodeBatterLevels().firstNotNullOf { (name, level) -> |
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 also thought about supporting for more than one watch, but this is more a corner case and would make it too complicated... So using only the first is fine!
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 did think about this too.. I figured it would be unlikely a user would have more than one device
WearPhoneConnection.getNodeBatterLevels().firstNotNullOf { (name, level) -> | ||
if (level > 0) | ||
levelStr = "$level%" | ||
deviceNameStr = name |
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 we can simplify the name, while removing "(...)" For example "Galaxy Watch 5 (abc)" would be nicer to have only "Galaxy Watch 5".
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 only have my own watch as reference which doesn't have anything in braces after the name.. I can check for that case and remove if required.
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.
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.
Or leave it as it is, I will change in the Wear class to have the same name everywhere.
…er to also send device name.
Hi Michael, I have moved this login into a separate class now. |
if (node != null) | ||
deviceName = node.displayName | ||
|
||
extra.putString(BatteryReceiver.DEVICENAME, deviceName); |
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 it makes more sense to add the nodeId, so the data can be requested by nodeId...
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.
Sure
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.
Have removed this field now as I'm using broadcast to update widgets.
try { | ||
Log.d(LOG_ID, "OnNotifyData called for source $dataSource ${extras?.toString()}") | ||
|
||
if (dataSource == NotifySource.NODE_BATTERY_LEVEL) { |
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.
What about CAPILITY_INFO?
I think it would be better to trigger a broadcast to like here:
GlucoDataHandler/mobile/src/main/java/de/michelinside/glucodatahandler/widget/GlucoseBaseWidget.kt
Line 71 in f546973
val intent = Intent(context, type.cls) |
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 see CAPILITY_INFO when my watch goes onto airplane mode or I disable all connectivity. Any ideas why I don't see these?
Changing to a broadcast should be straightforward enough
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.
Sometime it take some time, until the GDH gets this information. Just checkout the main screen for connection, as long the Watch is connected, GDH did not get the info from the system...
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.
Updated to use broadcast. Battery level and watch name now taken from nodesConnected data
appWidgetIds.forEach { appWidgetId -> | ||
BatteryLevelWidget.updateWidget(context, appWidgetManager, appWidgetId, | ||
extras?.getInt(BatteryReceiver.LEVEL) ?: 0, | ||
extras?.getString(BatteryReceiver.DEVICENAME) ?: context.getString(R.string.activity_main_disconnected_label) |
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 may cause problems, if there is more than one watch connected, the widget would change every time between the both watches. Better use the broadcast, which will trigger OnUpdate in Widget class.
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 this help for multiple watches? It's pretty edge case that someone would have more than one..
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.
BATTERY_LEVEL(BatteryLevelWidget::class.java) - This line shouldn't be there, I thought I had removed that. I'm not using your GlucoseBaseWidget class at all now.
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.
Michael, here's some example logs.. At line 18 I get the CAPILITY_INFO message ok.. after this I put my watch into airplane mode and observe the watch being removed from the main screen, but I don't get any corresponding CAPILITY_INFO message in my handler to indicate this...
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.
You are right. I have to check and will fix it. Sorry!
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.
Mi Michael, that isn't working quite as expected.. I get the message will null in extras when watch is disconnected.. When the watch connects I get the same message with a null payload..
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.
Yes, that is the reason that you should not use the extras. Just trigger a widget update with the related broadcast, which will cause calling "onUpdate", where you are using the first watch in list..
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 way I had planned to implement this was to stop the BATTERY_LEVEL notification if extras was null and restart if extras had data.. Are you happy to leave the BATTERY_LEVEL notifier running if the watch is disconnected?
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.
As long there is a widget present, you should keep notifier for both sources.
On each "OnNotifyData" for these sources, you should trigger an update of all widgets using the broadcast.
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.
Cool. That makes things simpler
@@ -28,15 +30,17 @@ enum class WidgetType(val cls: Class<*>) { | |||
GLUCOSE_TREND_DELTA(GlucoseTrendDeltaWidget::class.java), | |||
GLUCOSE_TREND_DELTA_TIME(GlucoseTrendDeltaTimeWidget::class.java), | |||
GLUCOSE_TREND_DELTA_TIME_IOB_COB(GlucoseTrendDeltaTimeIobCobWidget::class.java), | |||
OTHER_UNIT(OtherUnitWidget::class.java); | |||
OTHER_UNIT(OtherUnitWidget::class.java), | |||
BATTERY_LEVEL(BatteryLevelWidget::class.java); |
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.
Then remove all changes to this class.
…m BatteryReceiver
|
||
|
||
|
||
|
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.
Still some changes left ;-)
companion object { | ||
const val LOG_ID = "GDH.widget.BatteryLevelWidget" | ||
|
||
fun updateWidget( |
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.
Move to class (not companion) and make it private.
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 can be private, but not important...
mobile/src/main/java/de/michelinside/glucodatahandler/widget/BatteryLevelWidget.kt
Outdated
Show resolved
Hide resolved
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.
Really good work!!! Thank you soo much!
companion object { | ||
const val LOG_ID = "GDH.widget.BatteryLevelWidget" | ||
|
||
fun updateWidget( |
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 can be private, but not important...
|
||
// Use values from current connection if we have one as it might take a while to get a NODE_BATTERY_LEVEL message to update widget | ||
if (WearPhoneConnection.nodesConnected && !WearPhoneConnection.connectionError) { | ||
WearPhoneConnection.getNodeBatterLevels().firstNotNullOf { (name, level) -> |
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.
What about creating a new function return first BatteryLevel and name using pickBestNodeId()!?
GlucoDataHandler/common/src/main/java/de/michelinside/glucodatahandler/common/WearPhoneConnection.kt
Line 283 in f546973
fun pickBestNodeId(): String? { |
This can be moved to companion object of WearPhoneConnection and a new function return the battery level for this nodeId:
fun getNodeBatteryLevel(nodeId: String)
Just an improvement/suggestion...
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.
Have implemented this
|
||
fun init() { | ||
Log.d(LOG_ID, "init called") | ||
AddNotifier() |
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.
Only add, if there is a widget. Just keep the one in OnUpdate and remove the init...
|
||
val appWidgetManager = AppWidgetManager.getInstance(context) | ||
val componentName = | ||
ComponentName(context!!.packageName, BatteryLevelWidget::class.java.name) |
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.
Better use context as param!?
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.
Added safety checks
fun RemoveNotifier() { | ||
try { | ||
Log.d(LOG_ID, "RemoveNotifier called for " + this.toString()) | ||
InternalNotifier.remNotifier(context!!, this) |
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.
Same here, use context as Param
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.
Added safety checks
…hecks when accessing context
Hi Michael, do you need me to make any more changes to this branch? |
I will have a look tomorrow. |
Looks good so far! I will test it. |
Juggluco |
Thank you! Same like me. I hoped, you are using Dexcom, as it would be great to have someone, who knows my code and can check the Dexcom Share part, as I do not have access. I have to find another one :-) |
Good luck finding somebody :) |
@RobertoW-UK Are you interested in continuing develop something for GDH? Best regards, Michael |
Hey Michael, would be happy to help out.. Bear in mind I'm learning Android as I go though :) |
This is really no problem, as this is my first Android app and the most things, I´m doing, are also new to me.
You can also write me a mail: [email protected] Regards, Michael |
Note: The new string "widget_wearos_battery_level" will need translations.