notif: Switch to our own Pigeon-based plugin for showing a notification #592
Conversation
|
|
Ah thanks. The analyze failure is this issue I reported upstream in Pigeon: but that I then forgot about when sending this. I'll work around probably by just disabling the lint globally; I doubt many cases of it will accumulate before someone (possibly us) gets around to fixing that Pigeon issue. The build_runner failure looks like I need to tweak that check to know that not all I'll fix those, but meanwhile I think all the existing code in the PR is ready for review. |
|
Fixed those issues — CI is now passing. |
|
Cool! It's exciting to start using Pigeon. In manual testing on the office Android device (Samsung Galaxy S9 running Android 9), I don't seem to be getting notifications when the app is closed. They do appear when the app is open and foregrounded, or open in the background. I'm not sure if this behavior is new with these commits, though. I'll check and see what's happening on |
|
Ah yeah, I see the same thing happening on |
|
Yeah, I actually saw similar behavior when I was testing this yesterday, and similarly determined it was also present on main :-/ I have a local note to myself but hadn't gotten to investigating further. It's probably yet another distinct cause from #342, but definitely a similar symptom. One potential mitigating factor is that I think I've only seen that in debug builds, not when using |
|
Oh, FWIW the issue seems to have stopped reproducing for me. I'm still on
Did you happen to notice something similar? |
|
Anyway, the issue is present in |
Yeah, the issue has been flaky in my observation — sometimes it reproduces, sometimes not. That's the main reason I'm not confident in the "only debug builds" pattern I've observed.
Yeah, this is exactly the sequence that I did many times in the course of developing these changes (repeating it after various changes to the branch, to check that things were still working). I suspect a trigger for it might be that closing the app is taken as a bad sign by the system — the user force-closed the app! maybe it was misbehaving for them — and so because I was doing it again and again, the system eventually decided the app shouldn't be trusted with the privilege of starting up in the background. If so, then that'd be another mitigating factor in that it's not likely to happen with users who aren't developing the app. |
This will let us split this file in two.
For details, see 93741d7 where we made these sorted.
For the moment the new plugin is an empty shell, doing nothing. We'll add functionality to it shortly. The reason we need this somewhat complicated arrangement is so that the `flutter` tool sees this plugin and includes it in some code it generates, in order to cause the Flutter engine to automatically initialize the plugin on startup. For details, see the comment this commit includes in the new shim package's pubspec.
This doesn't yet do anything, as we have no pigeons yet. We'll add one shortly.
This will keep `tools/check build_runner` passing when we start adding Pigeon generated files.
For the plugin setup logic, we crib from UrlLauncherPlugin.
This delivers the first major tranche of zulip#351, switching from package:flutter_local_notifications to our own thin API binding based on Pigeon. With this commit, we've converted over one of the handful of different methods we use from that package, and it's the most complex of them by a substantial margin. Fixes-partly: zulip#351
|
Merged! Thanks for the review. |
|
And filed #597 for that issue where notifications sometimes don't appear. |
This delivers the first major tranche of #351, switching from
package:flutter_local_notifications to our own thin API binding
based on Pigeon. In particular we convert over one of
the handful of different methods we use from that package,
and it's the most complex of them by a substantial margin.
Getting this to work involves a bit of tricky, but fortunately
one-time, infrastructure: a shim extra Flutter plugin in order
to get our new Flutter plugin properly wired up.
Fixes-partly: #351
/cc @rajveermalviya