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

Mainactivity track packagename change #1140

Closed

Conversation

wedgberto
Copy link
Contributor

#<!--
Please make sure the checklist boxes are all checked before submitting the PR. The checklist is intended as a quick reference, for complete details please see our Contributor Guidelines:

http://cordova.apache.org/contribute/contribute_guidelines.html

Thanks!
-->

Platforms affected

Android

Motivation and Context

Cordova has built in hook functionality that enables customisation of the build - my project has used the hook feature to alter the package name via command line arguments so that side by side versions of the app can be built. e.g. io.cordova.helloword can be installed and ran alongside io.cordova.helloword2 because they have different package names. This is very useful for building a test, UAT or beta release. This was possible in cordova-android@8.1.0 but was broken by changes made in 9.0.0. When the package name of the project is changed in config.xml, the MainActivity.Java file is no longer moved to a folder that tracks the package name and ends up being removed.

This leads to an app crash when launched because the MainActivity class does not exist.

Subsequent build attempts of the project fail because the MainActivity.java file is missing from the project.

#1139

Description

This change reverts the behaviour of ensuring that MainActivity.java is located in a folder that tracks the package name read from config.xml. I.e a package name of io.cordova.helloworld in config.xml results in MainActivity.java existing in src/main/java/io/cordova/helloworld even if the package name in config.xml is altered after the platform is added to the project.

Testing

I have added a new Jasmine unit test "relocate MainActivity.java", but need some help proving that this test is correct.

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

erisu and others added 6 commits June 23, 2020 18:33
# Conflicts:
#	VERSION
#	bin/templates/cordova/Api.js
#	bin/templates/project/assets/www/cordova.js
#	framework/build.gradle
#	framework/src/org/apache/cordova/CordovaWebView.java
#	spec/unit/prepare.spec.js
Copy link
Contributor

@breautek breautek left a comment

Choose a reason for hiding this comment

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

Thank you for your time and effort into investigating and preparing this PR. Unfortunately this is a PR that cannot be accepted as it currently stands.

I've noted a number of changes that I suggest. Generally speaking, we should try to avoid changing code that is not relevant to the problem that the PR is trying to resolve.

Additionally, there appears to be a syntax error in prepare.spec.js that is preventing eslint and other tests to completely run.

> eslint . "bin/**/!(*.*|gitignore)"


D:\a\cordova-android\cordova-android\spec\unit\prepare.spec.js
  912:1  error  Parsing error: Unexpected token

✖ 1 problem (1 error, 0 warnings)

npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! cordova-android@9.1.0-dev lint: `eslint . "bin/**/!(*.*|gitignore)"`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the cordova-android@9.1.0-dev lint script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     C:\npm\cache\_logs\2020-12-03T15_46_34_987Z-debug.log
npm ERR! Test failed.  See above for more details.
Error: Process completed with exit code 1.

Please look into this. You can run the tests locally by running npm test.

Kind regards,
Norman

bin/templates/cordova/lib/prepare.js Outdated Show resolved Hide resolved
bin/templates/cordova/lib/prepare.js Outdated Show resolved Hide resolved
bin/templates/cordova/lib/prepare.js Outdated Show resolved Hide resolved
spec/unit/prepare.spec.js Outdated Show resolved Hide resolved
spec/unit/prepare.spec.js Outdated Show resolved Hide resolved
spec/unit/prepare.spec.js Outdated Show resolved Hide resolved
test/android/app/build.gradle Outdated Show resolved Hide resolved
bin/templates/gradle.properties Outdated Show resolved Hide resolved
spec/unit/prepare.spec.js Outdated Show resolved Hide resolved
revert changes not specific to this PR
@wedgberto
Copy link
Contributor Author

@breautek Thanks for the gentle comments. I have implemented your suggestions, but would like to request that this PR not be accepted until I have the new unit test working as intended.

@erisu
Copy link
Member

erisu commented Dec 4, 2020

Is there a reason why the branch of this PR is based off the 9.x branch/tag?

You have release related commits in this PR that is not in master.

@codecov-io
Copy link

codecov-io commented Dec 4, 2020

Codecov Report

Merging #1140 (65db4b5) into master (97e2d15) will decrease coverage by 0.26%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1140      +/-   ##
==========================================
- Coverage   71.19%   70.92%   -0.27%     
==========================================
  Files          21       21              
  Lines        1739     1747       +8     
==========================================
+ Hits         1238     1239       +1     
- Misses        501      508       +7     
Impacted Files Coverage Δ
bin/templates/cordova/lib/prepare.js 43.12% <0.00%> (-0.79%) ⬇️

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 97e2d15...65db4b5. Read the comment docs.

@wedgberto wedgberto closed this Jan 14, 2021
@wedgberto wedgberto reopened this Jan 18, 2021
@wedgberto
Copy link
Contributor Author

this PR has been abandoned and a new PR created

@wedgberto wedgberto closed this Jan 18, 2021
@wedgberto wedgberto deleted the mainactivity_track_packagename_change branch January 18, 2021 14:28
@wedgberto
Copy link
Contributor Author

Is there a reason why the branch of this PR is based off the 9.x branch/tag?

You have release related commits in this PR that is not in master.

I did that by mistake. I've created a new PR that was branched from master - #1154.
This issue is breaking the automated build of my app so I would really appreciate the new PR being reviewed.

@wedgberto
Copy link
Contributor Author

@bre

@breautek Thanks for the gentle comments. I have implemented your suggestions, but would like to request that this PR not be accepted until I have the new unit test working as intended.

@breautek I abandoned this PR and started a new one #1154 ('cos I base this this one from the wrong branch as highlighted by @erisu)

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