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

Rework implementation of app paths to factor out common implementations. #1964

Merged
merged 17 commits into from Jun 8, 2023

Conversation

freakboy3742
Copy link
Member

@freakboy3742 freakboy3742 commented Jun 6, 2023

This is effectively an audit of the app paths, except that it's not a widget.

Reworks the implementation of paths to use interface/impl separation, allowing for common logic to be shared between backends.

The new implementation is slightly backwards incompatible - previously, the code used the location of the __main__ module; the new code uses the module that contains the App class. This should only affect users who are using a literal toga.App, which is hopefully not many. For all other cases, it produces a more reliable answer, as it doesn't depend on sys.modules magic. This logic is particularly problematic when in tests, as __main__ can vary wildly.

Removes the platform-specific paths tests, as their main use case no longer exists; we can include testbed tests for the specific paths when the time comes.

Adds documentation for the paths class.

Refactors the probes in test backends to allow for an app probe, and adds backend tests that validate all the app paths are readable and writeable.

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

@freakboy3742 freakboy3742 requested a review from mhsmith June 6, 2023 07:02
docs/reference/api/resources/paths.rst Outdated Show resolved Hide resolved
@@ -0,0 +1,55 @@
App Paths
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
App Paths
Paths

There's no more reason for making the sidebar say "App paths" than there is to say "App icons" or "App commands".

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure I agree.

Icons and Commands are always "for" the app in the sense that the app uses them; however, you can have icons that aren't the icon of the app - most icons aren't the App's icon - so saying "App Icons" would be potentially confusing. In the case of commands, commands are always App based, because they can't be for anything else, so "App Commands" would be redundant.

However, you can have paths that are used by the app, but aren't "app paths" - any user-specified directory, for example, and users will need to use pathlib.Path in their code. The Paths object is a subset of very specific paths directly related to the operation of the app, not a generic collection of paths. Mentally, I guess I see this as app.paths, since that's the only place where this construct is used; but app.paths doesn't make for a good title.

docs/reference/api/resources/paths.rst Outdated Show resolved Hide resolved
@mhsmith
Copy link
Member

mhsmith commented Jun 7, 2023

The following paths have changed:

  • Android data and logs
  • iOS data and logs (I assume that cache still resolves to the same location)

This could cause upgraded apps to lose access to data which was written by previous versions. Since the paths API was never documented before, this isn't necessarily critical, although we have recommended many people to use it in the past. But what's the rationale for the change? I thought at first that it might be to avoid overlap between data and the newly-introduced config, but they do still overlap on Cocoa and Winforms.

@freakboy3742
Copy link
Member Author

But what's the rationale for the change? I thought at first that it might be to avoid overlap between data and the newly-introduced config, but they do still overlap on Cocoa and Winforms.

The rationale for the iOS changes was that the paths were wrong :-) It was a direct copy of the macOS implementation; if you tried to use them, they would have failed outright.

As for Android - you're correct in your analysis that I was trying to avoid collisions, but I've been inconsistent in implementing that collision avoidance. The comment in the docs about paths not being empty or unique is the other end of this inconsistency.

I've reverted the Android change because of the backwards inconsistency; however, I'd be interested in your take on whether the backwards inconsistency is worth it in this case.

I've also done a quick audit to see whether the paths we're using align with what platformdirs return - they mostly do, with the some exceptions:

  1. GTK uses .local/state for logs; this makes more sense to me, and the location of logs aren't really significant for backwards compatibility, so I've made this change.
  2. Cocoa uses the app name, rather than the app ID. Using the app name was definitely the historical behavior of macOS; however since the introduction of the App Store, they've encouraged the use of bundle IDs over names.
  3. Platformdirs on Android uses a Path(getFilesDir()).parent / "shared_prefs" for config files, which... I have no idea where that comes from. Android seems to prefer you use their API for storing key/values for preferences, which is something I'd like to support eventually (See Settings API for Toga #90), but unless shared_prefs is an actual Android convention (and I can't find any evidence it is), then I'd rather not start inventing conventions.
  4. For other config paths, the location provided by platformdirs gave some better suggestions, which I've incorporated. This is a new API, so there's no backwards compatibility issues with this change.
  5. platformdirs seems to like adding the app name as a disambiguation at the end of paths, even when the base path is already unambiguous (e.g. it returns /data/data/org.beeware.testbed/files/TestBed as the path for user data). I can't make any sense of this, so I've ignored it.

@freakboy3742
Copy link
Member Author

Following in-person discussion with @mhsmith, we've decided to live with the backwards incompatibility and guarantee that the app paths locations are all unique.

@mhsmith
Copy link
Member

mhsmith commented Jun 8, 2023

Platformdirs on Android uses a Path(getFilesDir()).parent / "shared_prefs" for config files, which... I have no idea where that comes from.

It comes from the SharedPreferences API, which does indeed use that directory. But I don't think its location is documented, or exposed in any API, and I've never seen Google recommend its use for anything other than SharedPreferences XML files. So using a subdirectory of getFilesDir() is definitely a better choice.

@mhsmith mhsmith merged commit 645140a into beeware:main Jun 8, 2023
43 checks passed
@freakboy3742 freakboy3742 deleted the app-paths branch June 8, 2023 22:32
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.

None yet

2 participants