Baseflow / flutter-geolocator

Android and iOS Geolocation plugin for Flutter
https://baseflow.com/
MIT License
1.24k stars 652 forks source link

Geolocator.getCurrentPosition() without prior permissions granted stops future builder from waiting #661

Closed lukaglo closed 3 years ago

lukaglo commented 3 years ago

🐛 Bug Report

I am using FutureBuilder in building my HomePage, like below:

return FutureBuilder(
                future:  Geolocator.getCurrentPosition(),
                builder: (context, positionSnapshot) {
...

If the app has not previously obtained permissions it will ask for permissions, however, after granting the permissions, the FutureBuilder gets stuck not resolving the position future. The message I get in debugging is:

Flutter location permissions not part of permissions send to onrequestpermissionsresult method.
Can request only one set of permissions at a time.

To correct the issue I had to separate the permission checking process from getting location. In fact it had to be moved to a completely separate part of the app logic (while the user logs in).

Please note - even putting the permissions check inside a custom position future did not work:

//even in the approach below the future gets stuck ...
Future<Position> getUserPositionWithPermission() async {
  Position position;
  try {
    var _permissionGranted = await Geolocator.checkPermission();

    if (_permissionGranted == LocationPermission.denied ||
        _permissionGranted == LocationPermission.deniedForever) {
      await Geolocator.requestPermission();
    }

    position = await Geolocator.getCurrentPosition();

    return position;
  } on Exception catch (e) {
    debugPrint('Geolocator.getCurrentPosition failed! - $e');
    rethrow;
  }
}

Expected behavior

The future should not get stuck after obtaining permissions in one sequence. Splitting permissions from getting location into separate parts of the app logic is not optimal.

Reproduction steps

The app has to be 'fresh' with no permissions granted

Configuration

Android 7.0 geolocator: ^6.2.0 Flutter 1.22.6

Platform:

mvanbeusekom commented 3 years ago

@lukaglo the problem with your setup is that your build method is called multiple times in rapid succession. This causes the problem the getCurrentPosition method is also called multiple times and Android doesn't allow you to request permissions simultaneously. The error message you receive is Android's way of telling you: "You are already requesting permissions, please wait for that to finish before requesting permissions again".

You probably didn't realise this is the result of the build method being executed multiple times by the Flutter framework. You can workaround this by initialising the Future outside the build method and then assign the variable containing the Future to the FutureBuilder. For example:

class MyWidget extends StatelessWidget {
  final Future<Position> _positionFuture = Geolocator.getCurrentPosition();

  Widget build(BuildContext context) {
    return FutureBuilder(
      future: _positionFuture,
      builder: (context, snapshot) {
        // Build your widget here...
      },
    );
  }
}

Some more information on why build can be called multiple time can be found in these articles:

Let me know if this helps.

mvanbeusekom commented 3 years ago

I will close the issue for now, if you have any questions feel free to leave a comment, I would be happy to re-open the issue.

lukaglo commented 3 years ago

Hello - thanks a lot. This seems such a simple issue yet I did not succeed. I declared the Future before the widget build as you suggested but still get this:

W/Activity(24892): Can reqeust only one set of permissions at a time
W/Geolocator(24892): Location permissions not part of permissions send to onRequestPermissionsResult method.

If you could enlighten me on where else to look I would be most grateful. Thanks in anticipation.

mvanbeusekom commented 3 years ago

Is it possible to share the code of the widget? Are you also requesting the location in another widget (or are you reusing the same widget multiple times in the same view)?

lukaglo commented 3 years ago

Hello, I think you are right about your hypothesis that getCurrentPosition() is being accessed by various parts of the app at the same time. In fact it is used by several other parts of the app on its initialisation. So maybe there needs to be a queueing mechanism introduced especially on the initialization? I currently use a method like the one below. So maybe I really need to convert it to a Cubit? Anyway - I have a workaround. I check and request permissions on user' login. This works fine so please do not take too much time to analyse this ... Thanks a lot!

Future<Position> storedOrGeoLocation({int minutesOld}) async {
  var lastUpdatedPosition = DateTime.fromMillisecondsSinceEpoch(0);
  Position _position;
  try {
    var prefs = await SharedPreferences.getInstance();
    var updateTimeExists = prefs.containsKey('user_pos_last_update');
    if (updateTimeExists) {
      lastUpdatedPosition =
          DateTime.parse(prefs.getString('user_pos_last_update'));
    }

    if (-timeToEndSeconds(expiryDateTime: lastUpdatedPosition) <
        minutesOld * 60) {
      if (prefs.getString('last_lat') != null &&
          prefs.getString('last_lng') != null &&
          prefs.getString('last_lat') != '' &&
          prefs.getString('last_lng') != '') {
        _position = Position(
          latitude: double.parse(prefs.getString('last_lat')),
          longitude: double.parse(prefs.getString('last_lng')),
        );
      } else {
        _position = await Geolocator.getCurrentPosition();
             }
    } else {
      _position = await Geolocator.getCurrentPosition();
          }
  } on Exception catch (e) {
      rethrow;
  }
    return _position;
}

Here is the code of the widget. If you are interested - it's long and winded ... ;)

import 'package:flutter/material.dart';
import 'package:flutter_bloc/flutter_bloc.dart';
import 'package:flutter_gen/gen_l10n/app_localizations.dart';
import 'package:geolocator/geolocator.dart';

import '../../cubits/job_list/job_list_cubit.dart';
import '../../cubits/realizations_manager/realizations_manager_cubit.dart';
import '../../cubits/user_auth/user_auth_cubit.dart';
import '../../models/job_description_and_list/job_description_and_list_model.dart';
import '../screen_elements/display_error_page.dart';
import '../screen_elements/display_loading_page.dart';
import '../screen_elements/side_drawer_list.dart';
import 'user_auth_screen.dart';

class HomePage extends StatefulWidget {
  static const String routeName = '/home-page';
  @override
  _HomePageState createState() => _HomePageState();
}

class _HomePageState extends State<HomePage> {
  static const double _desiredSplit = 0.6;
  double currentSplit = _desiredSplit;
  static const double _minMapFragment = 0.04;
  double listTopBarHeight = 42.0;

  //future to get user location
  final Future _userLocationFuture = Geolocator.getCurrentPosition();
  //storedOrGeoLocation(minutesOld: 5);

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: Text(AppLocalizations.of(context).appName),
        actions: [
          IconButton(
              icon: const Icon(Icons.refresh),
              onPressed: () {
                context.read<JobListCubit>().getJobList();  //USES LOCALIZATION
              }),
        ],
      ),
      drawer: Drawer(
        elevation: 16,
        child: SideDrawerList(),
      ),
      backgroundColor: Colors.white,
      body: LayoutBuilder(builder: (context, constraints) {
        // layout preparation -->
        final availableHeight = constraints.maxHeight - listTopBarHeight;
        final minMapHeight = availableHeight * _minMapFragment;
        final maxHeight = availableHeight - minMapHeight;
        var mapHeight = maxHeight * (1 - currentSplit) + minMapHeight;
        //to leave some space for the map always!
        var listHeight = maxHeight * currentSplit;
        // layout preparation <--

         return BlocConsumer<UserAuthCubit, UserAuthState>(
            listener: (context, authState) {
          if (authState is! UserAuthLogged) {
            Navigator.of(context)
                .pushReplacementNamed(UserAuthScreen.routeName);
          }
        },
            builder: (context, authState) {
          if (authState is! UserAuthLogged) {
            return DisplayLoadingPage(
                message: AppLocalizations.of(context).youHaveToLogIn);
          } else if (authState is UserAuthLogged) {
            return FutureBuilder(
                future: _userLocationFuture,
                builder: (context, positionSnapshot) {
                  if (!positionSnapshot.hasData) {
                    return DisplayLoadingPage(
                      message: AppLocalizations.of(context)
                          .downloadingLocationDataMessage,
                    );
                  } else {
                    return BlocConsumer<JobListCubit, JobListState>(   //USES LOCALIZATION
                      listener: (context, jobListState) {
                        if (jobListState is JobListError) {
                          Scaffold.of(context).showSnackBar(
                            SnackBar(
                              content: Text(jobListState.message),
                            ),
                          );
                        }
                      },
                      builder: (context, jobListState) {
                        if (jobListState is JobListLoaded) {
                          context
                              .read<RealizationsManagerCubit>()
                              .checkIfRelizationOnJoblistBooked();
                          context
                              .read<RealizationsManagerCubit>()
                              .updateRealizationsManagerState();

                          return buildHomePage(
                            listHeight: listHeight,
                            mapHeight: mapHeight,
                            latitude: positionSnapshot.data.latitude,
                            longitude: positionSnapshot.data.longitude,
                            jobList: jobListState.jobList,
                          );
                        } else if (jobListState is JobListLoading) {
                          return DisplayLoadingPage(
                            message: AppLocalizations.of(context)
                                .downloadingJobListMessage,
                          );
                        } else if (jobListState is JobListInitial) {
                          context.read<JobListCubit>().getJobList();
                          return DisplayLoadingPage(
                            message: AppLocalizations.of(context)
                                .downloadingJobListMessage,
                          );
                        } else if (jobListState is JobListError) {
                          return DisplayErrorPage(
                            message: jobListState.message,
                          );
                        } else {
                          return DisplayErrorPage(
                            message: AppLocalizations.of(context)
                                .unknownErrorLoadingJoblist,
                          );
                        }
                      },
                    );
                  }
                });
          } else {
            DisplayErrorPage(
              message: AppLocalizations.of(context).errorLoadingJobList,
            );
          }
        });
      }),
    );
  }

  Widget buildHomePage(
      {double listHeight,
      double mapHeight,
      latitude,
      longitude,
      JobList jobList}) {}
}
mvanbeusekom commented 3 years ago

