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 23:45:08 UTC
[GitHub] [cordova-android] breautek commented on a change in pull request #1140: Mainactivity track packagename change
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