Feature/notifications

[micahmo]

Pull Request Description

This PR introduces OS notifications for inbox messages. All message checking and notification generation is done locally (i.e., no Firebase needed).

New Packages

• flutter_local_notifications
  • Has the ability to generate local notifications, manage permissions, etc.
• background_fetch
  • Allows the app to periodically run a process in the background (in this case, polling for new messages), even when closed.

Notes

The following is a list of caveats and/or intentional limitations for this first round of implementation. Most of these things can be improved in the future, but this first round works great as is!

• I only set this up for Android. Perhaps @hjiangsu could do the iOS setup at some point, but it requires a bit of work for both packages. (flutter_local_notifications setup and background_fetch setup).
• Enabling notifications will most likely increase battery usage due to the background checks. This is noted in the new setting description.
• There are some "Release build configuration" changes suggested by flutter_local_notifications that I did not go through. I'd be curious to know if it works as-is (since it works fine in Debug), but we can test together.
• Clicking a notification only brings you to inbox page. It does not specifically focus the relevant message.
  • My idea to fix this in a future iteration is to add a new page which can display a single message and be shown on top of the navigation stack. This allows you to interact with the message, then go back to whatever you were doing in Thunder. (Right now, the page scroll to the Inbox page doesn't work so well if you're looking at a post, for example.)
• Notifications only work for replies, not mentions or messages.
• Notifications only work for the active account in Thunder.
• The interval of background_fetch is not super configurable (at all on iOS, and not without more permissions on Android). But I think the default of 15(-ish) minutes is fine!
  • For the sake of the demo video, the polling time was reduced. The code includes comments on how to do this.

Issue Being Fixed

Issue Number: First step / partial implementation of #125 and #219.

Screenshots / Recordings

Demo of permissions flow

permissions_flow.mp4

Demo of receiving notifications while app is open

notifs_with_app_running.mp4

Demo of receiving notifications while app is closed

notifs_without_app_running.mp4

Checklist

• Did you update CHANGELOG.md?
• Did you use localized strings where applicable?
• Did you add semanticLabels where applicable for accessibility?

[hjiangsu]

I would just like to say that this is awesome! I was also looking at ways to have notifications without having to host a server holding JWT tokens (for security purposes)

I'll definitely take a look at this when I have time and see if it can be adapted to iOS as well 😄

[micahmo]

I would just like to say that this is awesome! I was also looking at ways to have notifications without having to host a server holding JWT tokens (for security purposes)

Thanks, yeah, this seems to be a good enough solution, as long as we're ok with Thunder making the occasional API call!

If you'd like to try this out on your Android emulator to get a feel for how it works, I'd recommend a couple of changes to make testing easier.

• Change this value to 1.
  

  thunder/lib/main.dart

  Line 237 in 04cd9d7

  minimumFetchInterval: 15,

• Uncomment this line.
  

  thunder/lib/main.dart

  Line 247 in 04cd9d7

  //forceAlarmManager: true,

• Comment out the where part of this line.
  

  thunder/lib/utils/notifications.dart

  Line 57 in 04cd9d7

  final Iterable<CommentReplyView> newReplies = getRepliesResponse.replies.where((CommentReplyView commentReplyView) => commentReplyView.comment.published.isAfter(lastPollTime));

• Mark some messages as unread or change this to false.
  

  thunder/lib/utils/notifications.dart

  Line 49 in 04cd9d7

  unreadOnly: true,

With those things set, you should get some notifications for some/all inbox messages about once per minute whether the app is running or not!

[hjiangsu]

So I've been looking into the packages, especially background_fetch, and it seems like there might be some inconsistencies in how this works (depending on platform and manufacturer)

For iOS, background fetching will not work when the app is terminated. This means that we can't really use this package reliably to fetch for notifications every ~15 mins. This will only work if the app is either in the foreground, or when its not terminated (in the background)

