search

tjarvstrand / flutter_timezone

A fork of https://github.com/pinkfish/flutter_native_timezone
Apache License 2.0
42 stars 29 forks source link

Use desugaring with AGP update #5

Closed MaikuB closed 1 year ago

MaikuB commented 1 year ago

Whilst looking at some of the recent changes to the fork given the original plugin is now longer maintained, I realised it's got code that does Android version checks to see if it should the newer Java time APIs or not. With desugaring, it's possible now to get rid of this logic so it only uses the newer APIs thatI believe this would bring more consistency. Side effect of this is that AGP gets bump as it needs to be on 7+ to fix this issue that can occur with building using AGP 4.x. Going to 7+ requires bumping the minimum Flutter SDK constraint as the Flutter tooling will otherwise complain about using an unsupported Gradle version

Also removed references to Android v1 embedding as it's no longer needed

Note: my YAML formatting settings switched the pubspec.yaml file to use double quotes automatically on hitting save. Feel free to revert this if single quotes is preferred. Of course, feel free to reject this PR too if you disagree with the changes

tjarvstrand commented 1 year ago

Hey there! Thanks for the effort. As it stands however, I'm not sure what benefits this would bring for the user?

The code might look a little nicer but even with the if-statement, the Android platform code is very simple. I'm hesitant to add another step to integrate the plugin if it isn't strictly necessary and/or brings tangible improvements.

MaikuB commented 1 year ago

The if statements weren't the focus of this PR, they were more symptomatic on the same API isn't used across all Android versions so this is why brought up how the PR would bring consistency. This is likely something you may need to look more into but as I understand it, both APIs used had different focuses. The older TimeZone isn't officially documented to try to adhere to standards that the timezone IDs from the IANA timezone database but the newer ZoneId is. This means, in theory, one may see different results across different Android versions given they are different APIs. Not exactly certain that takes place but not a stretch to think this could happen. Perhaps desugaring does map ZoneId to TimeZone but I haven't checked.

Based on what you said though, it also leads to asking why bother using the ZoneId API? I know it's a fork so you may not the original context but if the choice is made to not use ZoneId then why not use TimeZone irrespective of the Android version? From what I saw, it's not deprecated

tjarvstrand commented 1 year ago

Based on what you said though, it also leads to asking why bother using the ZoneId API?

Fair question. I'm not terribly familiar with the details but it seems that TimeZone could potentially return an ambiguous 3-character short code instead of a unique zone-ID. For my own particular use case, I'm afraid this is a deal breaker.

Perhaps desugaring does map ZoneId to TimeZone but I haven't checked.

Indeed, it seems that the desugared API uses TimeZone under the hood: https://github.com/google/desugar_jdk_libs/blob/master/jdk11/src/java.base/share/classes/java/time/ZoneId.java#L272

In light of this I will reject this PR for now. The change introduces some additional complexity and as of now, I still don't think it brings much value. However, I really appreciate you taking the time and I'd be happy to revisit in the future if circumstances change.