[GitHub] Jule- commented on a change in pull request #576: Improve target-dir restriction for detecting new android project…

[GitBox <gi...@apache.org>]

Jule- commented on a change in pull request #576: Improve target-dir restriction for detecting new android project…
URL: https://github.com/apache/cordova-android/pull/576#discussion_r236001446
 
 

 ##########
 File path: bin/templates/cordova/lib/pluginHandlers.js
 ##########
 @@ -320,7 +320,7 @@ function generateAttributeError (attribute, element, id) {
 function getInstallDestination (obj) {
     var APP_MAIN_PREFIX = 'app/src/main';
 
-    if (obj.targetDir.startsWith('app')) {
+    if (obj.targetDir.startsWith('app/')) {
 
 Review comment:
   Indeed... Maybe I'm wrong but the point is to detect old fashioned cordova plugins in order to append `app/src/main` in front of them in order to the source files to be recognized and to be compiled by the project.
   
   So if `target-dir` is starting with `app/` then it is a new fashioned plugin that meets new android project requirement and so new cordova-android requirements to put source files in `app/src/main/` (and I think this is what we should put in the `startWith` method!). 
   
   If not then it is an old fashioned plugin that doesn't meets new android project requirements. So if it starts by `app` like `appsomething` this should go to `app/src/main/appsomething`.
   
   I don't know if there is some plugin patterns that take advantage of custom `build.gradle` file in order to compile external android app structure project files but I think I have never see one yet.
   
   And even if plugin author decides to specify `target-dir="app"` I think it is again some old fashion way and we don't want to treat it like a new fashion way. I mean `<source-file />` is for **source files** and source files in android app structure are in at least `app/src/main/`. I think it makes sense. So whenever we have source file we want to put it there. Simple and reliable.
   
   ```js
   if (obj.targetDir.startsWith('app/src/main/')) {
     // plugin targets explicitly source file root => use targetDir as it is.
     return path.join(obj.targetDir, path.basename(obj.src));
   } else {
     // plugin ignores underlying android app structure
     // @deprecated: BACKWARD COMPATIBILITY
   
     // I don't have enough history on this part so I don't know if there is specific
     // folder to put files into in order to compile correctly but I guess:
     if (obj.src.endsWith('.java')) {
       return path.join(APP_MAIN_PREFIX, 'java', obj.targetDir.replace(/^src\//, ''),
         path.basename(obj.src));
     } else if (obj.src.endsWith('.aidl')) {
       return path.join(APP_MAIN_PREFIX, 'aidl', obj.targetDir.replace(/^src\//, ''),
         path.basename(obj.src));
     } else if (obj.targetDir.startsWith('libs/')) {
       if (obj.src.endsWith('.so')) {
         // Here we should have kept the old form with .substring(5) since it is safe 
         // after .startsWith('libs/').
         // But I don't think we need optimization here more than readability.
         return path.join(APP_MAIN_PREFIX, 'jniLibs', obj.targetDir.replace(/^libs\//, ''),
           path.basename(obj.src));
       } else {
         // Don't know what sort of lib this should be handling
         return path.join('app', obj.targetDir, path.basename(obj.src));
       }
     } else if (obj.targetDir.startsWith('src/main/')) {
       // Seems to be an awkward form handling
       return path.join('app', obj.targetDir, path.basename(obj.src));
     }
   
     // For all other source files not using the new app directory structure,
     // add 'app/src/main' to the targetDir
     return path.join(APP_MAIN_PREFIX, obj.targetDir, path.basename(obj.src));
   }
   ```

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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


With regards,
Apache Git Services

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