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

fix(android): move MainActivity.java to folder that tracks the app package name (widget id) #1154

Merged

Conversation

wedgberto
Copy link
Contributor

@wedgberto wedgberto commented Jan 20, 2021

Platforms affected

Android

Motivation and Context

Cordova has built in hook functionality that enables customisation of the build. My project has used this hook feature to change the package name via the build command line arguments so that the app can be built with the same code but different package names. e.g. io.cordova.helloworld can be installed and ran alongside io.cordova.helloworld2.

This is very useful for installing test, UAT or beta releases side by side. This was possible in [email protected] but was broken by changes made in 9.0.0.

When the package name of the project is changed in config.xml after a cordova build has completed, and before another build, 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 source code for a class that extends CordovaActivity no longer exists.

Subsequent build attempts of the project fail because the source code for a class that extends CordovaActivity is missing from the project.

Fixes #1139

Description

This PR reinstates code that took care of moving the Java source code file containing the class that extends CordovaActivity to a folder path that matches the app's package name (e.g. app/src/main/java/io/cordova/helloworld).

Testing

  1. Create a new project and set its package name to io.cordova.helloworld
  2. Build the project for Android (cordova build android)
  3. Edit config.xml and set the package name to io.cordova.helloword2
  4. Build the project for Android again (cordova build android)
  5. The app will build but will crash when launched on an Android device because MainActivity is missing
  6. Build the project for Android again (yes, a third time!) (cordova build android)
  7. This time, the build fails with a compiler error (no class found that extends CordovaAcivity)

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

@codecov-io
Copy link

codecov-io commented Jan 20, 2021

Codecov Report

Merging #1154 (4625c33) into master (7428bd3) will increase coverage by 1.99%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1154      +/-   ##
==========================================
+ Coverage   71.80%   73.80%   +1.99%     
==========================================
  Files          21       21              
  Lines        1745     1752       +7     
==========================================
+ Hits         1253     1293      +40     
+ Misses        492      459      -33     
Impacted Files Coverage Δ
bin/templates/cordova/lib/prepare.js 59.07% <100.00%> (+11.27%) ⬆️

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 7428bd3...4625c33. Read the comment docs.

@wedgberto wedgberto changed the title Move MainActivity.java to folder that tracks the app package name (widget id) (android) Move MainActivity.java to folder that tracks the app package name (widget id) Apr 23, 2021
@Davilink
Copy link

Davilink commented Jun 7, 2021

I was attempting to do the same thing that you are doing, but discover the same problem that this pr want to fix.

.gitignore Outdated Show resolved Hide resolved
@wedgberto
Copy link
Contributor Author

@erisu @breautek please review this

@breautek breautek added this to the 11.0.0 milestone May 11, 2022
@breautek
Copy link
Contributor

Hi @wedgberto

I would like to include this with our next 11.x release, but as this PR is old and didn't get much attention when it should have, it will need to be rebased. Do you think you'll have time in the upcoming weeks to rebase? If not, do I have permission to rebase/cherry-pick your commits to get these changes in?

Kind regards,
Norman

@breautek breautek added the bug label May 11, 2022
@wedgberto
Copy link
Contributor Author

wedgberto commented May 12, 2022 via email

@breautek
Copy link
Contributor

Thanks! Please ping me when it's ready.

@wedgberto wedgberto force-pushed the mainactivity_track_packagename_from_master branch from 37d550e to 5ee340c Compare May 17, 2022 22:28
@wedgberto wedgberto force-pushed the mainactivity_track_packagename_from_master branch from 5ee340c to 98f4fb1 Compare May 18, 2022 00:58
@wedgberto
Copy link
Contributor Author

@breautek PR is now up to date with master

@codecov-commenter
Copy link

codecov-commenter commented May 18, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.90%. Comparing base (e730000) to head (24aeb85).
Report is 81 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1154      +/-   ##
==========================================
+ Coverage   73.20%   75.90%   +2.69%     
==========================================
  Files          21       21              
  Lines        1646     1656      +10     
==========================================
+ Hits         1205     1257      +52     
+ Misses        441      399      -42     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

Changes looks good, tested it locally and everything seems to work as expected + unit tests including your new unit tests passes.

I've just have one nit pick where you're using a package that isn't declared as a cordova-android dependency.

Thanks for your effort into preparing this PR.

spec/unit/prepare.spec.js Show resolved Hide resolved
@erisu erisu changed the title (android) Move MainActivity.java to folder that tracks the app package name (widget id) fix(android): move MainActivity.java to folder that tracks the app package name (widget id) May 18, 2022
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.

lgtm

Copy link
Member

@erisu erisu left a comment

Choose a reason for hiding this comment

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

Code LGTM

@breautek
Copy link
Contributor

All checks remain green. Thank you @erisu for your green check and thank you @wedgberto for your effort into preparing this PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MainActivity.java file deleted on build when package name is changed in config.xml
6 participants