search

mikez / spotify-folders

Get your Spotify folder hierarchy with playlists into JSON.
MIT License
58 stars 6 forks source link

Added Windows support #8

Closed Nitemice closed 1 year ago

Nitemice commented 1 year ago

Added default cache path Also had to replace call to grep with regex-based search in python, because Windows doesn't have grep.

mikez commented 1 year ago

@Nitemice Thank you for taking the intiative! I'm sure that many Windows users can benefit from this.

I would like to clarify two things:

  1. How common is the chosen Windows cache path? Will it work for 90% of the cases? See also discussion here which mentions two other paths.
  2. How much of a speed-hit does the regex-based search cause? And, will the "cp437" encoding also work on non-Windows computers?

If there's a speed hit, I'd suggest to leave get_all_persistent_cache_files as-is and instead put your code into a Windows-version of the function, which gets run for Windows users. Another thing you can try, is to use the command as outlined here and see if that speeds up things.

Nitemice commented 1 year ago

Hi @mikez

Thanks for building such a useful tool!

1. How common is the chosen Windows cache path? Will it work for 90% of the cases? See also discussion [here](https://github.com/mikez/spotify-folders/issues/6) which mentions two other paths.

To be honest, I wasn't totally sure if C:\\Cache\\Spotify was the correct path, but now that I've had a look at that discussion, I'm sure it's not.

My Spotify install is weird: it was originally installed stand-alone, then the Windows Store version was installed over the top. Plus, I have a custom cache location set, so that it doesn't take up room on my SSD.

I think, given there are a few different ways to install Spotify, there will likely be a few "common" cache paths. That said, I checked with a family member and C:\Users\USER\AppData\Local\Spotify\Storage seems to be the path used by Spotify when installed directly (not from the Windows Store), so it's probably the most common and layman-friendly choice. I will update the code to use that path by default.

2. How much of a speed-hit does the regex-based search cause? And, will the "cp437" encoding also work on non-Windows computers?

If there's a speed hit, I'd suggest to leave get_all_persistent_cache_files as-is and instead put your code into a Windows-version of the function, which gets run for Windows users. Another thing you can try, is to use the command as outlined here and see if that speeds up things.

I don't think there's any speed impact. It seems to return output almost instantaneously.

I had another look at the encoding. I think it should actually be 'latin-1', and that should work on non-Windows platforms too. Basically, it just seems to need to be something that extends ASCII, and includes valid characters at a few specific code points that are used (probably as sentinels).

Personally, I like the idea of not depending on external tools, but if you feel it is slower, I'm willing to make your suggested changes.

I'll make those fixes and push it ASAP. If you can test it on a non-Windows machine, and tell me if it still runs satisfactorily fast, we can decide from there if I need to separate the Windows way from the others.

mikez commented 1 year ago
  1. 👍
  2. I tested it on my machine and it's fast. Let's use your version.

Also, added some minor comments to the code.

mikez commented 1 year ago

@Nitemice What do you think?

Nitemice commented 1 year ago

@Nitemice What do you think?

I've pushed an update with some of the initial changes, but I'm still working on a solution for the PermissionError stuff. The next commit I push will include basically all your suggestions.

mikez commented 1 year ago

@Nitemice Except for the WINDOWS_PERSISTENT_CACHE_PATH fix, this looks wonderful to me, thank you. :)