podlove / podlove-web-player

Podlove Web Player is a Podcast-optimized, HTML5-based audio player based on VueJS.
https://docs.podlove.org/podlove-web-player/
Other
353 stars 68 forks source link

fix: The replay button is broken #881

Closed jnnklhmnn closed 5 years ago

jnnklhmnn commented 5 years ago

fixed bug regarding the playstate in the player component which prevented the replay button to work as expected

vercel[bot] commented 5 years ago

This pull request is automatically deployed with Now. To access deployments, click Details below or on the icon next to each push.

codecov[bot] commented 5 years ago

Codecov Report

Merging #881 into development will increase coverage by <.01%. The diff coverage is 100%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development     #881      +/-   ##
===============================================
+ Coverage        98.74%   98.75%   +<.01%     
===============================================
  Files               79       79              
  Lines             1039     1042       +3     
===============================================
+ Hits              1026     1029       +3     
  Misses              13       13
Impacted Files Coverage Δ
src/store/playstate/actions.js 100% <100%> (ø) :arrow_up:
src/effects/components/episode.js 97.14% <100%> (+0.08%) :arrow_up:
src/effects/player.js 100% <100%> (ø) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update a14c674...0d6bd02. Read the comment docs.

codecov[bot] commented 5 years ago

Codecov Report

Merging #881 into development will increase coverage by <.01%. The diff coverage is 100%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development     #881      +/-   ##
===============================================
+ Coverage        98.74%   98.75%   +<.01%     
===============================================
  Files               79       79              
  Lines             1039     1043       +4     
===============================================
+ Hits              1026     1030       +4     
  Misses              13       13
Impacted Files Coverage Δ
src/effects/components/episode.js 97.18% <100%> (+0.12%) :arrow_up:
src/store/playstate/actions.js 100% <100%> (ø) :arrow_up:
src/effects/player.js 100% <100%> (ø) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update a14c674...113278e. Read the comment docs.

alexander-heimbuch commented 5 years ago

Thanks for the contribution, only an integration test for the control bar seems to be broken :) If that is fixed I can merge it :P

alexander-heimbuch commented 5 years ago

Pretty sure this is just a timing issue. The back button get's disabled when playtime is 0. To show the progress bar play and pause is dispatched. Depending on the Cypress machine state some milliseconds may pass, which will cause the disabled state to be false. So my suggestion is to directly display the progress bar by dispatching toggleProgressBar and remove the cy.play and cy.pause from the test.

jnnklhmnn commented 5 years ago

seems legit, that would explain why the tests succeed on other machines, will fix it with the suggested solution

jnnklhmnn commented 5 years ago

I may be off track here but digging into this I’m not quite sure if the breaking test is testing quite the right thing. So as far as I can see this test is breaking: https://github.com/podlove/podlove-web-player/blob/development/cypress/integration/controllbar.spec.js#L152

It is regarding the backbutton, and is checking for two things:

When the last chapter was reached or even ended, i would expect the back button to be enabled and allow the user to skip through the chapters backwards nevertheless the audio file ended or is playing the last chapter. The disabling makes sense for the forward button in this player state and is working fine.

So I’m left with the question, is the disabling of the back button when the last chapter was reached expected behaviour? Then it needs to be fixed in the code. If not, is this specific test appropriate?

Feedback on this highly appreciated :)

alexander-heimbuch commented 5 years ago

I think the title is simply a copy paste error from the next button test :/ The back button should be disabled on first chapter.