Preview selected frame #90
Conversation
|
Alright, bug is fixed. Ready for review. :) |
| }); | ||
|
|
||
| // Preview a captured frame | ||
| document.querySelector("#reel-captured-imgs").addEventListener("click", function(e) { |
There was a problem hiding this comment.
Why is a querySelector used here rather than the frameReelRow variable?
There was a problem hiding this comment.
Because I did not know that variable existed and I didn't Ctrl+f the selector. Will fix.
|
@le717 this looks good! 😄 I've added a couple of notes related to code style. In terms of functionality you mention I've found a similar issue when looping isn't enabled after playing from a frame and playback stopping at the last frame. If "play" is pressed again at this point playback starts from the first frame in the frame reel rather than the previously selected frame. Another improvement I'd suggest is that when "stop" is pressed the white frame outline should move to the last frame in the frame reel. Also on a side note when playback is in progress there should be some sort of visual cue in the timeline, such as a vertical line that moves along with each frame displayed. Although I appreciate this point is beyond the scope of this particular pull request so don't worry about it for now! |
| @@ -54,7 +54,7 @@ var width = 640, | |||
| // Status bar | |||
| statusBarCurMode = document.querySelector("#currentMode span"), | |||
| statusBarCurFrame = document.querySelector("#currentFrame span"), | |||
| statusBarFrameNum = document.querySelector("#noOfFrames"), | |||
| statusBarFrameNum = document.querySelector("#num-of-frames span"), | |||
There was a problem hiding this comment.
#num-of-frames span is an id that is in lower case with hyphens and doesn't match the style of the other ids used in the status bar. Would you recommend the the other ids in the status bar are changed to match this format?
There was a problem hiding this comment.
Yes, but it does not have happen now unless you want me to do it.
There was a problem hiding this comment.
Yeah no need to do this for now
I tried this out. It was... technically interesting. However, I not sure it would be appropriate. First off, the outline does not move to each active frame during playback (your suggestion), so having it suddenly appear at stop would be odd. It also introduced a number of new cases where it'd need to be removed. I'll keep playing with it.
This was by design. There is no "previously played" state (and I don't think it'd be as simple as setting a true/false either). The frame preview is just that- a preview. It is only meant to temporarily alter the playback. If it were persistent, how would you reset it back to frame 1? A button in the UI? Maybe but could be confusing. Scroll back to frame 1, click it, then scroll back to the end and click the live view button? That is not acceptable. That's why I made it restart from the beginning. That being said, I can still look into it if you would like. |
|
I see what your saying. There isn't an issue with what you've implemented at all, my previous suggestion would in fact make things far more confusing. What I'm really trying to work out is how to handle repeated playback not from the beginning. I think I've figured out how it could work based on the notation program Sibelius. I'll explain this in more detail tomorrow. |
simplify confused live view button styling
|
Loop region playback would be something to explore but it is a tricky thing to handle (and should not be a current focus). BTW, this PR is done. The sooner you merge it, the sooner I can remove all those pesky delete buttons. ;) |
|
I've rechecked out your branch and I'm happy with how it works now and will therefore merge it. I'd like to take back what I said a couple of days about the playback reseting being "uninitiative". After thinking about it today I'm not sure why I saw a problem with it! |
Preview selected frame (thanks @le717)
|
Hey, no need to take it back. it wasn't offensive or anything. It's called discussion and is a part of review. You brought up a path I had considered but did not go with. Since it's my PR, it was my burden of proof to explain how the implemented way was, even for the time being, better. It's all good, don't worry about it. :) |
Fixes #47 (including playback) and the remaining items in #48. Allows #83 to be implemented.
Notes