A utility module has been created to work with android external storage files by SnayperTihCreator · Pull Request #2910 · beeware/toga

SnayperTihCreator · 2024-10-15T18:22:30Z

Решил заняться реализацией файлового диалога для андроид

…ge files

freakboy3742

Thanks for the contribution; however, there's a few issues here.

Firstly, have a read of our contribution guide, and in particular the section on submitting a pull request - there's a couple of additional pieces that we require in a pull request before it will pass CI.

Most of these are related to code style - you've altered some existing indentation from the preferred style. The changes are consistent with a formatting style imposed by PyCharm; I don't know if that's what you've done, but Toga uses the black code style.

Second, you've added some docstrings in Russian; we require all code and docs to be in English. Those are also getting caught by the precommit checks - mostly because of the choice of variable names like ist. We try to have more meaningful variables names (like input_stream), rather than trying to abbreviate. The same goes for the uf import alias - that's not a pattern we generally use.

Lastly - we get to the actual implementation. The VFile object you've created isn't quite the API that is required here. In your code, VFile represents an already open file; the existing Dialogs API returns a Path object that can be opened and manipulated. This also allows the user to read a file in text or binary mode. Any API here needs to be modelled after the existing API. The correct abstraction here is an object that wraps the contentResolver(), on which there is an open() context manager that returns a readable/writable object (depending on the args passed to open()), or to do that read/write in binary mode.

It would be worth reviewing the prior art related to this feature request: see #1171 and #1158 for details.

freakboy3742 · 2024-10-15T23:05:43Z

android/src/toga_android/libs/utilfile.py

+BufferedReader = java.jclass("java.io.BufferedReader")
+InputStreamReader = java.jclass("java.io.InputStreamReader")
+Objects = java.jclass("java.util.Objects")


You should be able to use from java.io import BufferedReader etc here.

Thank you for the information

freakboy3742 · 2024-10-15T23:18:40Z

android/src/toga_android/dialogs.py



 class OpenFileDialog(BaseDialog):
+    class HandlerOpenDialog(uf.HandlerFileDialog):


Unless there's a reason for nesting classes like this, we generally keep classes at the module level.

Here is the reason for the class to be used only with OpenFileDialog

That's not a reason to nest classes. It complicates the class definition, and makes debugging harder because there's implicit context (and potentially closures) that can contribute to any bug being observed.

freakboy3742 · 2024-10-15T23:19:28Z

android/src/toga_android/libs/utilfile.py

+Objects = java.jclass("java.util.Objects")
+
+
+class HandlerFileDialog(abc.ABC):


It's not clear to me why this is in utilfile.py, rather than part of the dialogs class.

I was just afraid to change something in the main file.

Ok... but it's a dialog class. It should be with dialogs, not with utilities for file handling.

freakboy3742 · 2024-10-15T23:20:08Z

android/src/toga_android/dialogs.py

+
+    def _handler(self, uri):
+        ist = self.mActive.getContentResolver().openInputStream(uri)
+        isr = uf.InputStreamReader(uf.Objects.requireNonNull(ist))


InputStreamReader is a java class; it should be imported from the Java namespace, not implicitly from the utilfile namespace.

I will change the import

freakboy3742 · 2024-10-15T23:27:31Z

changes/2910.misc.rst

@@ -0,0 +1 @@
+Partial OpenFileDialog implementation


The changelog message should be written in the form of a potential release note for the new feature. In this case, it should be a .feature (this isn't a small miscellaneous fix - it's a major new feature), and should read something like:

Suggested change

Partial OpenFileDialog implementation

Android now has support for OpenFileDialog.

And here I didn't expect it to be a major new feature. Because I looked at it and saw how a partial implementation of opening files

Generalization implementation of file interaction

freakboy3742

Thanks for those updates; however, there's still a lot more work that needs to be done (some of which relates to review comments I gave last time).

Fundamentally, it feels like you're missing part of the high-level architecture of Toga. The parts that are literally Android code seem OK; but that's never been the complication preventing this feature from being added. As noted in my last review, there's a "bigger picture" feature that needs to be addressed - #1171. That isn't an "Android" feature - it's something that needs to be designed and integrated as a high-level API throughout Toga's file handling. I'd almost suggest it needs to be implemented entirely independently, as a pre-requisite for implementing any Android File APIs.

freakboy3742 · 2024-12-17T00:05:04Z

android/src/toga_android/libs/utilfile.py

+
+    @abc.abstractmethod
+    def show(self):
+        """Запуск менеджера"""


As noted previously, we need the code to be documented in English.

freakboy3742 · 2024-12-17T00:05:26Z

android/src/toga_android/dialogs.py



 class OpenFileDialog(BaseDialog):
+    class HandlerOpenDialog(utilfile.HandlerFileDialog):


As noted previously, there's no reason for this class to be nested.

freakboy3742 · 2024-12-17T00:06:32Z

android/src/toga_android/libs/utilfile.py

+Objects = java.jclass("java.util.Objects")
+
+
+class HandlerFileDialog(abc.ABC):


Ok... but it's a dialog class. It should be with dialogs, not with utilities for file handling.

freakboy3742 · 2024-12-17T00:07:54Z

android/src/toga_android/libs/utilfile.py

+        pass
+
+
+class BasePath(io.TextIOBase, abc.ABC):


This seems like something that should be a core-level utility, not something that is Android specific. It's also unclear why an object named Path would be subclassing io.TextIOBase - is it a path, or a text-readable IO object?

freakboy3742 · 2024-12-17T00:08:31Z

android/src/toga_android/libs/utilfile.py

+
+
+class PathReader(BasePathReader):
+    """A phalloid object for reading.


A what? Are you sure this is the word you mean?

freakboy3742 · 2024-12-17T00:09:31Z

android/src/toga_android/managersFileSystem.py

@@ -0,0 +1,29 @@
+class Singtools(type):


I can't work out what this collection of utilities is trying to achieve, or why it is almost duplicated between the Android and Winforms backends.

freakboy3742 · 2024-12-17T00:10:37Z

changes/2910.misc.rst

+Android implementation of OpenFileDialog
+
+Adding a utility module for reading from external storage files
+
+Adding singleton for file management on Android/Windows for API communication
+
+Support with Python 3.10+


The change note isn't a commit message. It's a description of the new feature as it would appear in the release notes.

To that end, this shouldn't be a misc note - if it were to be merged, it would be a major new feature.

Изменение чтобы сосстествало общему API

SnayperTihCreator added 7 commits October 15, 2024 21:17

A utility module has been created to work with android external stora…

024dbb6

…ge files

The file opening dialog is partially implemented

23684ae

Create 2910.misc.rst

9b7af36

Update utilfile.py

c4e245f

Update utilfile.py

e46c5df

Добавлены коментарии

0d66bbf

Update utilfile.py

f908fef

freakboy3742 requested changes Oct 15, 2024

View reviewed changes

mhsmith changed the title ~~Android update~~ A utility module has been created to work with android external storage files Nov 10, 2024

SnayperTihCreator and others added 7 commits December 15, 2024 21:14

Generalization implementation

f122867

Generalization implementation of file interaction

Updating file system usage

132f093

Update utilfile.py

840bbbb

Update utilfile.py

31bd71d

Update 2910.misc.rst

ba66a0c

Update utilfile.py

4bc9ed0

Merge branch 'beeware:main' into AndroidUpdate

a89e3a9

freakboy3742 requested changes Dec 17, 2024

View reviewed changes

SnayperTihCreator and others added 4 commits December 19, 2024 22:50

update toga_android

cb321c1

Изменение чтобы сосстествало общему API

Merge remote-tracking branch 'origin/AndroidUpdate' into AndroidUpdate

3618da8

Update utilfile.py

653fdfc

Merge branch 'beeware:main' into AndroidUpdate

e6d39e0



		class OpenFileDialog(BaseDialog):
		class HandlerOpenDialog(uf.HandlerFileDialog):

		Objects = java.jclass("java.util.Objects")


		class HandlerFileDialog(abc.ABC):

	Partial OpenFileDialog implementation
	Android now has support for OpenFileDialog.



		class OpenFileDialog(BaseDialog):
		class HandlerOpenDialog(utilfile.HandlerFileDialog):



		class PathReader(BasePathReader):
		"""A phalloid object for reading.

Uh oh!

Comments

Conversation

SnayperTihCreator commented Oct 15, 2024

Uh oh!

freakboy3742 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

freakboy3742 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants