Variable for Require GPS Hardware #189

sithwarrior · 2020-02-19T09:55:12Z

Platforms affected

Android

Motivation and Context

Currently installing this plugin, has the sideeffect of your App only beeing installable on Android devices with GPS hardware. We have clients with tablets without GPS hardware.

closes #187
closes #171
closes #86
closes #98

Description

Added the variable GPS_REQUIRED so when installing the pluging, its optional if you want to require GPS Hardware on Android devices. Currently an app with this plugin, cannot run on devices without GPS Hardware, because of this setting in the plugin.xml

<uses-feature android:name="android.hardware.location.gps" />

Also added a description in the readme, under Android Quirks, as this only affects Android atm.

Naming of the variable is all caps, as is standard with install time variables, for editing the AndroidManifest.xml.

This PR is trying to finish the work in #171

Testing

Testing installing the plugin with and without the variable, and checked the resulting androidmanifest.xml and tested the resulting apk's.

Checklist

I've run the tests to see all new and existing tests pass
I added automated test coverage as appropriate for this change
[] Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
I've updated the documentation if necessary

jony89 · 2020-03-10T13:39:01Z

why isn't this merged ? this or #171 @breautek

breautek · 2020-03-10T16:21:21Z

why isn't this merged ? this or #171 @breautek

Two main reasons:

Because both PRs introduce preference in a UPPERCASE_FORMAT, which doesn't match existing format cordova uses for preferences, which in a PascalCase format. I'm hesitate to add inconsitencies.
Generally speaking, we need at least two positive votes and 0 negative votes from apache members for a PR to be accepted. So even if I agreed with this PR, I need one more member to review before I can merge.

jony89 · 2020-03-10T16:52:31Z

Actually variables are defined in UPPERCASE_FORMAT, as it can be seen in different plugins as well :

cordova-plugin-facebook4
cordova-android-firebase-gradle-release
cordova-android-play-services-gradle-release
cordova-android-support-gradle-release
cordova-plugin-admob-free

example

    <plugin name="cordova-plugin-admob-free" spec="^0.21.0">
        <variable name="ADMOB_APP_ID" value="ca-app-pub-****" />
    </plugin>
    <plugin name="cordova-android-support-gradle-release" spec="^3.0.1">
        <variable name="ANDROID_SUPPORT_VERSION" value="28.+" />
    </plugin>
    <plugin name="cordova-android-play-services-gradle-release" spec="^2.0.0">
        <variable name="PLAY_SERVICES_VERSION" value="+" />
    </plugin>
    <plugin name="cordova-android-firebase-gradle-release" spec="^2.0.0">
        <variable name="FIREBASE_VERSION" value="+" />
    </plugin>
    <plugin name="cordova-plugin-facebook4" spec="4.1.0">
        <variable name="APP_ID" value="***" />
        <variable name="APP_NAME" value="***" />
        <variable name="FACEBOOK_HYBRID_APP_EVENTS" value="false" />
        <variable name="FACEBOOK_ANDROID_SDK_VERSION" value="4.38.1" />
    </plugin>

sithwarrior · 2020-03-10T17:30:04Z

why isn't this merged ? this or #171 @breautek

Two main reasons:

Because both PRs introduce preference in a UPPERCASE_FORMAT, which doesn't match existing format cordova uses for preferences, which in a PascalCase format. I'm hesitate to add inconsitencies.

Generally speaking, we need at least two positive votes and 0 negative votes from apache members for a PR to be accepted. So even if I agreed with this PR, I need one more member to review before I can merge.

Actually as @jony89 also states UPPERCASE_FORMAT is the norm for this sort of setting, that effects the native build.

breautek · 2020-03-10T18:03:10Z

Those aren't apache's codebases. I'm talking about consistency within the apache's repositories. Third-party plugins may choose whatever format they like to use. If you look at any other apache cordova plugin, you will see preference names defined in PascalCase format.

jony89 · 2020-03-10T18:09:06Z

@sithwarrior can you please change rename the variable to GPSRequired so we can progress with this merge?
It is a crucial one.

sithwarrior · 2020-03-10T21:47:03Z

Those aren't apache's codebases. I'm talking about consistency within the apache's repositories. Third-party plugins may choose whatever format they like to use. If you look at any other apache cordova plugin, you will see preference names defined in PascalCase format.

@breautek Thats not the case for variables set at install time, that set native build settings, as mentioned earlier.

Here from the cordova docs
https://cordova.apache.org/docs/en/latest/plugin_ref/spec.html

API_KEY
plugman --platform android --project /path/to/project --plugin name|git-url|path --variable API_KEY=!@CFATGWE%^WGSFDGSDFW$%^#$%YTHGsdfhsfhyer56734

Here from the apache cordova camera app
<preference name="ANDROID_SUPPORT_V4_VERSION" default="27.+"/>

Here from the apache cordova media app
<preference name="KEEP_AVAUDIOSESSION_ALWAYS_ACTIVE" default="NO" />

Also it dosent work with PascalCase .
If you install a plugin with --variable switch, which we need here, it automaticly becomes UPPER_CASE when saved in config.xml, and then crashes on build, because it dosent match the PascalCase in the plugin.xml

breautek · 2020-03-11T15:46:40Z

Also it dosent work with PascalCase .
If you install a plugin with --variable switch, which we need here, it automaticly becomes UPPER_CASE when saved in config.xml, and then crashes on build, because it dosent match the PascalCase in the plugin.xml

I'll try to take a look later tonight. Is there a reason why we are using a --variable switch instead of a config.xml preference?

breautek

Had a deep look at this, and I understand that this cannot actually be done using the config xml preferences as I was originally requesting, that it must be done using the --variable switch because we need to use those variables during install time.

I don't really like it, as that means to change the variable, you must remove and re-add the plugin, but it is what is required.

You can also modify the PR description to include the following GH keywords:
closes #171
closes #86
closes #98

As this PR makes those PRs irrelevant.

Also would like to take the moment to thank everybody involved with their contributions and patience. I know I was being hard, I was just trying to ensure a certain level of quality and consistency. And obviously the way I wanted things done was not actually possible.

The tests are green and this PR defaults the required state to true to maintains backwards compatibility which makes this PR :+1 to me.

m4tta · 2020-03-18T06:56:33Z

@erisu can we get a review on this? Right now i have to do hacky stuff to get my project built correctly, and this would fix that.

README.md

timbru31

Besides the small README suggestion, LGTM!
I've done a quick test, the AndroidManifest file is changed as expected.

Unfortunately, my android devices all have GPS, so I can't really test this besides building the APK.

breautek · 2020-03-18T13:18:19Z

Besides the small README suggestion, LGTM!
I've done a quick test, the AndroidManifest file is changed as expected.

Unfortunately, my android devices all have GPS, so I can't really test this besides building the APK.

This was the extent to my testing. And I'm not entirely sure if the phone will let you install an APK even if you lack required features or not. This may also just be a google play store where the listing is simply hidden.

So once the README has been updated as per your suggestion I'll merge this PR.

timbru31 · 2020-03-18T13:26:42Z

Luckily Martin set the checkmark for "allow edit by maintainers". I've applied my small readme suggestion. Once the CI is green, feel free to merge.

breautek · 2020-03-18T13:36:45Z

All green. Thanks @timbru31 for taking the time to review 🎉

timbru31 · 2020-03-18T14:24:04Z

This also closes https://issues.apache.org/jira/browse/CB-14255

sithwarrior · 2020-03-18T14:36:43Z

Great stuff @breautek Any Idea, when this will be included in a new release, so we can use it, in our apps?

timbru31 · 2020-03-18T14:41:32Z

We generally do not give any ETAs on new releases. You can always install the master with cordova plugin add https://github.com/apache/cordova-plugin-geolocation.git though.

sithwarrior · 2020-10-28T08:53:20Z

Still no eta on a proper release?

I know I can install using the master branch, but that's basically the same thing, and setting your build setting to nightly, and sidesteps semver.

Variable for Require GPS Hardware

dd9b06c

breautek approved these changes Mar 11, 2020

View reviewed changes

breautek added the enhancement label Mar 11, 2020

breautek requested a review from erisu March 11, 2020 23:08

breautek requested review from timbru31 and NiklasMerz March 18, 2020 12:12

timbru31 reviewed Mar 18, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

timbru31 reviewed Mar 18, 2020

View reviewed changes

docs: wording changes for installation

aea76ba

timbru31 approved these changes Mar 18, 2020

View reviewed changes

breautek merged commit 89cf51d into apache:master Mar 18, 2020

timbru31 mentioned this pull request Mar 24, 2020

CB-11875 added android.hardware.location.gps uses-feature. #81

Closed

4 tasks

AE1NS mentioned this pull request Nov 25, 2020

Geolocation 4.1.0: Illegal value $GPS_REQUIRED danielsogl/awesome-cordova-plugins#3579

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Variable for Require GPS Hardware #189

Variable for Require GPS Hardware #189

sithwarrior commented Feb 19, 2020 •

edited

Loading

jony89 commented Mar 10, 2020

breautek commented Mar 10, 2020

jony89 commented Mar 10, 2020 •

edited

Loading

sithwarrior commented Mar 10, 2020

breautek commented Mar 10, 2020 •

edited

Loading

jony89 commented Mar 10, 2020

sithwarrior commented Mar 10, 2020

breautek commented Mar 11, 2020

breautek left a comment

m4tta commented Mar 18, 2020

timbru31 left a comment

breautek commented Mar 18, 2020

timbru31 commented Mar 18, 2020

breautek commented Mar 18, 2020

timbru31 commented Mar 18, 2020

sithwarrior commented Mar 18, 2020

timbru31 commented Mar 18, 2020

sithwarrior commented Oct 28, 2020

Variable for Require GPS Hardware #189

Variable for Require GPS Hardware #189

Conversation

sithwarrior commented Feb 19, 2020 • edited Loading

Platforms affected

Motivation and Context

Description

Testing

Checklist

jony89 commented Mar 10, 2020

breautek commented Mar 10, 2020

jony89 commented Mar 10, 2020 • edited Loading

sithwarrior commented Mar 10, 2020

breautek commented Mar 10, 2020 • edited Loading

jony89 commented Mar 10, 2020

sithwarrior commented Mar 10, 2020

breautek commented Mar 11, 2020

breautek left a comment

Choose a reason for hiding this comment

m4tta commented Mar 18, 2020

timbru31 left a comment

Choose a reason for hiding this comment

breautek commented Mar 18, 2020

timbru31 commented Mar 18, 2020

breautek commented Mar 18, 2020

timbru31 commented Mar 18, 2020

sithwarrior commented Mar 18, 2020

timbru31 commented Mar 18, 2020

sithwarrior commented Oct 28, 2020

sithwarrior commented Feb 19, 2020 •

edited

Loading

jony89 commented Mar 10, 2020 •

edited

Loading

breautek commented Mar 10, 2020 •

edited

Loading