Skip to content
This repository has been archived by the owner on Oct 24, 2022. It is now read-only.

Add workaround for using setting strings in cordova-android 7.x #716

Merged
merged 1 commit into from
Dec 15, 2018
Merged

Conversation

adipascu
Copy link

@adipascu adipascu commented Oct 15, 2018

Looks like cordova-android 7 does not correctly prefix the target resource with app/src/main/.
This PR keeps the old correct target and adds another target that includes the full project path.

@peterpeterparker
Copy link
Collaborator

Thx for the PR. It's a duplicate of #601 which wasn't merged, see discussion in this PR and also related issues #599

P.S.: Note also, if a PR would be merged, the compatibility with cordova-android < 7 has to be tested too

@adipascu
Copy link
Author

I gave this PR a bit more thought and I think this should be fixed in cordova-android. Adding workarounds here will just cause more issues in the future.

I could not find any relevant issue in the cordova-android tracker.

Thanks, @peterpeterparker!

@adipascu adipascu closed this Oct 16, 2018
@peterpeterparker
Copy link
Collaborator

@adipascu thx for the feedback and thx in any case for submitting a PR, highly appreciated!

@adipascu
Copy link
Author

adipascu commented Dec 9, 2018

Hi again @peterpeterparker .
I read #601 , its change-log contains more than my PR.

It seems like cordova-android 7 changed its project structure.

I have seen cordova-android 7 adopt some workarounds to be compatible with plugins targeting 6.x, but this does not cover whats being used in cordova-plugin-facebook4 (cordova-android#550, cordova-android#549)

I have tested this PR with cordova-android versions:

  • 6.4.0
  • 5.2.2

I have rebased and cleaned up the PR a bit. Looking forward!

@adipascu adipascu reopened this Dec 9, 2018
@peterpeterparker
Copy link
Collaborator

@adipascu sounds interesting, I like this PR because it seems to handle both Cordova 6 and >= 7 and update the doc in the meantime.

In #601 @amritk also suggest to update the path of AndroidManifest.xml which you didn't, could you please extend and test that too ?

@adipascu
Copy link
Author

Currently the path of AndroidManifest.xml relies on cordova-android 7 providing some degree backwards compatibility with cordova-android 6.
If we fix all the paths that use legacy targeting we would have quite a bit of duplicate code.
For "proper" cordova 7 compatibility we would also need to update:

  • L37
  • L50
  • L51 (current PR, this is not covered by cordova 7 backwards compatibility)
  • L56
  • L67

I suggest doing the least amount of changes required.

@peterpeterparker
Copy link
Collaborator

@adipascu I'm agree. I'll try to have a test with your PR today, if it works fine with my app, I'll merge it, close other PR and issues, release a new version of the plugin etc.

But, if I do so, could I count on you in case some users afterwards would notice some problems in case it would need improvements and fixes regarding this topic? I would appreciate that help

@adipascu
Copy link
Author

Yeah, I'll be active on github, if you need more info you can contact me by email (its in the git log)

@peterpeterparker peterpeterparker merged commit 2d782b7 into jeduan:master Dec 15, 2018
@peterpeterparker
Copy link
Collaborator

looks good, thx for the PR @adipascu 👍 it has been released in v3.3.0

@peterpeterparker peterpeterparker mentioned this pull request Dec 15, 2018
@adipascu
Copy link
Author

@peterpeterparker
People need to remove the previous workaround when updating to 3.3.0
This might be considered a breaking change (config level, not api level).

@peterpeterparker
Copy link
Collaborator

@adipascu at first I thought too I would release a major version but then I thought that you don't "need" but that you "could" remove the workaround

I mean if you still let the workaround in your config.xml then nothing bad will happens, therefore I finally thought that it isn't a breaking change

@adipascu
Copy link
Author

adipascu commented Dec 15, 2018

If somebody keeps the old workaround while on v3.3.0, they end up with the config in both strings.xml (the workaround) and facebookconnect.xml (config loaded with v3.3.0).
This results in the android build failing with Error: Duplicate resources.

I just tested the app with a fresh platforms/plugins install to confirm.

@peterpeterparker
Copy link
Collaborator

@adipascu v3.3.0 unpublished from npm and removed from the plugin, instead I just published v4.0.0 with a note about the breaking change in the CHANGELOG and release notes

Lindsay-Needs-Sleep pushed a commit to miloproductionsinc/cordova-plugin-facebook-connect that referenced this pull request Mar 7, 2020
Lindsay-Needs-Sleep pushed a commit to miloproductionsinc/cordova-plugin-facebook-connect that referenced this pull request Mar 7, 2020
Lindsay-Needs-Sleep pushed a commit to miloproductionsinc/cordova-plugin-facebook-connect that referenced this pull request Mar 7, 2020
Lindsay-Needs-Sleep pushed a commit to miloproductionsinc/cordova-plugin-facebook-connect that referenced this pull request Mar 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants