-
Notifications
You must be signed in to change notification settings - Fork 252
Swap platform_dirs for directories for config paths #702
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
Pogodaanton
left a comment
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.
Directory paths don't map 1:1 from platform_dirs to directories on Windows. E.g. former puts configs in the application's roaming root directory, while latter puts configs in a sub-directory. Similar story to cache directories.
Given this, users get logged out on Windows, past settings are forgotten and past cached data require a manual clean by the user.
|
Thanks for the catch, I'm honestly not sure what the standard cache directories are for Windows, but having a Windows specific piece of logic here feels a little strange. Do you think we should rethink the organization of the config in cache locations with this and have some simple migration script that runs if the application detects otherwise? |
|
I haven't found a standard for cache/config directories for Windows, except for the general advice to use AppData/Local or AppData/Roaming for user-specific miscellaneous application data (Local is fine for our use-case). Overall, I would say that if we opt for using A simple migration script should do the job, it can be deleted in a year or so I'd say. |
Pogodaanton
left a comment
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 tested the migration process on Windows. It worked, except for the missing country_code migration.
| "user-info", | ||
| ], |
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.
| "user-info", | |
| ], | |
| "user-info", | |
| "country_code", | |
| ], |
Country code is cached by psst-core... Honestly it feels weird that it's a tiny 2B file cached like this, but that's a story for another day.
| ("album", "albums"), | ||
| ("artist", "artists"), | ||
| ("key", "keys"), | ||
| ]; |
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.
Renaming is a good idea
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.
Maybe the file could be renamed, just in case. What if there is going to be a migration of another kind in the future, how are we going to differentiate it to this migration? IMO we can afford to call this file something verbose like migration_paths_to_use_directories.rs since we aim to remove this some time in the future eventually.
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.
Yeah. Hmm, I renamed legacy which might be enough? Everything in that module should eventually go away.
Thanks for the test! I need one more test on Linux to validate that it's all good. |
|
I'm on linux, what do I need to test? |
In theory, nothing should have changed on Linux/macOS. That's what needs to be verified. A quick smoke test is to verify whether you're still logged in when using this branch and whether your settings are the same as in previous versions. In settings under the section "cache", the size value should approximately (!) match the size in previous versions. |
Old crate we can get rid of nicely.