Android file browser

[t-arn]

This PR implements Window.open_file_dialog and Window.select_folder_dialog for Android.
It uses the Android Storage Access Framework (SAF) which works with content URIs.
So, open_file_dialog and select_folder_dialog return URIs, not paths like on other platforms.
Also, the user will need to use the SAF for reading/writing files.
The downside of this is that the user's code will be platform specific, but the advantage is that the user will actually be allowed to read from the chosen location and he also has access to all supported locations (e.g. also Cloud storage if supported by the device).
If the user's code should remain cross platform, it is pretty easy to create a real file path from the returned URI and then use the standard file I/O methods.

The PR includes a generic Intent invocation method (app.invoke_intent_for_result) which can be used for invoking arbitrary Android Intents. There is a new example (example/filebrowser) which I used for testing it all.
The example also shows how to use the exposed Intents of OpenIntent Filemanager.
(EDIT: The filebrowser example was removed in commit b4bc066)

Important:
For this example to work under Android, you need a briefcase android template
which supports onActivityResult in MainActivity.java
see this PR: beeware/briefcase-android-gradle-template#25

PR Checklist:

• All new features have been tested
• All new features have been documented
• I have read the CONTRIBUTING.md file
• I will abide by the code of conduct

[paulproteus]

Hi @t-arn! This is a great change overall!

With regard to this line, it might be nice to file a rubicon-java bug requesting string arrays be passable-in somehow. Based on beeware/rubicon-java#53 it should be possible to do that.

It doesn't have to block this PR!

[t-arn]

OK, I filed a rubicon enhancement request:
beeware/rubicon-java#55

[paulproteus]

This is awesome :D

[t-arn]

Yes, I also like that method. You can use it for calling any Intent you like.
In the example, I use it for calling the OI Filemanager Intents just to show how it can be used directly.

[paulproteus]

This code smoothly handles the cancellation possibility!

[paulproteus]

This is a great use of counters to avoid object leaks, and excellent bookkeeping of the running_intents dict.

[paulproteus]

I like to do return await result_future as one line, rather than two lines like you do here on 129-130. Your way is fine, too, so not a blocker, but just something that occurred to me.

[t-arn]

Well, I do not return result_future, I need to return result_future.result()
And return await result_future.result() did not work.

[paulproteus]

These c:/ paths will have to be replaced with names that work on others' machines, e.g. toga-core here, I believe.

I definitely appreciate that it's finnicky to swap these in when you're developing toga, then swap them out to match the style of other examples' pyproject.toml.

[t-arn]

Yeah, you're right there.
I just did not know, whether we will keep the filebrowser example or if we just use it until the PR is accepted.
There actually is already an example for dialogs, but I didn't want to change that completly (due to the async nature).
Besides, the existing dialog examle might have a too large height for Android where scroll view is not yet working.
Some controls might acutally be outside the visible area.

[paulproteus]

I haven't manually tested this example, and I've only looked at the Android parts of it, but those Android parts are stellar! I'll let @freakboy3742 comment on the rest.

I'm hopeful to find some time to manually test this sometime, but I don't know exactly when.

[freakboy3742]

The core of this looks really good. A few notes inline - mostly minor stuff. I think there might be a bug in the way multi-select is being handled; if the mistake is my reading of the code, then we definitely need some explanatory comments inline about the return value of getData() and getClipData() to clarify how the logic flows.

The new demo app is a nice touch (and having a clear test path is appreciated) - but in this case, it shouldn't be required. The dialogs demo already has a test of the file open dialogs; if there's a feature that you need to test that isn't exercised by that demo, preference is to modify the existing app rather than add a new one.

Lastly - your comments on the PR say:

If the user's code should remain cross platform, it is pretty easy to create a real file path from the returned URI and then use the standard file I/O methods.

Toga's externally facing API should always be cross platform - so, preference would be to do this conversion in the Android backend and return a path that is compatible with standard Python file methods.

[freakboy3742]

For logging purposes, it might be helpful to include requestCode and resultCode here.

[t-arn]

preference would be to do this conversion in the Android backend and return a path that is compatible with standard Python file methods

