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.

Sign up for GitHub

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

Jump to bottom

Fix Issues With Skipping & Playback #176

Merged
merged 4 commits into from Apr 7, 2018
Merged

Fix Issues With Skipping & Playback #176

merged 4 commits into from Apr 7, 2018

Conversation

dcvz
Copy link
Contributor

@dcvz dcvz commented Apr 7, 2018 •
edited

Would fix #146

@dcvz dcvz self-assigned this Apr 7, 2018
@dcvz dcvz requested a review from Guichaguri April 7, 2018 17:08
@dcvz
Copy link
Contributor Author

dcvz commented Apr 7, 2018

@Guichaguri including you because I changed the API for getCurrentTrack. I think it doesn't really make sense to reject the promise when nothing is playing. The user wishes to see what it is, I think returning null instead of rejecting is more aligned with what the user would expect.

Guichaguri
Guichaguri approved these changes Apr 7, 2018
View reviewed changes
Copy link
Collaborator

@Guichaguri Guichaguri left a comment

Choose a reason for hiding this comment

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

LGTM

@dcvz dcvz merged commit 8553021 into dev Apr 7, 2018
@dcvz dcvz deleted the chore/bug-fixes branch April 7, 2018 17:54
@osiloke
Copy link

osiloke commented May 11, 2018

Unfortunately, the same issue as #146 still occurs with this commit

Yannickgregoire pushed a commit to vpro/react-native-track-player that referenced this pull request Jul 25, 2018
Merge branch 'dev' of https://github.com/react-native-kit/react-nativ…
9753eae 
…e-track-player into feature/changed-event

* 'dev' of https://github.com/react-native-kit/react-native-track-player:
  Fixes channel issues with Android Oreo. Fixes doublesymmetry#204
  Bump to v0.2.4
  Add .npmignore
  Fix bugs, add tests & other improvements (doublesymmetry#177)
  Ignore example project on npm install
  Add Example Project and First Example (doublesymmetry#173)
  Fix Issues With Skipping & Playback (doublesymmetry#176)

# Conflicts:
#	android/src/main/java/guichaguri/trackplayer/metadata/components/MediaNotification.java
@willcosgrove
Copy link

I'm not a Swift developer, so I could be off base, but looking at this block:

https://github.com/react-native-kit/react-native-track-player/blob/7db804f3a128966b8f74c663e4766cd004c5fbb3/ios/RNTrackPlayer/Logic/MediaWrapper.swift#L172-L176

I see that we check that currentTrack?.id == queue[currentIndex].id. But currentTrack is defined as the same thing:
https://github.com/react-native-kit/react-native-track-player/blob/7db804f3a128966b8f74c663e4766cd004c5fbb3/ios/RNTrackPlayer/Logic/MediaWrapper.swift#L48-L50

So that should always be true, which means it isn't adding any behavior to this if statement.

I don't know enough Swift to be able to comment on what a good solution would be. Obviously, the play function needs to know if the track has been skipped, but there's a lot of different ways it could get that information. At the moment, I'm working around this bug from the JS side, not issuing a command to skip if we're paused. Ideally the player would handle this case properly, but because it is able to be worked around, I'm not desperate enough to learn Swift to submit a fix 😬

But to any future devs looking to fix it, I think this comment basically outlines the issue.

@willcosgrove willcosgrove mentioned this pull request Aug 9, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Guichaguri Guichaguri Guichaguri approved these changes

Assignees

@dcvz dcvz

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

iOS MediaWrapper.swift - play() invoked by playNext()/playPrevious()
4 participants
@dcvz @osiloke @willcosgrove @Guichaguri