Skip to content
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

Replace local Spotify API with web based API #303

Closed
wants to merge 7 commits into from
Closed

Replace local Spotify API with web based API #303

wants to merge 7 commits into from

Conversation

MeikelLP
Copy link

Due to the latest errors caused by Spotify removing the local API the Spotify part of this application did not work anymore (see #299).
To provide a future-proof solution I implemented the Spotify API for .NET which is also listed in the spotify libaries.

I also shortened the code drastically due multiple code parts are not required anymore.

If you have any questions like "how" / "why" ... just ask.

WARNING

  • To implement this library I was required to remove the applications signing (.snk files and references). Which caused the library to be not included (I don't know why exactly).
  • Using this library requires the user to authenticate via a browser. It also opens everytime you start the app saying "Auth successful". This is currently not fixable (my knowledge).

Meikel Philipp added 4 commits July 21, 2018 11:25
[ADD] dependency to SpotifyAPI-NET
[ADD] requirements for development when contributing
[DEL] Snip.snk and referencing files because it causes SpotifyAPI-NET to not work at all (because strongly typed assemblies).
[CHG] Replaced old code with new SpotifyAPI-NET
[DEL] (now) obsolete code
# Conflicts:
#	Snip/Players/Spotify.cs
[FIX] some merge issues.
@dlrudie
Copy link
Owner

dlrudie commented Jul 22, 2018

I like the idea of this just due to the fact Snip wouldn't require elevated privileges. I'll have to review this thoroughly.

@markspolakovs
Copy link
Contributor

markspolakovs commented Jul 22, 2018

Hmm - doesn't the spotify API library store the OAuth refresh token somewhere on the system? That wouldn't cause the "success" popup as long as the session is valid. If it isn't, it isn't doing its job properly.

EDIT: nevermind I'm a dumbass, it uses implicit grant, there's no way around that unless we run a server

@dlrudie
Copy link
Owner

dlrudie commented Jul 22, 2018

Two things.

One: I really dislike Linq and try my hardest to use it in nothing.
Two: I'd prefer to keep the metadata caching and loading.

Otherwise it seems good so far. I'll dig into it more and more.

As far as the web browser and people logging in... that's fine. I don't like the success aspect of it, but I do have my own web server that I can use for callbacks if that's what is needed.

@MeikelLP
Copy link
Author

@dlrudie

  1. I'm not using any LINQ in my code so far.
  2. I can see that. I'm adding it again.

For the web server topic: SpotifyAPI-NET has a "simple" HTTP server for this purpose self-implemented running under localhost and catches the callback. The library also opens the browser automatically. At this point, I don't know whether you can configure this behavior.

@dlrudie
Copy link
Owner

dlrudie commented Jul 22, 2018

In commit a041c54

Line 21 of Spotify.cs adds linq.
Line 143 uses linq to join strings.

@MeikelLP
Copy link
Author

Oh, you are right.
At this point I won't tell you why should use LINQ, you can google that yourself.
As a professional C# developer I can assure you that LINQ is nothing bad. Even (if you don't use LINQ2SQL or EntityFramework) it brings performance improvements. Ever modern language today uses lambda expressions to improve development speed. Even java did it finally with Java 8.

@dlrudie
Copy link
Owner

dlrudie commented Jul 22, 2018

Oh, I know its benefits. I just don't like the way it looks and integrates with existing code. It just looks funky to me to have a bunch of SQL-like code in the middle of other code. Plus if I started to implement LINQ then I'd want to re-write Snip from the ground up to take advantage of it. But currently Snip isn't exactly a heavy-duty application that needs a lot of performance. :)

@MeikelLP
Copy link
Author

I don't want to implement LINQ projects wide. I'm just used to use LINQ in as utilies (like here selecting online the name from a list of objects). And it's a simple lambda method nothing fancy like from trackInformation.Item.Artists artist select artist.name which would do the exact same thing as trackInformation.Item.Artists.Select(x => x.Name).

Meikel Philipp added 2 commits July 22, 2018 21:45
# Conflicts:
#	Snip/Players/Spotify.cs
[CHG] Removed (via comments) the new memory based spotify implementation.
[DEL] requirement of admin privileges on startup.
@MeikelLP
Copy link
Author

So in the new commit are the following contents:

  • I removed (commented) they new memory based implementation of the Spotify service.
  • I removed the requirements for an elevated application (app.manifest)
  • I did not bring back the JSON caching because at the moment I would check if the current track is new or not I already have enough information in the TrackInformation object to display everything I want (less it not possible with the Spotify API as far as I know). At this point caching brings nothing except more load on the program.

@MeikelLP
Copy link
Author

At this point, I am seriously considering rewriting the app from the ground up. Nothing against you or your work but this app needs some serious polishing. I am willing to go in contact with you @dlrudie and at least talk about planing a rework of the app :)

@dlrudie
Copy link
Owner

dlrudie commented Jul 22, 2018

I'm down for it. I don't mind.

Really the whole reason 99% of my programs exist is that I develop them originally for myself for my own need, and then release them to the public because why not. Then everyone wants features for this, and features for that, and all kinds of things that my program was originally never designed to have, and then things get messy and ugly. ;)

Now that it's gained in usage and popularity it could definitely use a lot of polish.

# Conflicts:
#	Snip/Players/Spotify.cs
#	Snip/ProcessFunctions.cs
@EpikYummeh
Copy link

Any update on merging this PR?

@MeikelLP
Copy link
Author

MeikelLP commented Feb 2, 2019

@dlrudie ?

@dlrudie
Copy link
Owner

dlrudie commented Apr 7, 2019

Sorry. Been super busy with work. (80+ hour work-weeks make me not want to come home and touch the computer). I'm trying to get caught up this weekend though. I'll review this again.

I'm actually surprised my implementation is still working perfectly fine with all the updates they've made to the Spotify client. I do want to get away from my implementation though due to needing elevated privileges.

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