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

Device Calendar Sync #2072

Merged
merged 8 commits into from
Aug 20, 2024
Merged

Device Calendar Sync #2072

merged 8 commits into from
Aug 20, 2024

Conversation

gnunicorn
Copy link
Contributor

@gnunicorn gnunicorn commented Aug 16, 2024

This PR brings syncing of all events not-denied to your device calendar on Android & iOS. See it in Action (the other app is the emulator Default Calendar App from Google):

device-calendar-integration-zero.mp4

It is behind a feature flag, that you can hard toggle to see and remove the calendar:

labs-flag.mp4

And on iOS (emulator) as well:

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-08-20.at.10.55.26.mp4

The following caveats apply:

  • It's behind a default-off feature flag
  • will only sync events you with rsvp status unknown, yes and maybe
  • Every event is added with a reminder of 10mins before (maybe we only want to do that for anything that is in yes?)
  • have this and the notifications on brings two annoying popup requests at the beginning
  • if the user refuses the permission we won't ask again.
  • there is no background sync, it needs the app to run (occasionally)
  • all rsvp status and event details changes are observed via riverpod and synced immediately (as the video above shows)
  • however, edits from the calendar by the user will be ignored
  • needs to be tested on an actual iOS

To the reviewer
Next to the actual feature the following refactors were necessary:

  1. 8ba4b58 refactors the labFlag as it wasn't actually loading on startup, or at least wasn't fast enough for us to notice in loading. This refactors this to allow us to have a provider we can actually await for as well as make sure that the data was submitted before going for initialize or it would fail. That it works shows the video above.
  2. 863e40e refactors the home_shell into an app_shell that is moved to config as it is about setting up the general infrastructure. Aside from moving the file and outsourcing the LogoutWidget (which is otherwise features wise untouched), this moves the _init process to give us more executive control, like in this case make sure we are not asking for multiple permissions at the same time but after one another (notifications & calendar permission).

@gnunicorn gnunicorn added s-integration All about external integrations and bridges merge post release only Merge this only after the new release has been cut labels Aug 16, 2024
Copy link
Contributor

@kumarpalsinh25 kumarpalsinh25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very existed about this feature!
Great Job! Code overall looks good to me.

);
}

Future<void> _refreshCalendar(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skipping review of _refreshCalendar as not getting much idea of the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a shame, as this is the main part of the code ;)

if (calendars == null) {
return [];
}
if (Platform.isAndroid) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for this platform specific condition?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, due to platform specific behaviors (iOS doesn't have any accountName property) this will only be correct on Android.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

had to update this slightly for iOS support. But nothing major. Also limiting our deleting to local calendars so we don't accidentally delete others ... like one they might have named "Acter" in their google calendar.

@gnunicorn
Copy link
Contributor Author

I take it.

@gnunicorn gnunicorn merged commit 5b995f5 into main Aug 20, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infrastructure merge post release only Merge this only after the new release has been cut s-calendar Calendar, events & meetings s-integration All about external integrations and bridges
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants