Skip to content

Conversation

@jacksongoode
Copy link
Collaborator

Old crate we can get rid of nicely.

Copy link
Contributor

@Pogodaanton Pogodaanton left a 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.

@jacksongoode
Copy link
Collaborator Author

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?

@Pogodaanton
Copy link
Contributor

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).
The directories library uses a deliberately opinionated folder structure for the various file groups (cache, config, etc.). They seem to be inspired by QT's conventions, though they even deviated from that.

Overall, I would say that if we opt for using directories, we should use their default folder structures, otherwise we can pick other less strict path resolving packages. A small benefit to sub-folders is that safe-to-delete folders like cached files will be easier to find.

A simple migration script should do the job, it can be deleted in a year or so I'd say.

Copy link
Contributor

@Pogodaanton Pogodaanton left a 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.

Comment on lines +37 to +38
"user-info",
],
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"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"),
];
Copy link
Contributor

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

Copy link
Contributor

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.

Copy link
Collaborator Author

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.

@jacksongoode
Copy link
Collaborator Author

I tested the migration process on Windows. It worked, except for the missing country_code migration.

Thanks for the test! I need one more test on Linux to validate that it's all good.

@Cleboost
Copy link
Contributor

I'm on linux, what do I need to test?

@Pogodaanton
Copy link
Contributor

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.

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.

4 participants