HomeWidget.updateWidget support Long type add Int to error message #270
Conversation
|
I will merge the beta branch today to handle the size error. Could you already add a test case to the android integration test to ensure that Long is supported properly? Thanks!! |
|
Hey @bannzai would you have time to merge back the latest changes from main and add an android integration test for this? |
|
Okay! I'll try it soon. |
…/android-valid-type
| @@ -73,7 +80,12 @@ class HomeWidgetPlugin : FlutterPlugin, MethodCallHandler, ActivityAware, | |||
| val value = prefs.all[id] ?: defaultValue | |||
|
|
|||
| if(value is Long) { | |||
| result.success(java.lang.Double.longBitsToDouble(value)) | |||
| // The reason for falling back to Double is that it was supported before Long. | |||
| when (prefs.getString(ACTUAL_WIDGET_DATA_TYPE, "Double")) { | |||
There was a problem hiding this comment.
This Key was added because there is no way to distinguish between Double and Long.
There was a problem hiding this comment.
I believe the old logic should have already covered the logic. Looking at the code I also think that there is no benefit in this extra logic.
home_widget already handled this behavior by storing handling doubles always as long. so in that way we trade 2 bytes per number for the behavior.
So can you please revert your changes to the initial fix (adding is Long to the setWidgetData call)
There was a problem hiding this comment.
Why do you use longBitsToDouble to convert a Long to a Double? Also, if I want a Long (int in Dart) to be returned, is that ok?
|
Test Passed on my local mac ✅ @ABausG Please review it. |
There was a problem hiding this comment.
I don't fully understand the necessity for the changes you made in the last commit. Could you revert those/explain a bit more why those changes would be necessary?
Apart from that can you potentially also adjust the iOS integration test to include the Long case?
I will let the pipeline run once the comments are addressed
| @@ -10,6 +10,7 @@ void main() { | |||
| 'intKey': 12, | |||
| 'boolKey': true, | |||
| 'floatingNumberKey': 12.1, | |||
| 'longKey': 8640000000000000, | |||
There was a problem hiding this comment.
This should rather be double.maxFinite
There was a problem hiding this comment.
What difference does this make to the double test case?
| // For example, both Double and Long are saved in SharedPreferences with putLong, | ||
| // and this Key is used to distinguish between them when retrieving. | ||
| private const val ACTUAL_WIDGET_DATA_TYPE = "actualWidgetDataType" |
There was a problem hiding this comment.
This would never work when Users wanted to store both double and long as this would always override
There was a problem hiding this comment.
Oh, Okay. I'll figure out another way.
| @@ -73,7 +80,12 @@ class HomeWidgetPlugin : FlutterPlugin, MethodCallHandler, ActivityAware, | |||
| val value = prefs.all[id] ?: defaultValue | |||
|
|
|||
| if(value is Long) { | |||
| result.success(java.lang.Double.longBitsToDouble(value)) | |||
| // The reason for falling back to Double is that it was supported before Long. | |||
| when (prefs.getString(ACTUAL_WIDGET_DATA_TYPE, "Double")) { | |||
There was a problem hiding this comment.
I believe the old logic should have already covered the logic. Looking at the code I also think that there is no benefit in this extra logic.
home_widget already handled this behavior by storing handling doubles always as long. so in that way we trade 2 bytes per number for the behavior.
So can you please revert your changes to the initial fix (adding is Long to the setWidgetData call)
|
Dart double is not Kotlin Long. Dart int can be a Kotlin Long. e.g) Dart.now.millisecondsSinceEpoch. |
|
Thanks for the Explanation! I have looked further into this and possible solutions and considered it quicker to open the fix for this in #280 and will close this PR for this. Thanks though for Investigating and Proposing this first Fix!! |
resolved: #269