From an architecture point of view I would indeed treat fetching the location in a similar way as fetching data from a data source. Meaning I would abstract it away and make sure that my UI logic doesn't know anything about how to use the Geolocator API.

Looking at your setup, it might make sense to include the latitude and longitude as part of your JobListLoaded state and request the current position from your JobListCubit.getJobList method. Or if you want more precise control, you could create a separate PositionCubit and handle getting the position in there and pass the position retrieved from the PositionCubit to the getJobList method (something like context.read<JobListCubit>().getJobList(position.latitude, position.longitude)).

From the example you gave me I would refactor it to something like this (note that I left out equality and other methods to keep things brief and also made some assumptions on implementations of certain state classes, but it should give you an idea on what is going on):

JobListLoaded class

class JobListLoaded extends JobListState {
  const JobListLoaded(
    this.latitude,
    this.longitude,
    this.jobList,
  );

  final double latitude;
  final double longitude;
  final JostList jobList;
}

JobListCubit class

class JobListCubit extends Cubit<JobListState> {
  JobListCubit(this._jobRepository) : super(JobListInitial);

  final JobRepository _jobRepository;

  Future<void> getJobList() async {
    try {
      emit(JobListLoading());

      // Fetch the current position
      final position = await Geolocator.getCurrentPosition();
      // Fetch the list of jobs in the neighbourhood
      final jobList = await _jobRepository.getJobs(position.latitude, position.longitude);

      emit(JobListLoaded(
        position.latitude, 
        position.longitude, 
        jobList,
      ));
    } catch (e) {
      emit(JobListError(message: e.toString()));
    }
  }
}

