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

iOs crash with date nil with zoned schedule #1950

Closed
youssefali424 opened this issue Apr 9, 2023 · 9 comments · Fixed by #1953
Closed

iOs crash with date nil with zoned schedule #1950

youssefali424 opened this issue Apr 9, 2023 · 9 comments · Fixed by #1953

Comments

@youssefali424
Copy link
Contributor

Describe the bug
Choosing specific date crashes on specific regions leads to failure in parsing dates on iOs using "yyyy-MM-dd'T'HH:mm:ss

To Reproduce

  1. set timezone to Africa/cairo
  2. init package normally
  3. zonedschedule 28/4/2023 at 12 am
  4. date fails to parse the given date and nil date sent to notification trigger
  5. app crashes

example using latest package: (was tested on 13.0.0 and 14.0.0)
https://github.com/youssefali424/local_notification_test

Expected behavior
to not crash the app

Sample code to reproduce the problem

Sample code
import 'dart:convert';

import 'package:flutter/material.dart';
import 'package:flutter_local_notifications/flutter_local_notifications.dart';
import 'package:timezone/timezone.dart' as tz;
import 'package:timezone/data/latest_all.dart' as tz;

FlutterLocalNotificationsPlugin flutterLocalNotificationsPlugin =
    FlutterLocalNotificationsPlugin();
Future<void> main() async {
  WidgetsFlutterBinding.ensureInitialized();
  tz.initializeTimeZones();
  tz.setLocalLocation(tz.getLocation("Africa/Cairo"));
  const AndroidInitializationSettings initializationSettingsAndroid =
      AndroidInitializationSettings('app_icon');
  const DarwinInitializationSettings initializationSettingsDarwin =
      DarwinInitializationSettings(
    requestSoundPermission: true,
    requestBadgePermission: true,
    requestAlertPermission: true,
  );

  const InitializationSettings initializationSettings = InitializationSettings(
    android: initializationSettingsAndroid,
    iOS: initializationSettingsDarwin,
    macOS: initializationSettingsDarwin,
  );
  await flutterLocalNotificationsPlugin.initialize(
    initializationSettings,
    onDidReceiveNotificationResponse:
        (NotificationResponse notificationResponse) async {},
  );
  runApp(const MyApp());
}

class MyApp extends StatelessWidget {
  const MyApp({super.key});

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      title: 'Flutter Demo',
      theme: ThemeData(
        primarySwatch: Colors.blue,
      ),
      home: const MyHomePage(title: 'Flutter Demo Home Page'),
    );
  }
}

class MyHomePage extends StatefulWidget {
  const MyHomePage({super.key, required this.title});
  final String title;
  @override
  State<MyHomePage> createState() => _MyHomePageState();
}

class _MyHomePageState extends State<MyHomePage> {
  void _incrementCounter() {
    var iOSDetails = const DarwinNotificationDetails(
      presentAlert: true,
      presentBadge: true,
      presentSound: true,
    );
    AndroidNotificationDetails androidDetails =
        const AndroidNotificationDetails(
            'eve', 'Fired on event time with custom sound',
            autoCancel: true,
            channelShowBadge: true,
            enableLights: true,
            importance: Importance.max,
            priority: Priority.high,
            playSound: true,
            icon: "@mipmap/ic_launcher");

    var notificationDetails =
        NotificationDetails(android: androidDetails, iOS: iOSDetails);
    flutterLocalNotificationsPlugin.zonedSchedule(
      1,
      "test",
      "test",
      tz.TZDateTime(tz.local, 2023, 4, 28),
      notificationDetails,
      androidAllowWhileIdle: true,
      uiLocalNotificationDateInterpretation:
          UILocalNotificationDateInterpretation.absoluteTime,
    );
  }

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: Text(widget.title),
      ),
      body: Center(
        child: Column(
          mainAxisAlignment: MainAxisAlignment.center,
          children: <Widget>[
            const Text(
              'Add notification:',
            ),
          ],
        ),
      ),
      floatingActionButton: FloatingActionButton(
        onPressed: _incrementCounter,
        tooltip: 'Increment',
        child: const Icon(Icons.add),
      ), // This trailing comma makes auto-formatting nicer for build methods.
    );
  }
}
@youssefali424
Copy link
Contributor Author

