moinejf / abc2svg

project moved to https://chiselapp.com/user/moinejf/repository/abc2svg
GNU Lesser General Public License v3.0
48 stars 7 forks source link

play: onend() does not fire if play is aborted via stop() #87

Closed bwl21 closed 6 years ago

bwl21 commented 6 years ago

I have implemented stop such that the play button becomes active after the music really stopped. When user hits the stop button the events already being queued still fire.

But now after all I do not get the onend() fired after the last event being queued was fired. onend() fires if the music comes to its end (without being stopped manually).

It was working with 1.15.8

moinejf commented 6 years ago

I did not change anything about onend() since 1.15.8. There should be a bug of yours.

bwl21 commented 6 years ago

The problem was introduced in commit 1b6564aa791a5a13ade2c39d64927eea444851c0

As you fire endplay() immediately when user hits the stop button, you cannot notice that onend() does not fire if play is aborted. If you you can try this with edit-1.xhtml if you comment out line 556 in edit.js

    if (playing) {
        abcplay.stop();
        //endplay()   <- let onend() switch the button text
        return
    }

As the player does not stop immediately (it plays the already queued events), I am running the following approach:

I think the root cause is that you clear timeout in line toaudio.js line 580. therefore play_next is not invoked anymore after calling stop(), and therefore has no chance to fire onend(),

Solution could be in line 462 to assign the result of timout = setTimeout(play_next, (t + stime - ac.currentTime)

not to timout

bwl21 commented 6 years ago

sorry, ba892a37fbf057287642a90aafa111283508fb10 does not fully solve the problem. I tried in editor-1.js. When I click on "stop", the button changes to "play" immediately, even if the player still fires the note_on events.

I guess we should not cancel all the timeouts such that at least one more call to play_next happens. play_next emits onend() if there is nothing more to play.

I implemented my proposal and it works. In contrast to 1.15.8, the sound now stops immediately, but the "follow" events still happen, so one can see the walking highlights in the notes. After these events are all done, then "Stop" becomes "Play".

moinejf commented 6 years ago

You are right. The fix was too easy... Thanks.

bwl21 commented 6 years ago

Now it works. Thanks. I think it is an improvement, that stop() imeditately causes silence, even if there are some "follow" events to fire.