HomePageState class

class _HomePageState extends State<HomePage> {
  static const double _desiredSplit = 0.6;
  double currentSplit = _desiredSplit;
  static const double _minMapFragment = 0.04;
  double listTopBarHeight = 42.0;

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: Text(AppLocalizations.of(context).appName),
        actions: [
          IconButton(
              icon: const Icon(Icons.refresh),
              onPressed: () {
                context.read<JobListCubit>().getJobList();  //USES LOCALIZATION
              }),
        ],
      ),
      drawer: Drawer(
        elevation: 16,
        child: SideDrawerList(),
      ),
      backgroundColor: Colors.white,
      body: LayoutBuilder(builder: (context, constraints) {
        // layout preparation -->
        final availableHeight = constraints.maxHeight - listTopBarHeight;
        final minMapHeight = availableHeight * _minMapFragment;
        final maxHeight = availableHeight - minMapHeight;
        var mapHeight = maxHeight * (1 - currentSplit) + minMapHeight;
        //to leave some space for the map always!
        var listHeight = maxHeight * currentSplit;
        // layout preparation <--

         return BlocConsumer<UserAuthCubit, UserAuthState>(
            listener: (context, authState) {
          if (authState is! UserAuthLogged) {
            Navigator.of(context)
                .pushReplacementNamed(UserAuthScreen.routeName);
          }
        },
            builder: (context, authState) {
          if (authState is! UserAuthLogged) {
            return DisplayLoadingPage(
                message: AppLocalizations.of(context).youHaveToLogIn);
          } else if (authState is UserAuthLogged) {
            return BlocConsumer<JobListCubit, JobListState>(  
                      listener: (context, jobListState) {
                        if (jobListState is JobListError) {
                          Scaffold.of(context).showSnackBar(
                            SnackBar(
                              content: Text(jobListState.message),
                            ),
                          );
                        }
                      },
                      builder: (context, jobListState) {
                        if (jobListState is JobListLoaded) {
                          context
                              .read<RealizationsManagerCubit>()
                              .checkIfRelizationOnJoblistBooked();
                          context
                              .read<RealizationsManagerCubit>()
                              .updateRealizationsManagerState();

                          return buildHomePage(
                            listHeight: listHeight,
                            mapHeight: mapHeight,
                            latitude: jobListState.latitude,
                            longitude: jobListState.longitude,
                            jobList: jobListState.jobList,
                          );
                        } else if (jobListState is JobListLoading) {
                          return DisplayLoadingPage(
                            message: AppLocalizations.of(context)
                                .downloadingJobListMessage,
                          );
                        } else if (jobListState is JobListInitial) {
                          context.read<JobListCubit>().getJobList();
                          return DisplayLoadingPage(
                            message: AppLocalizations.of(context)
                                .downloadingJobListMessage,
                          );
                        } else if (jobListState is JobListError) {
                          return DisplayErrorPage(
                            message: jobListState.message,
                          );
                        } else {
                          return DisplayErrorPage(
                            message: AppLocalizations.of(context)
                                .unknownErrorLoadingJoblist,
                          );
                        }
                      },
                    );
                  }
                });
      }),
    );
  }

  Widget buildHomePage(
      {double listHeight,
      double mapHeight,
      latitude,
      longitude,
      JobList jobList}) {}
}
lukaglo commented 3 years ago

Hello - super, great thanks. Sorry to have dragged you into code review :) Much appreciated!

mvanbeusekom commented 3 years ago

No problem at all, glad I could help.