Added LastFM support

[marcfusch]

Hello, (this is a PR in response to #424)
I have made the required modifications to the codebase in order to plug psst into the rustfm-scrobbler-proxy library.
I have created a new lastfm.rs file that contains the necessary functions that communicates with the api library.
The necessary functions are called from within the controller/playback.rs file.
I have also added a new tab into the preferences menu that requires api key, api secret, username and password from your lastfm account.
The two firsts can be obtained here: https://www.last.fm/api/account/create
Please guide me through (if necessary) to make the final modifications.
Thanks!

[jacksongoode]

This is great, I'll test now!

[marcfusch]

Mhhhhhh, I cannot get your code to compile with cargo to try your edits.

[jacksongoode]

Hmm, let me double check that. I made some edits to consolidate the LastFM details into the "Account" tab to simplify.

[marcfusch]

Hmm, let me double check that. I made some edits to consolidate the LastFM details into the "Account" tab to simplify.

You also changed the exchange_code_for_token (spotify only) to return errors but it is not used through the rest of the program. I am just returning the token now instead of using Ok() and map_err but you may want to go back and change your event in the preferences.rs to better propagate the error.

[jacksongoode]

Ah thank you. Hmm even still it would be nice to have a button that confirms the LastFM credentials/keys and makes sure its valid and then just hides the fields and shows the username signed in and a "Deregister" button. I think it's a little weird to display it in plain text.

[marcfusch]

Yes that would be awesome. I also think that making a other tab like menu under the accounts section would be nice instead of having a long scrollable list of credentials to fill all at once. This is pure esthetics but could be nice too. 👍

[jacksongoode]

Okay reviewing the crate, it seems you can authenticate with Last.FM using the Oauth flow (webbrowser and then save a token). This would be better than inputting username/password. We really shouldn't be saving them in the config file.

[marcfusch]

I 100% agree with you. I saw the library that I am using also supports auth via token or a session key. I don't know how you are handling oauth authentication with spotify but it may be worth it to see how it works.

[jacksongoode]

I think you can have a look at the way the Spotify oauth works in the oauth file! Would you like to try and implement it for Last.fm?

[jacksongoode]

@marcfusch I think I got it all working pretty well now. I also did some general cleanup since the Last.fm and Spotify Oauth share methods now.

[jacksongoode]

@SO9010 could I get a quick look over this PR? A lot of the changed files were a result of a general lint format I ran and just figured I'd keep in this PR.

But enough was rearranged that it would be good to have a third pair of eyes.

[marcfusch]

Wow, incredible work! Everything works on my side.

[SO9010]

Ok I will check it out now :)

[marcfusch]

I am having a new problem with the latest commits, session key isn't exchanged and lastfm isn't being authed on startup :/

[SO9010]

This is very cool! Just a few notes, though (sorry for being picky)... @jacksongoode please do say if you disagree with anything I just want this feature to be done right.

The first thing that I think is needed is that we need to have it so that there should be a toggle like a "private" session so that if people don't want to log out, they can play without it being reported. I remember a few issues of people wanting that as a feature.

I think it would be good if there was an option to right-click on the now-playing album cover to report it to Scrobble.

When logging in, I think it would be good to link to the API creation page as it took me a struggle to find it :P

I also think that it currently writes to the config too much. Each time that I type a new letter into the API key or Secret, it reupdates it. This should only be done when the connection is complete.

Additionally, I'm not sure about the use of scrobble proxy over normal scrobble. If I understand it right, it will add a variant of ureq which detects proxy, which makes sense, but as we already have the ability to add proxies if the user wants, I'm not sure if it makes sense to add another dependency... Also Ureq is outdated for both psst and that dependancy. I don't think it would be the hardest to actually implement the API calls in psst itself without needing an external crate.

In psst-gui/src/webapi.client.rs, there is a section which shows how we deal with the API requests:

pub fn new(
        session: SessionService,
        proxy_url: Option<&str>,
        cache_base: Option<PathBuf>,
        paginated_limit: usize,
    ) -> Self {
        let agent = default_ureq_agent_builder(proxy_url).unwrap().build();
        Self {
            session,
            agent,
            cache: WebApiCache::new(cache_base),
            token_provider: TokenProvider::new(),
            local_track_manager: Mutex::new(LocalTrackManager::new()),
            paginated_limit,
        }
    }

I personally think that it makes more sense to implement it here with psst-gui rather than in the core as the core is meant to be for the music and spotifty component and is meant to be the lightest part so that others can use it. @jacksongoode what do you think or am I being too picky? I certainly think that this makes a lot more sense.

[jacksongoode]

@SO9010 I was able to do the toggle proxy. @marcfusch Could I ask you to handle the linking to the api page in the description under the account title and only saving the config to the config file once we successfully auth?

[jacksongoode]

Ok I've actually done the last two so I'm going to merge if I can get one final ok :)

I think the LastFM crate replacement can be done a little later. It would be nice to have this in!

[SO9010]

Yeah all looks good to me, I was also thinking if we implemented the API by ourselves it would make sense to do that after updating ureq! We could also then implement something like discord presence API as that doesn't look too bad to implement.

[marcfusch]

@SO9010 I was able to do the toggle proxy. @marcfusch Could I ask you to handle the linking to the api page in the description under the account title and only saving the config to the config file once we successfully auth?

Sorry I was sleeping here in France. Is there anything more I can do? I was thinking of supporting libre.fm and listenbrainz to for people that do not use last.fm

[marcfusch]

Ok I've actually done the last two so I'm going to merge if I can get one final ok :)

I think the LastFM crate replacement can be done a little later. It would be nice to have this in!

Did you also check lastFM auth on startup?

[SO9010]

Good point, I didn't check if it works on new startup I will do that later when I have the time :P

[SO9010]

I should be able to do a proper check later in the evening :)

[SO9010]

Sorry I was sleeping here in France. Is there anything more I can do? I was thinking of supporting libre.fm and listenbrainz to for people that do not use last.fm

Personally I think it should be ok for now but it's up to @jacksongoode to what he thinks. I think it would make sense to do this after all the ureq changes which I will try and get done asap, but that will require some rewrites of things to no introduce unessisary packages :P

[jacksongoode]

Let's do it after!