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

Resolve issue with plugin target-dir="app*" subdirs #572

Merged
merged 3 commits into from Nov 22, 2018

Conversation

Projects
None yet
4 participants
@brodybits
Copy link
Contributor

brodybits commented Nov 22, 2018

(subdirectories) such as "appco", with unit tests to verify correct operation

Needed for @katzer plugins that use de/appplant subdirectory, for example:

Also needed for cordova-plugin-inappbrowser

Resolves #571

Resolve issue with plugin target-dir "app*" subdir
such as "appco", with unit tests to verify

Needed for @katzer plugins that use de/appplant subdirectory,
for example:
* cordova-plugin-local-notifications
* cordova-plugin-badge
* cordova-plugin-background-mode

Co-authored-by: Christopher J. Brody <chris.brody@gmail.com>
Co-authored-by: Julio César <jcesarmobile@gmail.com>

@brodybits brodybits requested review from janpio and jcesarmobile Nov 22, 2018

@kamilbrk

This comment has been minimized.

Copy link

kamilbrk commented Nov 22, 2018

FYI the inappbrowser plugin is affected, does not load at all on 7.1.3

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 22, 2018

Codecov Report

Merging #572 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #572   +/-   ##
=======================================
  Coverage   62.11%   62.11%           
=======================================
  Files          17       17           
  Lines        1985     1985           
  Branches      371      371           
=======================================
  Hits         1233     1233           
  Misses        752      752
Impacted Files Coverage Δ
bin/templates/cordova/lib/pluginHandlers.js 87.13% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a014228...688bf03. Read the comment docs.

janpio and others added some commits Nov 22, 2018

SQUASH-FIX: Update spec/unit/pluginHandlers/handlers.spec.js
Co-Authored-By: brodybits <chris.brody@gmail.com>
SQUASH-FIX: Update spec/unit/pluginHandlers/handlers.spec.js
Co-Authored-By: brodybits <chris.brody@gmail.com>

@janpio janpio changed the title Resolve issue with plugin target-dir "app*" subdirectories such as "appco" Fix issue with plugin target-dir "app*" subdirectories such as "appco" Nov 22, 2018

@janpio

janpio approved these changes Nov 22, 2018

@brodybits

This comment has been minimized.

Copy link
Contributor Author

brodybits commented Nov 22, 2018

Thanks @janpio for the review. I got a call in 5 minutes, will test with more plugins and merge afterwards.

@brodybits

This comment has been minimized.

Copy link
Contributor Author

brodybits commented Nov 22, 2018

And thanks to @kamilbrk for pointing out the issue with cordova-plugin-inappbrowser.

@brodybits

This comment has been minimized.

Copy link
Contributor Author

brodybits commented Nov 22, 2018

Tested in a new Cordova project

reproduced using cordova-android@latest (7.1.3)

  • cordova platform add android@latest
  • cordova plugin add cordova-plugin-inappbrowser

then the plugin Java files were incorrectly installed in the following relative directory path: platforms/android/src/org/apache/cordova/inappbrowser

Verified on proposed bug fix

  • cordova platform add https://github.com/brodybits/cordova-android#app-subdir-bugfix (based on the master branch, which includes bleeding-edge updates for Cordova 9)
  • cordova plugin add cordova-plugin-inappbrowser

then the plugin Java files are correctly installed in the following relative directory path: platforms/android/app/src/main/java/org/apache/cordova/inappbrowser

I also tested that cordova-plugin-inappbrowser Java files install correctly on cordova-android@7.1.2.

Merging now.

P.S. Some additional testing done after merge:

Tried cordova-plugin-badge with 7.1.3, https://github.com/apache/cordova-android#master, and 7.1.2

Reproduced issue on 7.1.3, resolved on https://github.com/apache/cordova-android#master, no issue observed on 7.1.2.

@brodybits brodybits changed the title Fix issue with plugin target-dir "app*" subdirectories such as "appco" Resolve issue with plugin target-dir "app*" subdirectories such as "appco" Nov 22, 2018

@brodybits brodybits changed the title Resolve issue with plugin target-dir "app*" subdirectories such as "appco" Resolve issue with plugin target-dir "app*" subdirs Nov 22, 2018

@brodybits brodybits changed the title Resolve issue with plugin target-dir "app*" subdirs Resolve issue with plugin target-dir="app*" subdirs Nov 22, 2018

@brodybits brodybits merged commit ef24341 into apache:master Nov 22, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@brodybits brodybits deleted the brodybits:app-subdir-bugfix branch Nov 22, 2018

brodybits added a commit to brodybits/cordova-android that referenced this pull request Nov 22, 2018

Resolve issue with plugin target-dir="app*" subdirs (apache#572)
(subdirectories) such as "appco", with unit tests to verify

Needed for @katzer plugins that use de/appplant subdirectory,
for example:
* cordova-plugin-local-notifications
* cordova-plugin-badge
* cordova-plugin-background-mode

Also needed for cordova-plugin-inappbrowser

Co-authored-by: Christopher J. Brody <chris.brody@gmail.com>
Co-authored-by: Julio César <jcesarmobile@gmail.com>
Co-authored-by: Jan Piotrowski <piotrowski+github@gmail.com>

@brodybits brodybits referenced this pull request Nov 22, 2018

Merged

7.1.4 patch updates #573

brodybits added a commit to brodybits/cordova-android that referenced this pull request Nov 23, 2018

Other target-dir remapping fixes
Use startsWith instead of `includes` to check for remapping of
`target-dir` that starts with `libs` or `src/main`

(Followup to PR apache#572)

brodybits added a commit to brodybits/cordova-android that referenced this pull request Nov 23, 2018

Other target-dir remapping fixes
Use `startsWith` instead of `includes` to check for remapping of
`target-dir` that starts with `libs` or `src/main`

(Followup to PR apache#572)
@brodybits

This comment has been minimized.

Copy link
Contributor Author

brodybits commented Nov 23, 2018

7.1.4 patch release is now published to resolve the issue.

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