You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@cordova.apache.org by GitBox <gi...@apache.org> on 2020/12/03 15:44:53 UTC

[GitHub] [cordova-android] wedgberto opened a new pull request #1140: Mainactivity track packagename change

wedgberto opened a new pull request #1140:
URL: https://github.com/apache/cordova-android/pull/1140


   #<!--
   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
   <!-- Why is this change required? What problem does it solve? -->
   <!-- If it fixes an open issue, please link to the issue here. -->
   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
   <!-- Describe your changes in detail -->
   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
   <!-- Please describe in detail how you tested your changes. -->
   I have added a new Jasmine unit test "relocate MainActivity.java", but need some help proving that this test is correct.
   
   
   ### Checklist
   
   - [x] 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)`)
   - [x] 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](https://help.github.com/articles/closing-issues-using-keywords/))
   - [ ] I've updated the documentation if necessary
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org

[GitHub] [cordova-android] wedgberto commented on pull request #1140: Mainactivity track packagename change

Posted by GitBox <gi...@apache.org>.
wedgberto commented on pull request #1140:
URL: https://github.com/apache/cordova-android/pull/1140#issuecomment-854762190


   @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)


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org

[GitHub] [cordova-android] wedgberto commented on a change in pull request #1140: Mainactivity track packagename change

Posted by GitBox <gi...@apache.org>.
wedgberto commented on a change in pull request #1140:
URL: https://github.com/apache/cordova-android/pull/1140#discussion_r535955962



##########
File path: bin/templates/gradle.properties
##########
@@ -0,0 +1,4 @@
+org.gradle.daemon=true
+org.gradle.jvmargs=-Xmx2048m
+android.useAndroidX=false
+android.enableJetifier=false

Review comment:
       this is an unintended artefact of the new unit test. I will remove it




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org

[GitHub] [cordova-android] wedgberto commented on pull request #1140: Mainactivity track packagename change

Posted by GitBox <gi...@apache.org>.
wedgberto commented on pull request #1140:
URL: https://github.com/apache/cordova-android/pull/1140#issuecomment-760073797


   I am going to abandon this PR and start again


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org

[GitHub] [cordova-android] wedgberto commented on pull request #1140: Mainactivity track packagename change

Posted by GitBox <gi...@apache.org>.
wedgberto commented on pull request #1140:
URL: https://github.com/apache/cordova-android/pull/1140#issuecomment-738684459


   @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. 


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org

[GitHub] [cordova-android] wedgberto removed a comment on pull request #1140: Mainactivity track packagename change

Posted by GitBox <gi...@apache.org>.
wedgberto removed a comment on pull request #1140:
URL: https://github.com/apache/cordova-android/pull/1140#issuecomment-760073797


   I am going to abandon this PR and start again


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org

[GitHub] [cordova-android] wedgberto commented on a change in pull request #1140: Mainactivity track packagename change

Posted by GitBox <gi...@apache.org>.
wedgberto commented on a change in pull request #1140:
URL: https://github.com/apache/cordova-android/pull/1140#discussion_r535962581



##########
File path: spec/unit/prepare.spec.js
##########
@@ -493,7 +494,7 @@ describe('prepare', () => {
         });
     });
 
-    describe('prepareIcons method', function () {
+    describe('prepareIcons', function () {

Review comment:
       all non related changes have been reverted




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org

[GitHub] [cordova-android] codecov-io edited a comment on pull request #1140: Mainactivity track packagename change

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #1140:
URL: https://github.com/apache/cordova-android/pull/1140#issuecomment-738696981


   # [Codecov](https://codecov.io/gh/apache/cordova-android/pull/1140?src=pr&el=h1) Report
   > Merging [#1140](https://codecov.io/gh/apache/cordova-android/pull/1140?src=pr&el=desc) (65db4b5) into [master](https://codecov.io/gh/apache/cordova-android/commit/97e2d15634f3c7a37e468cdafcb697257340570c?el=desc) (97e2d15) will **decrease** coverage by `0.32%`.
   > The diff coverage is `0.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/cordova-android/pull/1140/graphs/tree.svg?width=650&height=150&src=pr&token=q14nMf6C5a)](https://codecov.io/gh/apache/cordova-android/pull/1140?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #1140      +/-   ##
   ==========================================
   - Coverage   71.19%   70.86%   -0.33%     
   ==========================================
     Files          21       21              
     Lines        1739     1747       +8     
   ==========================================
     Hits         1238     1238              
   - Misses        501      509       +8     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/cordova-android/pull/1140?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [bin/templates/cordova/lib/prepare.js](https://codecov.io/gh/apache/cordova-android/pull/1140/diff?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9jb3Jkb3ZhL2xpYi9wcmVwYXJlLmpz) | `43.12% <0.00%> (-0.79%)` | :arrow_down: |
   | [bin/templates/cordova/lib/check\_reqs.js](https://codecov.io/gh/apache/cordova-android/pull/1140/diff?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9jb3Jkb3ZhL2xpYi9jaGVja19yZXFzLmpz) | `69.75% <0.00%> (-0.49%)` | :arrow_down: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/cordova-android/pull/1140?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/cordova-android/pull/1140?src=pr&el=footer). Last update [97e2d15...65db4b5](https://codecov.io/gh/apache/cordova-android/pull/1140?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org

[GitHub] [cordova-android] erisu commented on pull request #1140: Mainactivity track packagename change

Posted by GitBox <gi...@apache.org>.
erisu commented on pull request #1140:
URL: https://github.com/apache/cordova-android/pull/1140#issuecomment-738696288


   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.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org

[GitHub] [cordova-android] wedgberto commented on a change in pull request #1140: Mainactivity track packagename change

Posted by GitBox <gi...@apache.org>.
wedgberto commented on a change in pull request #1140:
URL: https://github.com/apache/cordova-android/pull/1140#discussion_r535954644



##########
File path: spec/unit/prepare.spec.js
##########
@@ -833,4 +834,78 @@ describe('prepare', () => {
             ).toBeResolved();
         });
     });
