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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from
Nov 22, 2018
Merged

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

merged 3 commits into from
Nov 22, 2018

Conversation

brodybits
Copy link
Contributor

@brodybits brodybits commented Nov 22, 2018 •
edited

(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

@jcesarmobile
Resolve issue with plugin target-dir "app*" subdir
688bf03 
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>
@kamilbrk
Copy link

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

@codecov-io
Copy link

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 2 commits November 22, 2018 07:48
SQUASH-FIX: Update spec/unit/pluginHandlers/handlers.spec.js
ae98b2e 
Co-Authored-By: brodybits <chris.brody@gmail.com>
SQUASH-FIX: Update spec/unit/pluginHandlers/handlers.spec.js
9bb08bb 
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
@brodybits brodybits mentioned this pull request Nov 22, 2018
Closed
@brodybits
Copy link
Contributor Author

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

@brodybits
Copy link
Contributor Author

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

@brodybits
Copy link
Contributor Author

brodybits commented Nov 22, 2018 •
edited

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
@brodybits brodybits deleted the app-subdir-bugfix branch November 22, 2018 13:59
brodybits pushed a commit to brodybits/cordova-android that referenced this pull request Nov 22, 2018
@jcesarmobile
Resolve issue with plugin target-dir="app*" subdirs (apache#572)
8586d4a 
(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 mentioned this pull request Nov 22, 2018
Merged
brodybits pushed a commit to brodybits/cordova-android that referenced this pull request Nov 23, 2018
Other target-dir remapping fixes
87a202d 
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 pushed a commit to brodybits/cordova-android that referenced this pull request Nov 23, 2018
Other target-dir remapping fixes
9bd952f 
Use `startsWith` instead of `includes` to check for remapping of
`target-dir` that starts with `libs` or `src/main`

(Followup to PR apache#572)
This was referenced Nov 23, 2018
Closed
Open
@brodybits
Copy link
Contributor Author

7.1.4 patch release is now published to resolve the issue.

@terangel terangel mentioned this pull request Nov 26, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@janpio janpio janpio approved these changes

@jcesarmobile jcesarmobile Awaiting requested review from jcesarmobile

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@brodybits @kamilbrk @codecov-io @janpio