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

Content providers #1119

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Content providers #1119

wants to merge 7 commits into from

Conversation

pavelsof
Copy link
Contributor

This PR complements kivy/python-for-android#2200, adding two config options that can be used in buildozer.spec:

  • android.manifest.content_providers expects a path to a file, the contents of which will be inserted into the <application> tag of the Android manifest.
  • android.add_xml_resources expects a list of file paths to be copied into the src/main/res/xml subdirectory of the build, so that they can be referenced in the Android manifest.

As an example of how it would look like from the viewpoint of a developer using Kivy, one may take a look at pavelsof/mobile-wormhole@d0c100e.

Copy link
Member

@AndreMiras AndreMiras left a comment

Choose a reason for hiding this comment

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

This is looking good.
I think inclement was following this on Discord.
May I just ask you to add unit tests in tests/targets/test_android.py. Buildozer coverage is already too low and I don't want things to get worst. So I'd like new feature to be covered.
Basically the test should simply check that the build command is reflected by your new parameters

@pavelsof
Copy link
Contributor Author

Thank you, @AndreMiras! No problem, I will try to add some tests (:

@AndreMiras
Copy link
Member

Thanks a mil! I'm happy to give some guidance if you need some help (I'm on Discord)

@pavelsof
Copy link
Contributor Author

I took the liberty to refactor the test_android module a bit in order to make it easier to add tests for particular config options. Please let me know whether it seems OK to you (:

AndreMiras
AndreMiras previously approved these changes May 23, 2020
Copy link
Member

@AndreMiras AndreMiras left a comment

Choose a reason for hiding this comment

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

Well done, yes I think the test refactoring makes sense the way you need it. Only added a minor comment.
I think the overall PR makes sense too and is good for a merge once the p4a one is merged kivy/python-for-android#2200

"""
self.temp_dir.cleanup()

def init_target(self, options={}):
Copy link
Member

Choose a reason for hiding this comment

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

default mutable arguments is not recommended: https://docs.python-guide.org/writing/gotchas/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, fixed it (:

Copy link
Member

@AndreMiras AndreMiras left a comment

Choose a reason for hiding this comment

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

Looking good thanks

@AndreMiras
Copy link
Member

Hi @pavelsof
Since this is not yet being reviewed/merged on p4a, but the test refactoring is interesting nonetheless, do you mind making another PR that address just that? Basically I'm about to address the --numeric-version buildozer side and I'd like to add tests on top of your refactoring.
Basically I need the tests/targets/test_android.py changes minus the new tests related to the new content providers feature (test_build_package_content_providers, test_build_package_add_xml_resources).
If you're too busy to make a PR that just does that I will do it.
Also if you're on Discord, what's your nickname so we can discuss the other PR directly?

@pavelsof
Copy link
Contributor Author

Hello @AndreMiras!

Fair enough, I will make another PR later today then :)
Do you know what is the problem with the other PR?
And I do not use Discord, but you have mentioned it already a few times, so I will take a look.

@AndreMiras
Copy link
Member

Thanks @pavelsof!
So for the other PR, the idea is really just to address the refactoring, I'll review/merge as soon as you would post it and then you will be able to rebase this PR on top of it.
Yes sorry for Discord, it's painful to have another messaging service here, but it could be useful for developments discussions sometimes.
I don't think there's a problem with the other PR (the p4a one), the only thing is I'm not comfortable with content providers yet, so I was expecting another dev to review it, but they look busy. Or maybe they just haven't seen it. I think it's only that and Discord can also help in that sense

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

Successfully merging this pull request may close these issues.

2 participants