Well, I have thought about this as well and it could be done pretty easily.
But then, we would need to reject all files selected from content providers except for the local storage content provider.
So no Cloud, no image provider, maybe not even downloads provider.

In addition there is the problem that in Android 11 the file I/O will not be supported anymore on external storage:
https://developer.android.com/about/versions/11/privacy/storage
So, using file paths on Android should be considered deprecated already.

That's why I think we should stick to the content URI params and result of open_file_dialog.
I also mentioned in the PR description that the user will need to use SAF method to actually read and write files.
This can be achieved by calling them through app._impl.whatever but I really don't like this idea!
I therefore suggest that in Toga, we should use abstract "file identifier strings" to reference files.
On Android, they would be content URIs, on all other platforms, they would be file paths.
We could then add some high-level cross-platform file-util module for accessing files.
The module would accept the "file identifier strings" and support following methods:
read_text_file (with handling encoding and EOL conversion)
read_binary_file
write_text_file (with handling encoding and EOL conversion)
write_binary_file
delete_file
create_folder
delete_folder

Just my 2 cents...

[freakboy3742]

Hrm. That's... annoying... but equally annoying is that the reasons that make a lot of sense. :-)

I agree that a high level "just get me the file content" API would be extremely useful. The general problem of "download this file in the background" is a universal one; it's just a little more obvious on mobile (and Android is clearly forcing the issue by locking down the security on those APIs). However, this does give us an opportunity to provide an API that is Pathlib/File duck-compatible, but abstracts network (or file) access.

We don't need to solve that problem right now - but we do need to work out how to not box ourselves into a corner for a future where this API exists.

I'm not wild about the idea of Android returning a different type of result than other platforms; however, I'm also not completely sure I have a better idea as an alternative. I guess it's easy enough to distinguish content:// and file:// from /home/myname and c:\Users...

[t-arn]

OK, I added some method documentation in toga\window.py open_file_dialog and select_folder_dialog to make it clear to the user that on Android, he should work with content URIs

[freakboy3742]

pop returns the object that was popped, so these two lines can be combined.

[t-arn]

Cool, I learned something here :-)

[freakboy3742]

These declarations should be moved to toga_android.libs so they aren't accidentally declared multiple times.

[t-arn]

Yes, I saw that there are lots of such declarations in libs/android_widgets.py
I just didn't know whether you wanted to have them there because my declarations are not widgets...
Maybe, we should rename libs/android_widgets.py to libs/android_classes.py maybe?

[freakboy3742]

That's a good point. I'd go even simpler - just call it android.py, and then organise the internals of the file by Java class structure - i.e., put in (alphabetically ordered) sections for android.app.*, android.content.*, android.widget.*, and so on. That will end up with one very large file - but it will be easier to determine if a class has already been wrapped.

[t-arn]

Should I create a new file libs/android.py with just my classes or should I rename the existing libs/android_widget.py?
Renaming sounds dangerous. Wouldn't that break existing code?

[t-arn]

I now created a new file with only my declaration.
I'll leave it to you whether you want to merge the two files...and take the responsibility that everything still works afterwards :-)

[freakboy3742]

Why the conversion to str for the dictionary key?

[t-arn]

Are keys not required to be strings in Python dictionaries?

[t-arn]

No, I just found that keys can be of any type...I'm still a Python newbie, I'm afraid..
I'll use the int values as keys, then

[freakboy3742]

Minor clarification - they dict keys can be any hashable type. You can't use a dictionary as a hash key, for example, because the dict itself can change, so you can't generate a consistent hash. But integers, strings, floats, most simple objects, and tuples of those primitives are all fair game as dict keys.

[freakboy3742]

Would be worth putting in a NotImplemented call here so that the end-user sees a manifestation of this limitation in the logs.

[t-arn]

ok, will do

[freakboy3742]

is True is a redundant expression here; also - where is the else clause?

[t-arn]

When multiselect is False, then there is no clip_data and the result is already in selected_uri

[freakboy3742]

Ah - ok; I think I see how it works now. There's definitely a need for some inline documentation to explain the logic here - in particular, what the return types are, and how they interact with multiselect.