For Android, it seems like the reliability of background fetching varies depending on the device manufacturer (https://dontkillmyapp.com/). There seems to be some workarounds for the specific manufacturers but each one is different.

• [BUG]not working when app is terminated on android transistorsoft/flutter_background_fetch#355
• Headless task not working on actual device transistorsoft/flutter_background_fetch#339

[micahmo]

For iOS, background fetching will not work when the app is terminated.

Oh man I missed that in the docs. That is a real bummer. 😞 And I suppose this is a limitation of iOS in general, not background_fetch, so no other package would be any better.

To be honest, I don't have any other ideas for local notifications, and I'm pretty sure you're not wanting to set up a server for this.

I suppose we could add a warning for iOS that the app must be open or in the background for notifications to work. We can have a similar but less severe warning for Android, to handle different manufacturers.

I've been daily driving this branch since I opened the PR, and having notifications is just so handy. I would love to be able to add this, even if it's not quite as useful across-the-board as I was hoping.

Let me know what you think!

[hjiangsu]

Yeah, unfortunately, I think the only viable solution on iOS would be to set up a server to handle push notifications. I could play around with it a bit and see how that would work, but needing to have the JWT token stored on the server in order to fetch notifications feels like a big security risk.

There might be a potential for self-hosting the server which reduces the security risk for those who are able to do so, but I don't know how that interacts with the push notification services (whether or not that's even possible)

I think we can still go ahead and push this local notifications since its really useful to have, but we should be clear that it's experimental and might not work on all Android devices (and provide some instructions on what to do to prevent the app from being killed in the background)

[micahmo]

we should be clear that it's experimental and might not work on all Android devices (and provide some instructions on what to do to prevent the app from being killed in the background)

Any recommendations for how we should communicate this? Maybe a popup warning when enabling the feature?

[hjiangsu]

I think adding "Experimental" to the subheading of the option works well to let the user know that it may not fully work. We can also have a popup dialog which explains how the notifications work so that its more transparent

• Only happens on ~15 mins intervals, disable battery optimizations to give it the best chance to work properly, etc.

[micahmo]

I think adding "Experimental" to the subheading of the option works well to let the user know that it may not fully work. We can also have a popup dialog which explains how the notifications work so that its more transparent

Sounds good! How does this look?

qemu-system-x86_64_gsL8oINwv4.mp4

Here's a screenshot of just the dialog so you can read it.

[hjiangsu]

Sounds good! How does this look?

Thanks, that looks good to me! It'll take some time for me to review this PR still, since as you mentioned, there are some release build configuration that needs to be done before it can be used for release builds (it seems like it has to do with the build process and ensuring that the build process does not remove resources needed for the notifications package to function properly)

Once I look through the PR, and set up the release build configurations, I'll most likely just generate a release build for you to test out if you'd like, to ensure that everything still works as expected on Android's end!

[micahmo]

It'll take some time for me to review this PR

No problem at all! I really appreciate that you're willing to review and accept this feature. Take all the time you need.

I'll most likely just generate a release build for you to test

Sounds good!

[hjiangsu]

I've pushed some commits to update with the latest develop changes, and also to add in release build configuration! I can create a release build for you to test out if you'd like (I can send it over matrix)

I also did a quick code review and had some comments!

[hjiangsu]

I would place all this logic into its own function (e.g., similar to _initPreferences). This way, you can perform the async operations!

[micahmo]

Sure thing, I refactored this section to its own method. Although I'm not sure exactly what you mean by "This way, you can perform the async operations!".

[hjiangsu]

Ahh, I think I misinterpreted the comment. We can do something like this so that it doesn't mutate areAndroidNotificationsAllowed directly:

Although in this case, it might be fine!

bool _areAndroidNotificationsAllowed = await androidFlutterLocalNotificationsPlugin?.areNotificationsEnabled();

setState(() {
  areAndroidNotificationsAllowed: _areAndroidNotificationsAllowed,
});

[micahmo]

Ok I updated it to do this! I think the end result is the same (the UI is redrawn with the current state), but I agree that this provides a clearer intention as to why setState is being called at all.

[micahmo]

I've pushed some commits to update with the latest develop changes, and also to add in release build configuration! I can create a release build for you to test out if you'd like (I can send it over matrix)

I also did a quick code review and had some comments!

Thank you so much for reviewing and adding the release configs! I've addressed your PR comments. If/when you get a chance, feel free to shoot over a release build in Matrix and I'll see how things work!

[micahmo]

@hjiangsu Notifications have been working fine in the build you sent me, so I think this is good to go!