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 #2200

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Content providers #2200

wants to merge 3 commits into from

Conversation

pavelsof
Copy link
Contributor

This PR should make it possible to add content providers to the Android manifest. Two options are added to the common build.py script:

  • --content-providers is practically identical to the --intent-filters option, it expects the path to an XML file the contents of which are included in the AndroidManifest.xml.
  • --add-xml-resource can be used to copy one or more files over to the src/main/res/xml subdir of the build dir. This is needed because <provider> elements in the Android manifest usually have to refer to another XML file that defines the paths the app is allowed to "export".

If approved, I will also prepare a PR for the Buildozer repo to add the buildozer.spec counterparts.

@pavelsof
Copy link
Contributor Author

Just opened kivy/buildozer#1119 as an example of how the two new build.py options could be used by Buildozer.

Please consider following up on either this PR here or on #1922, so that there is an easy way to setup content providers in a Kivy app.

@AndreMiras AndreMiras requested a review from inclement May 21, 2020 15:30
@AndreMiras
Copy link
Member

Thanks @pavelsof for the PR, looks clean.
I'm thinking, would it make sense to make the flag name more generic e.g. --extra-application-xml?
Because your application actually looks generic enough to cover other cases.
I'm wondering what @inclement thinks about it 🤔

@@ -92,6 +92,10 @@
</intent-filter>
</receiver>
{% endif %}

{%- if args.content_providers -%}
Copy link
Member

Choose a reason for hiding this comment

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

Why {%- rather than just {% (and same elsewhere)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No other reason apart from not being familiar with Jinja and copy-pasting the lines that render the intent filters. I will fix it.

'filename containing xml. The filename should be '
'located relative to the python-for-android '
'directory'))
ap.add_argument('--add-xml-resource', dest='add_xml_resource',
Copy link
Member

Choose a reason for hiding this comment

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

Didn't we add something else recently for moving stuff into the src/main/res folder?
I'm wondering if what we really want is a generic "copy this file/folder into src/main/res" argument, rather than many arguments for different subfolders.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I can replace it with a more generic argument to copy files/directories into src/main/res.

@Fak3
Copy link
Contributor

Fak3 commented May 30, 2020

I believe this is more generic version of my pr #1922. Maybe this one is better, as it is more flexible. But please note that I had to add more Gradle build flags for this to work. (See my original pr)

@pavelsof
Copy link
Contributor Author

pavelsof commented Jun 2, 2020

@AndreMiras, I agree, it would be better if the option name makes it clear that it can be used to insert arbitrary XML in the <application> element. Should I change it?

@Fak3, thank you for jumping in! Indeed, just as you did, I also had to add com.android.support:support-v4:26.1.0 as a dependency in Gradle; but I used the android.gradle_dependencies option in buildozer.spec. Also, I did not have to enable multidex.

@AndreMiras
Copy link
Member

Thanks guys for the discussion!
@pavelsof if #1922 would work for you use case and @Fak3 is willing to resume his work on it then we could go for it instead.
If we were to integrate yours, then yes I would say let's make the parameters a bit more generic and add unit tests.
Otherwise if we really need both then we would go for both.

@pavelsof
Copy link
Contributor Author

pavelsof commented Jun 2, 2020

@AndreMiras, yes, #1922 works for me. It would be slightly easier to use as well, because a Kivy developer would only need the paths XML, but not the <provider> boilerplate.

@RobertFlatt
Copy link
Contributor

This would be great if this was signed off and 'dozer supported the args, I just did it by hand and inserted :

    <provider
        android:name="android.support.v4.content.FileProvider"
        android:authorities="{{ args.package }}.fileprovider"
        android:grantUriPermissions="true"
        android:exported="false">
        <meta-data
            android:name="android.support.FILE_PROVIDER_PATHS"
            android:resource="@xml/provider_paths" />
    </provider>

Also I used
android.gradle_dependencies = "com.android.support:support-core-utils:27.0.0"

I tested build and execution on api 27, 29, and 30 all good

Arbitrary html would be a good thing in general.
And this case androidx support will require a change the android:name field value

@RobertFlatt
Copy link
Contributor

FYI, the steps to move from the depreciated packages to androidx #2020 (comment)

@pavelsof
Copy link
Contributor Author

Ping! This is a polite reminder/request to please merge this PR. I have been using my python-for-android and Buildozer forks for a pet Kivy app, and there have not been problems for what is now almost a year. Content providers constitute an important thing in Android and Kivy should be able to work with them.

@Fak3
Copy link
Contributor

Fak3 commented Jan 24, 2021

Ping! This is a polite reminder/request to please merge this PR.

I believe we have to fix #2385 first, it would not work without it

@pavelsof
Copy link
Contributor Author

It works (at least for me) with the older, probably deprecated by now, android.support.v4.content way. But I agree, AndroidX would be much better.

What can we do to prod the core maintainers to say what is wrong with either PR? 😕

@RobertFlatt
Copy link
Contributor

What can we do to prod the core maintainers to say what is wrong with either PR? 😕

I wish I knew the answer to that question 🤔

If #2385 where approved (sigh) then support here for both versions might look like this:

	{% if args.enable_androidx %}
	android:name="androidx.core.content.FileProvider"
	{% else %}
	android:name="android.support.v4.content.FileProvider"
	{% endif %}	

@RobertFlatt
Copy link
Contributor

As an enhancement I suggest a default provider_paths.xml
This would be a lot easier for the script kiddies.

For example the attached provide_paths.xml that provides all the default directories in both local storage directories.
So a custom provider_paths.xml would only be required if the user makes a custom directory, this is easy to explain.
provider_paths.txt (renamed from .xml to .txt to keep Github happy).

File belongs in pythonforandroid/bootstraps/sdl2/build/src/main/res/xml which must be created.
I have not checked if an explict copy is required in the build.

@darpan5552
Copy link

Instead of Fileprovider there can be other conditions also where we need to add extra entries to Manifest. Furthermore there is no guarantee that in future there will not be more such important requirements.

Why not keep everything thing as xml file as the boilerplate code added to manifest also follows xml syntax.

Adding two parameters --extend-manifest <something.xml> and --add-xml-resource <something 2.xml> should do the thing.

@RobertFlatt
Copy link
Contributor

Is this what you want?
https://github.com/kivy/python-for-android/blob/develop/pythonforandroid/bootstraps/sdl2/build/templates/AndroidManifest.tmpl.xml#L45

Also there is at least one xml related PR #2330 that sits and rots :(

@darpan5552
Copy link

darpan5552 commented Mar 11, 2021

Is this what you want?
https://github.com/kivy/python-for-android/blob/develop/pythonforandroid/bootstraps/sdl2/build/templates/AndroidManifest.tmpl.xml#L45

Yes, i wanted this. It would be better if it also integrates addition of intent-filters in it. I know both are at different levels in xml tree but there are not too much use cases that in can't be handled on p4a side.

Also there is at least one xml related PR #2330 that sits and rots :(

#2330 is also a very specific case where he wants to insert intent filters manually but automating the modification of manifest file.
In contrast, i am on the side of No Automation Policy as there can be large cases for such automation. A better way could be to allow the user to write modification and we just push the modifications in its place.

@Fak3
Copy link
Contributor

Fak3 commented Mar 11, 2021

I think the most generic thing to address all cases will be to allow p4a use custom manifest template file, via an option --manifest=/path/to/mymanifest.xml.tmpl

@darpan5552
Copy link

darpan5552 commented Mar 11, 2021 via email

@HeRo002
Copy link

HeRo002 commented Apr 7, 2022

I just asked "Has content providers been incorporated in p4a?" on https://groups.google.com/g/kivy-users/c/7VVYdkA0gjQ - and received this answer by RobertFlatt :

I think the whole content provider thing faded to irrelevance, turned out that the MediaStore can provide a uri.

I wrote this as a test case: https://github.com/Android-for-Python/Share-Send-Example

@Fak3
Copy link
Contributor

Fak3 commented Apr 7, 2022

So how this can help with plyer camera, which requires fileprovider to work?

@RobertFlatt
Copy link
Contributor

@Fak3 You want to save an image to shared storage on Android >= 10?
Write a temp image to local storage and then copy that to the MediaStore.

I wrote a Camera Widget that on Android can save screen shots to public storage (for photos the widget uses native image capture that handles this). https://github.com/Android-for-Python/Camera4Kivy

The save and copy code is here https://github.com/Android-for-Python/Camera4Kivy/blob/main/src/camera4kivy/preview_camerax.py#L224-L253

@Fak3
Copy link
Contributor

Fak3 commented Apr 8, 2022

I want it to be compatible with Android 7

@RobertFlatt
Copy link
Contributor

You don't say what "it" is, but if "it" is save a file then on Android <10 shared storage is a file system.

@Fak3
Copy link
Contributor

Fak3 commented Apr 8, 2022

I want to capture a photo with external camera app. On Android 7 it throws an error when you just try to give an external app a filesystem path to save an image to. It only works if you properly define fileprovider in your Android manifest.

@Fak3
Copy link
Contributor

Fak3 commented Apr 8, 2022

See kivy/plyer#549

@RobertFlatt
Copy link
Contributor

Yep, Plyer Camera looks like it has had bit rot for several years. The design approach is obsolete.

I see that you would have that issue.

@Fak3
Copy link
Contributor

Fak3 commented Apr 8, 2022

Working approach for Android 7 is still needed. It will take years for users to switch to Android 10

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.

7 participants