+
+    describe('updateProjectAccordingTo', function () {
+    describe('relocate MainActivity.java', function () {

Review comment:
       This was a consequence of basing this test on an existing test




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org

[GitHub] [cordova-android] wedgberto commented on a change in pull request #1140: Mainactivity track packagename change

Posted by GitBox <gi...@apache.org>.
wedgberto commented on a change in pull request #1140:
URL: https://github.com/apache/cordova-android/pull/1140#discussion_r535962167



##########
File path: bin/templates/gradle.properties
##########
@@ -0,0 +1,4 @@
+org.gradle.daemon=true
+org.gradle.jvmargs=-Xmx2048m
+android.useAndroidX=false
+android.enableJetifier=false

Review comment:
       the new unit test generates this file every time it runs




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org

[GitHub] [cordova-android] wedgberto commented on a change in pull request #1140: Mainactivity track packagename change

Posted by GitBox <gi...@apache.org>.
wedgberto commented on a change in pull request #1140:
URL: https://github.com/apache/cordova-android/pull/1140#discussion_r535950136



##########
File path: bin/templates/cordova/lib/prepare.js
##########
@@ -249,14 +249,18 @@ function updateProjectAccordingTo (platformConfig, locations) {
         events.emit('log', 'Multiple candidate Java files that extend CordovaActivity found. Guessing at the first one, ' + java_files[0]);
     }
 
-    const destFile = java_files[0];
-
-    // var destFile = path.join(locations.root, 'app', 'src', 'main', 'java', androidPkgName.replace(/\./g, '/'), path.basename(java_files[0]));
-    // fs.ensureDirSync(path.dirname(destFile));
-    // events.emit('verbose', java_files[0]);
-    // events.emit('verbose', destFile);
-    // console.log(locations);
-    // fs.copySync(java_files[0], destFile);
+    var destFile = java_files[0];
+
+    // if package name has changed, path to MainActivity.java has to track it
+    var newDestFile = path.join(locations.root, 'app', 'src', 'main', 'java', androidPkgName.replace(/\./g, '/'), path.basename(java_files[0]));
+    if (newDestFile !== destFile) {
+        fs.ensureDirSync(path.dirname(newDestFile));
+        events.emit('verbose', destFile);
+        events.emit('verbose', newDestFile);
+        console.log(locations);

Review comment:
       I was trying to figure out what was going on with the unit test




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org

[GitHub] [cordova-android] wedgberto commented on pull request #1140: Mainactivity track packagename change

Posted by GitBox <gi...@apache.org>.
wedgberto commented on pull request #1140:
URL: https://github.com/apache/cordova-android/pull/1140#issuecomment-854759871


   > 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.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org

[GitHub] [cordova-android] codecov-io commented on pull request #1140: Mainactivity track packagename change

Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #1140:
URL: https://github.com/apache/cordova-android/pull/1140#issuecomment-738696981


   # [Codecov](https://codecov.io/gh/apache/cordova-android/pull/1140?src=pr&el=h1) Report
   > Merging [#1140](https://codecov.io/gh/apache/cordova-android/pull/1140?src=pr&el=desc) (65db4b5) into [master](https://codecov.io/gh/apache/cordova-android/commit/97e2d15634f3c7a37e468cdafcb697257340570c?el=desc) (97e2d15) will **decrease** coverage by `0.32%`.
   > The diff coverage is `0.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/cordova-android/pull/1140/graphs/tree.svg?width=650&height=150&src=pr&token=q14nMf6C5a)](https://codecov.io/gh/apache/cordova-android/pull/1140?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #1140      +/-   ##
   ==========================================
   - Coverage   71.19%   70.86%   -0.33%     
   ==========================================
     Files          21       21              
     Lines        1739     1747       +8     
   ==========================================
     Hits         1238     1238              
   - Misses        501      509       +8     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/cordova-android/pull/1140?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [bin/templates/cordova/lib/prepare.js](https://codecov.io/gh/apache/cordova-android/pull/1140/diff?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9jb3Jkb3ZhL2xpYi9wcmVwYXJlLmpz) | `43.12% <0.00%> (-0.79%)` | :arrow_down: |
   | [bin/templates/cordova/lib/check\_reqs.js](https://codecov.io/gh/apache/cordova-android/pull/1140/diff?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9jb3Jkb3ZhL2xpYi9jaGVja19yZXFzLmpz) | `69.75% <0.00%> (-0.49%)` | :arrow_down: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/cordova-android/pull/1140?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/cordova-android/pull/1140?src=pr&el=footer). Last update [97e2d15...65db4b5](https://codecov.io/gh/apache/cordova-android/pull/1140?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org

[GitHub] [cordova-android] codecov-io edited a comment on pull request #1140: Mainactivity track packagename change

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #1140:
URL: https://github.com/apache/cordova-android/pull/1140#issuecomment-738696981


   # [Codecov](https://codecov.io/gh/apache/cordova-android/pull/1140?src=pr&el=h1) Report
   > Merging [#1140](https://codecov.io/gh/apache/cordova-android/pull/1140?src=pr&el=desc) (65db4b5) into [master](https://codecov.io/gh/apache/cordova-android/commit/97e2d15634f3c7a37e468cdafcb697257340570c?el=desc) (97e2d15) will **decrease** coverage by `0.26%`.
   > The diff coverage is `0.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/cordova-android/pull/1140/graphs/tree.svg?width=650&height=150&src=pr&token=q14nMf6C5a)](https://codecov.io/gh/apache/cordova-android/pull/1140?src=pr&el=tree)
   
   ```diff
   @@            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](https://codecov.io/gh/apache/cordova-android/pull/1140?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [bin/templates/cordova/lib/prepare.js](https://codecov.io/gh/apache/cordova-android/pull/1140/diff?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9jb3Jkb3ZhL2xpYi9wcmVwYXJlLmpz) | `43.12% <0.00%> (-0.79%)` | :arrow_down: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/cordova-android/pull/1140?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/cordova-android/pull/1140?src=pr&el=footer). Last update [97e2d15...65db4b5](https://codecov.io/gh/apache/cordova-android/pull/1140?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org

[GitHub] [cordova-android] wedgberto commented on pull request #1140: Mainactivity track packagename change

Posted by GitBox <gi...@apache.org>.
wedgberto commented on pull request #1140:
URL: https://github.com/apache/cordova-android/pull/1140#issuecomment-762284382


   this PR has been abandoned and a new PR created


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org

[GitHub] [cordova-android] wedgberto commented on a change in pull request #1140: Mainactivity track packagename change

Posted by GitBox <gi...@apache.org>.
wedgberto commented on a change in pull request #1140:
URL: https://github.com/apache/cordova-android/pull/1140#discussion_r535962331



##########
File path: test/android/app/build.gradle
##########
@@ -25,7 +25,7 @@ android {
     buildToolsVersion "29.0.2"
 
     defaultConfig {
-        applicationId "org.apache.cordova.unittests"
+        applicationId "org.apache.cordova.unittests2"

Review comment:
       reverted




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org

[GitHub] [cordova-android] breautek commented on a change in pull request #1140: Mainactivity track packagename change

Posted by GitBox <gi...@apache.org>.
breautek commented on a change in pull request #1140:
URL: https://github.com/apache/cordova-android/pull/1140#discussion_r535720543



##########
File path: bin/templates/cordova/lib/prepare.js
##########
@@ -249,14 +249,18 @@ function updateProjectAccordingTo (platformConfig, locations) {
         events.emit('log', 'Multiple candidate Java files that extend CordovaActivity found. Guessing at the first one, ' + java_files[0]);
     }
 
-    const destFile = java_files[0];
-
-    // var destFile = path.join(locations.root, 'app', 'src', 'main', 'java', androidPkgName.replace(/\./g, '/'), path.basename(java_files[0]));
-    // fs.ensureDirSync(path.dirname(destFile));
-    // events.emit('verbose', java_files[0]);
-    // events.emit('verbose', destFile);
-    // console.log(locations);
-    // fs.copySync(java_files[0], destFile);
+    var destFile = java_files[0];
+
+    // if package name has changed, path to MainActivity.java has to track it
+    var newDestFile = path.join(locations.root, 'app', 'src', 'main', 'java', androidPkgName.replace(/\./g, '/'), path.basename(java_files[0]));

Review comment:
       ```suggestion
       let newDestFile = path.join(locations.root, 'app', 'src', 'main', 'java', androidPkgName.replace(/\./g, '/'), path.basename(java_files[0]));
   ```

##########
File path: bin/templates/cordova/lib/prepare.js
##########
@@ -249,14 +249,18 @@ function updateProjectAccordingTo (platformConfig, locations) {
         events.emit('log', 'Multiple candidate Java files that extend CordovaActivity found. Guessing at the first one, ' + java_files[0]);
     }
 
-    const destFile = java_files[0];
-
-    // var destFile = path.join(locations.root, 'app', 'src', 'main', 'java', androidPkgName.replace(/\./g, '/'), path.basename(java_files[0]));
-    // fs.ensureDirSync(path.dirname(destFile));
-    // events.emit('verbose', java_files[0]);
-    // events.emit('verbose', destFile);
-    // console.log(locations);
-    // fs.copySync(java_files[0], destFile);
+    var destFile = java_files[0];

Review comment:
       For new code we are trying to migrate to using `let` or `const` over `var` for variable declarations. So here we should use `let` instead.
   
   ```suggestion
       let destFile = java_files[0];
   ```

##########
File path: test/android/app/src/main/java/org/apache/cordova/unittests/StandardActivity.java
##########
@@ -17,7 +17,7 @@ Licensed to the Apache Software Foundation (ASF) under one
        under the License.
 */
 
-package org.apache.cordova.unittests;
+package org.apache.cordova.unittests2;

Review comment:
       Why is this change required?

##########
File path: bin/templates/cordova/lib/prepare.js
##########
@@ -249,14 +249,18 @@ function updateProjectAccordingTo (platformConfig, locations) {
         events.emit('log', 'Multiple candidate Java files that extend CordovaActivity found. Guessing at the first one, ' + java_files[0]);
     }
 
-    const destFile = java_files[0];
-
-    // var destFile = path.join(locations.root, 'app', 'src', 'main', 'java', androidPkgName.replace(/\./g, '/'), path.basename(java_files[0]));
-    // fs.ensureDirSync(path.dirname(destFile));
-    // events.emit('verbose', java_files[0]);
-    // events.emit('verbose', destFile);
-    // console.log(locations);
-    // fs.copySync(java_files[0], destFile);
+    var destFile = java_files[0];
+
+    // if package name has changed, path to MainActivity.java has to track it
+    var newDestFile = path.join(locations.root, 'app', 'src', 'main', 'java', androidPkgName.replace(/\./g, '/'), path.basename(java_files[0]));
+    if (newDestFile !== destFile) {
+        fs.ensureDirSync(path.dirname(newDestFile));
+        events.emit('verbose', destFile);
+        events.emit('verbose', newDestFile);
+        console.log(locations);

Review comment:
       Console usage should not remain in the codebase.
   
   ```suggestion
   ```

##########
File path: spec/unit/prepare.spec.js
##########
@@ -493,7 +494,7 @@ describe('prepare', () => {
         });
     });
 
-    describe('prepareIcons method', function () {
+    describe('prepareIcons', function () {

Review comment:
       Arrow functions shouldn't converted to `function ()` style.
   
   Additionally this change appears to be unrelated to the problem this PR is trying to solve. PRs should be concise to only include changes required to address a problem.

##########
File path: spec/unit/prepare.spec.js
##########
@@ -833,4 +834,78 @@ describe('prepare', () => {
             ).toBeResolved();
         });
     });
+
+    describe('updateProjectAccordingTo', function () {
+    describe('relocate MainActivity.java', function () {

Review comment:
       sub describe blocks should have an indentation level. Functions should be in an arrow function format.
   
   ```suggestion
       describe('updateProjectAccordingTo', () => {
       describe('relocate MainActivity.java', () => {
   ```

##########
File path: spec/unit/prepare.spec.js
##########
@@ -81,9 +82,9 @@ function mockGetIconItem (data) {
     }, data);
 }
 
-describe('prepare', () => {
-    describe('updateIcons method', function () {
-    // Rewire
+describe('prepare', function () {
+    describe('updateIcons', function () {
+        // Rewire

Review comment:
       Arrow functions shouldn't converted to `function ()` style.
   
   Additionally this change appears to be unrelated to the problem this PR is trying to solve. PRs should be concise to only include changes required to address a problem.

##########
File path: test/android/app/build.gradle
##########
@@ -25,7 +25,7 @@ android {
     buildToolsVersion "29.0.2"
 
     defaultConfig {
-        applicationId "org.apache.cordova.unittests"
+        applicationId "org.apache.cordova.unittests2"

Review comment:
       Why is this change required?

##########
File path: bin/templates/gradle.properties
##########
@@ -0,0 +1,4 @@
+org.gradle.daemon=true
+org.gradle.jvmargs=-Xmx2048m
+android.useAndroidX=false
+android.enableJetifier=false

Review comment:
       Why is this file being added here?

##########
File path: spec/unit/prepare.spec.js
##########
@@ -833,4 +834,78 @@ describe('prepare', () => {
             ).toBeResolved();
         });
     });
+
+    describe('updateProjectAccordingTo', function () {
+    describe('relocate MainActivity.java', function () {
+        // Rewire
+        let Api;
+        let api;
+        let prepare;
+
+        // Spies
+        let replaceFileContents;
+
+        // Mock Data
+        let cordovaProject;
+        let options;
+        let packageName;
+
+        beforeEach(function () {

Review comment:
       ```suggestion
           beforeEach(() => {
   ```




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org

[GitHub] [cordova-android] wedgberto closed pull request #1140: Mainactivity track packagename change

Posted by GitBox <gi...@apache.org>.
wedgberto closed pull request #1140:
URL: https://github.com/apache/cordova-android/pull/1140


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org

[GitHub] [cordova-android] wedgberto commented on a change in pull request #1140: Mainactivity track packagename change

Posted by GitBox <gi...@apache.org>.
wedgberto commented on a change in pull request #1140:
URL: https://github.com/apache/cordova-android/pull/1140#discussion_r535962824



##########
File path: spec/unit/prepare.spec.js
##########
@@ -81,9 +82,9 @@ function mockGetIconItem (data) {
     }, data);
 }
 
-describe('prepare', () => {
-    describe('updateIcons method', function () {
-    // Rewire
+describe('prepare', function () {
+    describe('updateIcons', function () {
+        // Rewire

Review comment:
       all non related changes have been reverted




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org

[GitHub] [cordova-android] wedgberto commented on a change in pull request #1140: Mainactivity track packagename change

Posted by GitBox <gi...@apache.org>.
wedgberto commented on a change in pull request #1140:
URL: https://github.com/apache/cordova-android/pull/1140#discussion_r535955300



##########
File path: test/android/app/src/main/java/org/apache/cordova/unittests/StandardActivity.java
##########
@@ -17,7 +17,7 @@ Licensed to the Apache Software Foundation (ASF) under one
        under the License.
 */
 
-package org.apache.cordova.unittests;
+package org.apache.cordova.unittests2;

Review comment:
       this PR is about being able to change the package name. I must have changed this for that reason but I don't remember if it is in fact needed so I will revert it




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org

[GitHub] [cordova-android] wedgberto closed pull request #1140: Mainactivity track packagename change

Posted by GitBox <gi...@apache.org>.
wedgberto closed pull request #1140:
URL: https://github.com/apache/cordova-android/pull/1140


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org

[GitHub] [cordova-android] wedgberto commented on a change in pull request #1140: Mainactivity track packagename change

Posted by GitBox <gi...@apache.org>.
wedgberto commented on a change in pull request #1140:
URL: https://github.com/apache/cordova-android/pull/1140#discussion_r535956926



##########
File path: spec/unit/prepare.spec.js
##########
@@ -833,4 +834,78 @@ describe('prepare', () => {
             ).toBeResolved();
         });
     });
+
+    describe('updateProjectAccordingTo', function () {
+    describe('relocate MainActivity.java', function () {
+        // Rewire
+        let Api;
+        let api;
+        let prepare;
+
+        // Spies
+        let replaceFileContents;
+
+        // Mock Data
+        let cordovaProject;
+        let options;
+        let packageName;
+
+        beforeEach(function () {

Review comment:
       another copy/paste artefact




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org

[GitHub] [cordova-android] breautek commented on a change in pull request #1140: Mainactivity track packagename change

Posted by GitBox <gi...@apache.org>.
breautek commented on a change in pull request #1140:
URL: https://github.com/apache/cordova-android/pull/1140#discussion_r536011685



##########
File path: bin/templates/gradle.properties
##########
@@ -0,0 +1,4 @@
+org.gradle.daemon=true
+org.gradle.jvmargs=-Xmx2048m
+android.useAndroidX=false
+android.enableJetifier=false

Review comment:
       You may be able to use spies to prevent actual code logic from running. I fear if unit tests causes side effects such as creating files, then they may not run in a consistent manner if the file already exists.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org