[t-arn]

OK, added some inline documentation

[freakboy3742]

This seems like it's not at the right indent level... is this the else for the multiselect?

[t-arn]

No, the problem is that even when multiselect is True, the user might only select 1 file which will be returned with getData() (and clip_data will be empty). Only when multiselect is True AND the user selected several files, clip_data will contain the list with the files (and getData will be None)

[freakboy3742]

A few more cleanups flagged in the docstrings.

The other big subject is the extra demo that has been added. I raised this as an item in my previous review. It isn't clear to me why a new demo is required - and if there are capabilities that need to be demonstrated, then it shouldn't be an "Android Demo" for the purposes of the final patch. So - why is an additional demo app required? Why can this not be integrated into the existing dialogs demo?

[freakboy3742]

Platform specific instructions like these aren't workable in a cross-platform toolkit. We need to find a common format that will work for all platforms, even if some data won't be used on some platforms.

[t-arn]

Why I made a specific example? Because the methods do_open_file and do_open_folder need to be async for Android. I tried to just add async to the action_open_file_dialog method of the dialogs example, but then the method did not work anymore on Windows.

And: the demo shows how to call arbitrary Intents.

You can remove the filebrowser example if you want - I only needed it for testing the PR

[t-arn]

I made some tests with the dialogs example. I can make it work on Android and Windows by doing this:

        if toga.platform.current_platform == 'android':
            btn_open = toga.Button('Open File', on_press=self.action_open_file_dialog_android, style=btn_style)
        else:
            btn_open = toga.Button('Open File', on_press=self.action_open_file_dialog, style=btn_style)

and then defining action_open_file_dialog_android as async

So, yes, I can do without the filebrowser example.

[t-arn]

OK, I now extended the dialogs example so it works on Android (and still works on the other platforms).
So, I will remove the filebrowser example in the next push.

[freakboy3742]

I understand that URIs are required for Android support, but specific references to Android in the platform-agnostic API layer isn't desirable.

We should either (a) double down on this, say the return value here is "a file path or URI", and expect the user to support both; or (b) say that it returns a path, and leave it as a footnote that this will be a URI in some contexts. (a) is committing to a permanent solution; (b) would a precursor to a longer term "file access API" that we discussed in chat.

I think I'm leaning towards (b), even if we can only get as far as documenting the longer term plan as a ticket.

[freakboy3742]

Yes - but why will there be a null reference exception? Docstrings should be about the why, not the what. Dif size == 3: # check if size is 3 doesn't tell me anything I couldn't work out by reading the code; # size 3 is prohibited by the standard tells me why the code exists.

[t-arn]

Well, I'm not sure if there ever will be a null pointer exception.
On the other hand, I'm also not sure that clip_data will always be not-null when resultData is null...

[freakboy3742]

There's an analogous need for docstrings in this implementation; we can't assume someone will read both methods.

[mhsmith]

The invoke_intent_for_result method (renamed to intent_result), was merged in #1217, but still isn't documented (#1798). It can be used to launch a native file open dialog like this:

from android.content import Intent
from java import jarray, jbyte

fileChose = Intent(Intent.ACTION_GET_CONTENT)
fileChose.addCategory(Intent.CATEGORY_OPENABLE)
fileChose.setType("*/*")

# Assuming `app` is your toga.App object
results = await app._impl.intent_result(Intent.createChooser(fileChose, "Choose a file"))  
data = results['resultData'].getData()
context = app._impl.native
stream = context.getContentResolver().openInputStream(data)

def read_stream(stream):
    block = jarray(jbyte)(1024 * 1024)
    blocks = []
    while True:
        bytes_read = stream.read(block)
        if bytes_read == -1:
            return b"".join(blocks)
        else:
            blocks.append(bytes(block)[:bytes_read])

content = read_stream(stream)

[xymsyh]

thanks for your code. it helpd me!

[mhsmith]

It looks like the recommended intent for saving files is ACTION_CREATE_DOCUMENT. I don't have an example in Python, but here's one in Java: https://gist.github.com/neonankiti/05922cf0a44108a2e2732671ed9ef386.