Skip to content
This repository was archived by the owner on Nov 8, 2023. It is now read-only.

Allowing access to location in Airplane mode with GPS on #116

Merged
merged 4 commits into from
Mar 15, 2018

Conversation

manojdcoder
Copy link
Contributor

#108: After lot of research at my end, I think this is the only way to handle GPS in airplane mode.

@ghost ghost added the new PR label Mar 9, 2018
@@ -4,8 +4,11 @@ import { setTimeout, clearTimeout } from "timer";
import { LocationBase, defaultGetLocationTimeout, fastestTimeUpdate, minTimeUpdate } from "./geolocation.common";
import { Options, successCallbackType, errorCallbackType } from "./location-monitor";
import * as permissions from "nativescript-permissions";
import System = android.provider.Settings.System;
Copy link
Contributor

Choose a reason for hiding this comment

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

This will break the Webpack + Snapshot build. We should not access the native APIs (like android... or com...) in the root scope. Such shorthands should be avoided.

src/package.json Outdated
@@ -1,6 +1,6 @@
{
"name": "nativescript-geolocation",
"version": "4.2.3",
"version": "4.3.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I see this is a bug fix, not a feature. It could be just 4.2.4.

@manojdcoder
Copy link
Contributor Author

@DimitarTachev Thanks for taking time to review, have pushed the requested changes. Hope its good to merge now.

@@ -6,6 +6,7 @@ import { Options, successCallbackType, errorCallbackType } from "./location-moni
import * as permissions from "nativescript-permissions";

declare var com: any;
const LocationSettingsStatusCodes = com.google.android.gms.location.LocationSettingsStatusCodes;
Copy link
Contributor

@DimitarTachev DimitarTachev Mar 14, 2018

Choose a reason for hiding this comment

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

I believe the Snapshot build will also fail here + you should also rebase your branch onto master :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have pushed the changes, thanks for letting me know about these issues.

But just wondering still my app works fine with webpack and snapshot using same code. Is there a documentation that explains do / don'ts with webpack / snapshot?

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that we should improve the docs. Meanwhile, you could take a look at this blog post: https://www.nativescript.org/blog/improving-app-startup-time-on-android-with-webpack-v8-heap-snapshot

Copy link
Contributor Author

@manojdcoder manojdcoder Mar 15, 2018

Choose a reason for hiding this comment

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

Thank you @DimitarTachev but that blog also looks like very old, I'm not finding any guidelines on what we suppose to not do / follow with snapshot builds. Am I missing something?

Hope to see some guidelines in appropriate docs soon - https://docs.nativescript.org/best-practices/bundling-with-webpack#v8-heap-snapshot

Thanks a lot for taking time to response.

@manojdcoder
Copy link
Contributor Author

manojdcoder commented Mar 14, 2018

@DimitarTachev Can you please take a look and merge this, I could really use the updated version.

@DimitarTachev DimitarTachev merged commit de6d7f6 into NativeScript:master Mar 15, 2018
@ghost ghost removed the new PR label Mar 15, 2018
@manojdcoder
Copy link
Contributor Author

Thank you @DimitarTachev Could you please push a build to NPM as well?

@DimitarTachev
Copy link
Contributor

@manojdcoder We will try to publish it later today or tomorrow.

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

Successfully merging this pull request may close these issues.

3 participants