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

Audio Player Improvements #294

Merged
merged 25 commits into from
Jul 11, 2020
Merged

Audio Player Improvements #294

merged 25 commits into from
Jul 11, 2020

Conversation

ShresthPratapSingh
Copy link
Contributor

@ShresthPratapSingh ShresthPratapSingh commented Jun 17, 2020

This PR includes following features:

  1. A queue View Conrtoller for next up tracks in the queue.
  2. Interactive and interruptive animations for the queue VC.
  3. Fixed iOS 13 bug in the Audio Player( playing music crashes the app #289 )

@cpg
Copy link
Member

cpg commented Jun 18, 2020

Great job!

My quick feedback about the UI:

  • the Up next word should also pop the queue view up (and down)
  • the music has no icons, however they are available in the app already, can we use them instead of the generic music notes?
  • the remove action (the stop sign) should be on the right side and the reordering handle (the three lines) on the left
  • the re-sorting queuing does not actually seem to work at all. I put a song next (at the top) and it does not get played next. I do not have the random shuffle on. I looked to see if it was the last one on the queue and it was not.
  • the background of the Up next panel changes, for some reason. It became somewhat red and I was wondering if it was some warning or error. Later it appears to be random. What is happening here?
  • later, I found that the queue as it appears in the Up next panel seems to be working
    but the top song is not what goes next. it never gets moved from the top when played. Not intuitive.

It takes a full ~9 seconds from the moment of tapping on a song to it actually audio playing. The entire app is frozen. I am in LAN mode for sure (I enforced it in the settings), so it's not a Remote access issue. Any ideas why this is?

This appears to fix the crashing in audio in #289 but not the video crashing. We can fix that separately under another issue/PR but I just wanted to note it here.

@cpg
Copy link
Member

cpg commented Jun 18, 2020

Two more things:

  • the name of the songs goes too far into the slider, clashing with them
  • the font is way too big. it should not be
  • selecting the random play takes three touches for it to become white.

It's clear we need to focus and go step by step.

@ShresthPratapSingh
Copy link
Contributor Author

Okay I'll check. Also can you provide me some more information about the "name going too far in the slider" issue? Can you share a screenshot?

@cpg
Copy link
Member

cpg commented Jun 18, 2020

IMG_3330

@cpg
Copy link
Member

cpg commented Jun 28, 2020

Where are we on this?

@ShresthPratapSingh
Copy link
Contributor Author

I'm working on this.

@ShresthPratapSingh
Copy link
Contributor Author

@cpg I've updated this PR and resolved the conflicts.

Changes in this PR :

  1. tapping anywhere on the "UP next" view will open the queue.
  2. Fixed radom play.
  3. Fixed the font being too big and overlapping other views.
  4. Fixed music thumbnails in the queue.
  5. The queue shows the currently playing song thus fixing aparent confusion in shuffling of queue.
  6. fixed delay in presenting the audio player view controller.

ShresthPratapSingh and others added 3 commits July 2, 2020 16:46
reverted spacing changes.
Removing apiconfig from the pull request
@cpg
Copy link
Member

cpg commented Jul 2, 2020

It's not quite there:

  • the song playing should not be at the top
  • The queue does not remove the song at the top when it starts playing. I get it that there is some (bright red!!!! why!?) icon on the song playing. This is unnecessary. Users have lists of hundreds of songs some time. We should not subject them to scroll all the way to wherever the song is. Also, the (red) icon sometimes pulsates and sometimes it does not. One step forward, two steps back.
  • the remove action (the stop sign) should be on the right side and the reordering handle (the three lines) on the left
  • in iOS12, it shows all "light" interface in the up next area in an otherwise dark app. It would be great if it were dark by default at least in ios12 like the rest of the app
  • the player turns "vertical" forcibly in iPad even if the iPad is horizontal. Edit: this issue was in beta prior to these changes. my bad. we can handle it separately.

@ShresthPratapSingh
Copy link
Contributor Author

@cpg we also support repeat all and going to previous song functionality in the player. If we're removing the songs from the queue once they're played, then everytime user clicks previous button we'll have to add the exact previous song to queue again, this is a heavy task as compared to other approaches since we'll have to reload the table view everytime previous is clicked.

Now the issue that you mentiontioned is a genuine one as not removing will make the user to scroll way down to next song.

What we can do is automatically scroll the table view to the next song when the queue is presented. If the user wishes to go to previous we don't even have to update/reload the data, we just need to scroll to the next song. Also, if a user wants to re-listen to a track he can just drag down that song to appropriate order or just tap on it.

@ShresthPratapSingh
Copy link
Contributor Author

@cpg you can test the changes now. The iPad rotation issue will be resolved in Gestures PR.

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.

2 participants