Skip to content
This repository has been archived by the owner on Jun 7, 2022. It is now read-only.

Fix a couple of issues #2

Closed
wants to merge 1 commit into from
Closed

Conversation

mnd999
Copy link

@mnd999 mnd999 commented Jul 16, 2017

Okay, I'm not a rust developer either, but I think I've fixed a couple of crashes I've seen whilst trying to get this to run on FreeBSD. I've not yet managed to make it work successfully, but this feels like an improvement so I thought I'd share:

  • The connect loop seems to poll the future every time, even if it's already complete. This was causing a crash for me.

  • The get credentials code was calling unwrap() without checking for an error. I think I've fixed that too (although I always get an error, so I might have broken the success case).

@mherger
Copy link
Contributor

mherger commented Jul 16, 2017

Thanks a lot! I'll look into this as soon as I'm back from the mountains 😀

@NikolajHansen
Copy link

Hi, am a developer myself. If I can do anything to help please let me know.

@mherger
Copy link
Contributor

mherger commented Aug 8, 2017

Uh, oh... plietar just applied a bunch of changes to librespot which break spotty. I'll have to do some more work, soon.

@michaelherger
Copy link
Owner

I've started a new dev branch using librespot's latest code. I had to manually port your changes, as the base code has changed too much. Feel free to check it out: e245006

@@ -264,8 +266,10 @@ impl Future for Main {
progress = true;
}
}

if let Async::Ready(session) = self.connect.poll().unwrap() {
Copy link
Owner

Choose a reason for hiding this comment

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

This part of this pull request actually breaks the dev branch. Did you check it out? It implements the latest of plietar's changes.

@u363280
Copy link

u363280 commented Jan 10, 2018

This PR should still be merged, it resolves #6.

@michaelherger
Copy link
Owner

I'm reluctant to merge as my comment is still true (patch breaks newer code), and #6 while ugly doesn't break functionality. Credentials would be verified/saved despite the error message.

@u363280
Copy link

u363280 commented Jan 10, 2018

I understand.

If I isolate the code that fixes the error and submit a seperate PR, would you merge?

@michaelherger
Copy link
Owner

The librespot code has seen so many changes, this pull request is rather useless. Please submit a new pull request, should you see need. Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants