search

daspartho / SpotiByeAds

Skip spotify ads by automatically restarting application when ad plays
GNU General Public License v3.0
284 stars 41 forks source link

PR to address issue #30 #31

Closed atridey closed 3 years ago

AnonymouX47 commented 3 years ago

Did you test it and it works?... I don't develop on windows, so i'm quite unable to test it.

Nevertheless, i'm still quite experienced with windows and it's CLI and as far as i know, it's case-insensitive, this is also majorly due to how Windows presents/abstracts NTFS, which windows systems use.

Also, shutil.which() is primarily for linux OSes in this case since on Windows and Mac, the location of the Spotify executable is not added to the PATH environment variable by default.

While on linux where it correctly applies, the common filesystems and CLI are case sensitive. The executable on linux is completely lower-case.

So, I'm not sure this PR actually fixes anything, except you tested it and there was a difference.

atridey commented 3 years ago

I tried changing it back to see if it works without this change, but now I run into issue #26. I tried cloning from the repo in PR #27, but it doesn't solve the error. I don't know what's happening anymore.

AnonymouX47 commented 3 years ago

@daspartho Like i note earlier, please note that this PR breaks the re-launching of Spotify on Linux.

The command on linux is completely lowercase and commands are case-sensitve,

atridey commented 3 years ago

I'll also add that it doesn't change anything on Windows. It broke itself and fixed itself. I reverted the code to how it was before this PR and it worked fine. It's my fault for causing the confusion as I should have drafted the PR.

AnonymouX47 commented 3 years ago

Ouch! Could be painful.

Actually for windows, the best way would be to use the exact installation path. Using which to find Spotify requires that its location be on the PATH environment variable and this is the case on Windows by default.

And as for killing the task, the case doesn't matter on windows, as I've explained earlier.

atridey commented 3 years ago

Yes, that's the solution to the problem. Since it somehow worked earlier without Spotify being on the PATH variable I disregarded the instruction to add it, but adding it fixed it. I'll add this to the documentation and create a PR to revert this one.

atridey commented 3 years ago

I made PR #37 to set it back to as it was before.

AnonymouX47 commented 3 years ago

As at https://github.com/daspartho/SpotiByeAds/pull/29/commits/0b5668ce05c2c21b61aea7c92b179542be6649cf in my PR #29, it works fine on linux and theoretically should on Windows (i've not tested it because of the lack of windows on my side but i tested the path on wine and it should be correct for any Windows system).

It's not yet merged though.