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

Display prompt for enabling GPS #252

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Display prompt for enabling GPS #252

wants to merge 2 commits into from

Conversation

bernamaxim
Copy link

@bernamaxim bernamaxim commented Feb 8, 2017

Display prompt for enabling GPS, when it disable. Because when forget to enable Location service, Traccar client cannot send location at all.

Copy link
Member

@tananaev tananaev left a comment

Choose a reason for hiding this comment

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

Added some comments.

{
final AlertDialog.Builder builder = new AlertDialog.Builder(activity);
final String action = Settings.ACTION_LOCATION_SOURCE_SETTINGS;
final String message = "Enable location service to find current location. Click OK to go to.";
Copy link
Member

Choose a reason for hiding this comment

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

Please don't use hardcoded strings.

@@ -230,6 +233,25 @@ private void initPreferences() {
findPreference(KEY_DEVICE).setSummary(sharedPreferences.getString(KEY_DEVICE, null));
}

private void displayPromptForEnablingGPS(
Copy link
Member

Choose a reason for hiding this comment

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

GPS should be camel-cased.

final Activity activity)
{
final AlertDialog.Builder builder = new AlertDialog.Builder(activity);
final String action = Settings.ACTION_LOCATION_SOURCE_SETTINGS;
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need a variable for this?

@@ -255,6 +277,10 @@ private void startTrackingService(boolean checkPermission, boolean permission) {
startService(new Intent(this, TrackingService.class));
alarmManager.setInexactRepeating(AlarmManager.ELAPSED_REALTIME_WAKEUP,
15000, 15000, alarmIntent);
String locationProviders = Settings.Secure.getString(getContentResolver(), Settings.Secure.LOCATION_PROVIDERS_ALLOWED);
if (locationProviders == null || locationProviders.equals("")) {
Copy link
Member

Choose a reason for hiding this comment

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

Use TextUtils.isEmpty(...) instead.

@@ -255,6 +277,10 @@ private void startTrackingService(boolean checkPermission, boolean permission) {
startService(new Intent(this, TrackingService.class));
alarmManager.setInexactRepeating(AlarmManager.ELAPSED_REALTIME_WAKEUP,
15000, 15000, alarmIntent);
String locationProviders = Settings.Secure.getString(getContentResolver(), Settings.Secure.LOCATION_PROVIDERS_ALLOWED);
if (locationProviders == null || locationProviders.equals("")) {
displayPromptForEnablingGPS(this);
Copy link
Member

Choose a reason for hiding this comment

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

Why is it called "GPS" when you check for all providers?

@@ -255,6 +277,10 @@ private void startTrackingService(boolean checkPermission, boolean permission) {
startService(new Intent(this, TrackingService.class));
Copy link
Member

Choose a reason for hiding this comment

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

Should we wait till user enables providers before starting the service?

Copy link
Author

Choose a reason for hiding this comment

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

Sure, because if user not enable providers, it will be useless. Traccar-client cannot update location and send it.

Copy link
Member

Choose a reason for hiding this comment

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

You said yes, but you haven't fixed it.

Copy link
Author

Choose a reason for hiding this comment

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

I'm sorry, I'm still trying but have not been successful. I try to use AsyncTask (https://developer.android.com/reference/android/os/AsyncTask.html)

{
final AlertDialog.Builder builder = new AlertDialog.Builder(activity);
builder.setMessage(R.string.prompt_location_service)
.setPositiveButton("OK",
Copy link
Member

Choose a reason for hiding this comment

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

Still hardcoded string here.

@@ -44,4 +44,5 @@
<string name="status_connectivity_change">Connectivity change</string>
<string name="hidden_app_name">Device Settings</string>
<string name="hidden_alert">The app has been hidden. To open it again please dial 8722227 (TRACCAR).</string>
<string name="prompt_location_service">Enable location service to find current location. Click OK to go to.</string>
Copy link
Member

Choose a reason for hiding this comment

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

Please use some better wording. Something like:

Please enable location services. Click OK to open settings screen.

@@ -255,6 +277,10 @@ private void startTrackingService(boolean checkPermission, boolean permission) {
startService(new Intent(this, TrackingService.class));
Copy link
Member

Choose a reason for hiding this comment

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

You said yes, but you haven't fixed it.

@vliparsov
Copy link

😁

@Anton-V-K
Copy link
Contributor

Are the code conflicts resolved already? Is this feature ready for releasing?

@tananaev
Copy link
Member

tananaev commented Jan 1, 2018

As you can see, there were no commits after my review, so nothing is solved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants