ENH: Added "force end Routine" option for Sound Component#6766
ENH: Added "force end Routine" option for Sound Component#6766TEParsons merged 9 commits intopsychopy:devfrom
Conversation
|
@mh105 would appreciate if you could give this a go and sanity check, having recently worked on the Sound Component :) |
mh105
left a comment
There was a problem hiding this comment.
Looks great! I'm glad this Force End Routine option is added for Sound Component.
I have two minor suggested changes. To make it easier to see what I meant and to accept the changes, I've made a PR to your TEParsons:dev-enh-sound-force-end branch. If you think they are acceptable, you can just merge and then merge this current PR.
| hint=msg, | ||
| label=_translate("Sync start with screen")) | ||
| # --- Playback params --- | ||
| self.order += [ |
There was a problem hiding this comment.
self.order isn't created concisely with all the correct parameters. We should fix it. See my PR to your branch.
There was a problem hiding this comment.
Generally I prefer the approach of doing += for each section, as it just makes it less of a monolith to maintain. e.g. if I add one param to Playback, I only have to think of it in relation to the params on the same page as it, rather than a block of all params at the top. This is just stylistic preference though, works the same either way :)
PR REVIEW: changes to the PR to add force-end to sound component
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #6766 +/- ##
==========================================
+ Coverage 49.60% 49.62% +0.02%
==========================================
Files 332 332
Lines 61078 61191 +113
==========================================
+ Hits 30297 30369 +72
- Misses 30781 30822 +41
|
Required a rewrite of the "stop" logic on the JS end to imitate isPlaying/isFinished until they're implemented in actual PsychoJS, as we need to know when the sound itself has finished if we're to end the Routine on the basis of it, but I've tested a few use cases out and it seems consistent with before.