Skip to content
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

Consider switching to Pigeon for Android #2504

Open
Levi-Lesches opened this issue Dec 24, 2024 · 3 comments
Open

Consider switching to Pigeon for Android #2504

Levi-Lesches opened this issue Dec 24, 2024 · 3 comments

Comments

@Levi-Lesches
Copy link
Contributor

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.

@Levi-Lesches
Copy link
Contributor Author

Levi-Lesches commented Dec 24, 2024

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:

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.

@MaikuB
Copy link
Owner

MaikuB commented Dec 24, 2024

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

@Levi-Lesches
Copy link
Contributor Author

Levi-Lesches commented Dec 24, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants