-
-
Notifications
You must be signed in to change notification settings - Fork 669
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
Conversation
@@ -0,0 +1,55 @@ | |||
App Paths |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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".
There was a problem hiding this comment.
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.
The following paths have changed:
This could cause upgraded apps to lose access to data which was written by previous versions. Since the |
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:
|
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. |
It comes from the |
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 theApp
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: