-
-
Notifications
You must be signed in to change notification settings - Fork 781
Local app config for Linux #3482
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
base: main
Are you sure you want to change the base?
Conversation
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.
This needs a changenote, and there's one suggestion inline about using a context manager; but otherwise this looks great!
b4cbb02
to
45e078d
Compare
Does this need any documentation beyond the changenote? |
I'm sure this is on Russell's radar...but as a doublecheck: while I agree this is a bugfix, I would also consider it a candidate for the |
I'd say it's worth adding a platform note to the app paths docs. That page doesn't have a notes section at present, but a lot of other widgets have a Notes section to capture "platform quirks" and things like that (see MapView for one example). As @rmartin16 notes - it's probably worth a |
I've been thinking for the past week that today might be the day I could get back to this and finish it up, but some work things intervened. It's still very much my intention to finish this ASAP. I haven't taken the coin out of the sleeve, or looked at it since I got back. Not until I properly earn it. 😆 |
Since Toga handles creating the root directories if they don't exist, we don't want to create the parent folders here, so that it fails if the directory wasn't created by Toga.
This is in preparation for a future change.
No backends implement it yet.
This commit also updates related documentation.
45e078d
to
31e5ad5
Compare
I've updated this and it's almost ready to merge. One of the failing checks is a spelling issue I've already fixed locally (I added "config" to the word list). The other three failing checks are because the GTK tests are testing different app paths, which is conflicting with the caching of app paths introduced in #3544 / #3669 (cc: @HalfWhitt ). It seems that we can either remove the caching, which is a pretty minor optimization, or add some ability to clear the cache. Thoughts? |
The I'm not currently at a computer to verify that works; it's possible I'm overlooking some detail of how exactly to properly access the function that's wrapped by the property... |
Refactored tests to respect XDG app path customization, and added the functionality for the GTK backend while marking it as not implemented on the other backends.
This fixes the problem of Linux users not having their app files stored in the place they've configured.
Fixes (partially) #2867 (since that mentions Windows as well).
PR Checklist: