MaikuB
Consider switching to Pigeon for Android
There's a lot of Java and Dart code around parsing/serializing/deserializing notifications, details, and options. I'd be interested in trying out pigeon, which would generate all that based on a few Dart files. That should get rid of lots of hand-written code, and prevent the need for hand-writing parsing code for future features.
If this works, it should be relatively trivial to use Pigeon for iOS/MacOS as well.
I'm working at this now, see #2507. I haven't done much testing yet, but here are some Pigeon issues to keep an eye on:
- https://github.com/flutter/flutter/issues/160827
- https://github.com/flutter/flutter/issues/117819
- https://github.com/flutter/flutter/issues/66550
- https://github.com/flutter/flutter/issues/160829
- https://github.com/flutter/flutter/issues/160830
Until some of these issues are resolved, the best way to integrate Pigeon for Android, iOS, and MacOS would be to write platform-specific Pigeon files and map the existing platform interface classes to the respective platform-specific classes. For example, instead of trying to convert NotificationAppLaunchDetails to Pigeon, it's better to make AndroidNotificationAppLaunchDetails and DarwinNotificationAppLaunchDetails, converting as necessary in the platform-specific plugin classes
It's also worth looking into using JNI to call Java functions directly. That would mean not needing to write Pigeon code at all, but it would also mean implementing FFI for iOS and MacOS separately, whereas Pigeon can handle both in the short-term.
I've not taken a look yet but something to call out that may have been missed is that on the Android side, there are classes used to so that notification details can be serialised via GSON and saved to shared preferences. Some of the code around this also went through changes and needed to account for backwards compatibility. I know you have a task that says ensure that are no breaking changes but wanted to mention it could be easy to miss this one. IMO it's the area with the highest risk and is one of the reasons why I've not looked at porting
I was mainly considering not breaking the API, so this is good to keep in mind.
Seems like the only class being used with GSON is NotificationDetails... that's quite a big one though. It would be a shame to have to keep that -- and all the classes it depends on --- in Java even though Pigeon can generate them for us.
Instead of trying to shape the Pigeon code to exactly match the old GSON output, we can follow the GSON docs on custom serialization to define how they should be converted to JSON in a backward-compatible way. This will also make the JSON format a bit more explicit, which is a benefit in my eyes if that needs to remain backward-compatible. Doing so can also open the door to switching to Kotlin in the future.
After some discussions on the Flutter repo and Discord. I was mistaken in my initial assessment. I was under the impression that, like Protobuf/gRPC, we should be using Pigeon for public APIs, but that is strongly discouraged. Instead, Pigeon should be treated as a purely internal tool to reduce the friction of using method channels.
So I'll likely be closing my PR and opening a new one with this new mindset. That means a lot of duplicate classes and conversions, but the Pigeon API won't need to be convenient for users, so I'm hopeful it can be still be relatively condensed. And it won't be any more wasteful than the hand-written conversions that exist today.
Thanks for the research. Putting that aside, I was going to say it's not just about lines of code that should considered. Introducing Pigeon effectively means everything needs to be retested as all method channel calls would impacted. That can provide enough reason to not consider going down this path as it depends on personal time to develop and test. In other words, I don't believe the cost of change is worth it.
I've also had issues in the past where prereleases had been active for a significant amount of time and only once a stable release was done did an issue get raised that should've been picked up early. Based on that, this indicates not a lot of the community use prereleases to help test. Pub is still missing download counts based on version that could've help boost confidence in this area and is something I've raised already
It's a time-consuming change, but once tested, no other method channel code would have to be tested on any platform, increasing confidence in future features. In that sense, I think it's at least worth investigating -- when I have the time, which won't be for another month or so. But you're right that there will be no immediate benefits. More analytics would definitely help though.
Unsolicited thoughts: if there is willingness to do it, a switch over to Pigeon would be well worth the effort over the long run. I did this twice, once for the native_geofence plugin and again for the alarm plugin.
@MaikuB I thought of you while reading the 3.7 release! Pub now has statistics how many users are using which versions of your package: https://medium.com/dartlang/announcing-dart-3-7-bf864a1b195c