@MaikuB "dateComponents must not be nil" ,I saw an old issue about this but I don't think it is related
the only thing I could do currently was to avoid having this issue when date is nil
but I am trying and exprimenting more I don't know what is the issue yet

@youssefali424
Copy link
Contributor Author

my guess this happens because some weird 12 or 24 hours conversion with HH being "00" changing the hour fixes the issue
i only noticed this by pure chance on Crashlytics and it was a hell ride trying to guess what is the problem

@MaikuB
Copy link
Owner

MaikuB commented Apr 10, 2023

It's not an issue with parsing "00" as this works fine with other timezones. If you check https://www.timeanddate.com/time/change/egypt/cairo, your issue is most likely because daylight savings takes place exactly at that time where the clock will move forward an hour. This means the specified time is "non-existent" as it'll be 1am so I believe Apple's APIs return nil to represent this. It does work if 1am is specified and this looks like it would be the only solution that is likely only specific to iOS and macOS. From the plugin's side, this will be patched to trigger an error so at least it can be caught and have an explanation attached

@MaikuB
Copy link
Owner

MaikuB commented Apr 10, 2023

If you happen to know of a different solution, please do submit a PR but the one does the changes I spoke of can be found at #1951. Haven't merged it in and done a release to include this yet in case there are better solutions that I'm not aware of

@youssefali424
Copy link
Contributor Author

sorry for the late response
there is a solution i came up with
to change the parsing of the date and use iso 8601 completely
changes done here but won't submit a pr with this change , i was experimenting so the commit has other changes
master...youssef4242014-gmail-com:flutter_local_notifications:master

    NSISO8601DateFormatter *dateFormatter =  [[NSISO8601DateFormatter alloc] init];
    [dateFormatter setTimeZone:timezone];
    dateFormatter.formatOptions = NSISO8601DateFormatWithFractionalSeconds | NSISO8601DateFormatWithInternetDateTime;
    NSDate *date = [dateFormatter dateFromString:scheduledDateTime];
    if(date == nil) return nil;

in dart send the full iso 8601 date to native side

@youssefali424
Copy link
Contributor Author

yes I thought of that it is weird really , I am from Egypt and we didn't have a daylight saving since years
but how NSISO8601DateFormatter can work with this but the normal formatter can't ?

@MaikuB
Copy link
Owner

MaikuB commented Apr 10, 2023

Thanks will need to investigate this more but sounds like my PR won't be needed then. It is better that there's a consistent way for the plugin to work on all platforms. Thought about the other formatter with ISO 8601 string but didn't get around to it. Not sure why the regular formatter doesn't work either. Perhaps Apple stopped maintaining it or that it fails as it's "confused" on how to parse the date and time in this scenario. If using the ISO 8601 is the way to go then will need to be careful that this doesn't break existing notifications for Android as the existing format is what gets saved to shared preferences to reschedule on reboot. It might be best create a separate variable for the date/time in ISO 8601 that is used only by iOS and macOS. By that I mean, to create an additional entry in the dictionary around

where the key would be called, say, scheduledDateTimeISO8601 and that is what gets read on at least iOS and macOS. It may be possible to change Android to read from there going forward by adding a field etc to this class and remove the dictionary entry for scheduleDateTime but would still need to keep this line and the related logic for backwards compatibility. Would you by chance be up for creating a PR on this to the repo? Note that rather than committing straight to master on your fork, it is better that you create a separate branch as this allows master to be reserved for keeping up to date with the latest changes from the original repo

@youssefali424
Copy link
Contributor Author

yes i understans yes i will do that

@youssefali424
Copy link
Contributor Author

youssefali424 commented Apr 10, 2023

#1953
@MaikuB this is the fix tried it with the examples and the example that I created above that shows the crash and all good
please review if you have some free time

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment