Voice messages: Composer controls #5935
Conversation
This fixes a bug where we couldn't upload voice messages because the audio buffer was being read, therefore changing the position of the cursor. When this happened, the upload function would claim that the buffer was empty and could not be read.
This all started with a bug where the clock wouldn't update appropriately, and ended with a whole refactoring to support later playback in the timeline. Playback and recording instances are now independent, and this applies to the <Playback* /> components as well. Instead of those playback components taking a recording, they take a playback instance which has all the information the components need. The clock was incredibly difficult to do because of the audio context's time tracking and the source's inability to say where it is at in the buffer/in time. This means we have to track when we started playing the clip so we can capture the audio context's current time, which may be a few seconds by the first time the user hits play. We also track stops so we know when to reset that flag. Waveform calculations have also been moved into the base component, deduplicating the math a bit.
We say the limit is 2 minutes, not 1m59s, so let's give the user that last frame.
| const minutes = Math.floor(this.props.seconds / 60).toFixed(0).padStart(2, '0'); | ||
| const seconds = Math.round(this.props.seconds % 60).toFixed(0).padStart(2, '0'); // hide millis | ||
| const seconds = Math.floor(this.props.seconds % 60).toFixed(0).padStart(2, '0'); // hide millis |
There was a problem hiding this comment.
@mvgorcum feels mathematically offended and says
2.999 seconds shouldn't round to 2 seconds, it should round to 3
More constructively: round first, calculate minutes and seconds later
GitHub doesn't let me suggest over two lines where one was modified, but I think it's supposed to be something like this?
const roundedSeconds = Math.round(this.props.seconds);
const minutes = Math.floor(roundedSeconds / 60).toFixed(0).padStart(2, '0');
const seconds = (roundedSeconds % 60).toFixed(0).padStart(2, '0'); // hide millis
There was a problem hiding this comment.
I'm not sure the seconds should be rounded, this is as far as I understand used during a recording. All values above 0.5 of a second would be ceiled, meaning that at 0.5 seconds of recording the clock would jump to displaying "1 second" which will skew the time perception of the user and be off by 50% at that point
| position: relative; | ||
| width: 32px; | ||
| height: 32px; | ||
| border-radius: 32px; |
There was a problem hiding this comment.
To avoid this value going out of sync you can use 50% here and it will always form a circle when the width and height are matching
There was a problem hiding this comment.
There's a lot of things we're supposed to do regarding border radiuses, but this is the consistent approach for us. I'd like to see us fix it everywhere if we're going to fix it
There was a problem hiding this comment.
more accurately, I'd like to see us decide and codify the approach for this. I'm happy to fix this up (and the translate3d below) in a later PR if we deem this Not The Way™
| top: 8px; // center | ||
| left: 12px; // center |
There was a problem hiding this comment.
This value can quite easily go out of sync and end up misaligning the buttons. I usually use the following to center items dynamically
| top: 8px; // center | |
| left: 12px; // center | |
| top: 50%; | |
| left: 50%; | |
| transform: translate3d(-50%, -50%, 0); |
| const minutes = Math.floor(this.props.seconds / 60).toFixed(0).padStart(2, '0'); | ||
| const seconds = Math.round(this.props.seconds % 60).toFixed(0).padStart(2, '0'); // hide millis | ||
| const seconds = Math.floor(this.props.seconds % 60).toFixed(0).padStart(2, '0'); // hide millis |
There was a problem hiding this comment.
I'm not sure the seconds should be rounded, this is as far as I understand used during a recording. All values above 0.5 of a second would be ceiled, meaning that at 0.5 seconds of recording the clock would jump to displaying "1 second" which will skew the time perception of the user and be off by 50% at that point
| 'mx_Waveform_bar': true, | ||
| 'mx_Waveform_bar_100pct': isCompleteBar, | ||
| }); | ||
| return <span key={i} style={{height: (h * 100) + '%'}} className={classes} />; |
There was a problem hiding this comment.
I found that this kind of rapid changes on the height property can actually cost quite a bit as it will trigger layout/paint/compositing.
However you can skip the layout and paint operaiton if all of your span have a height: 100% and you play with transform: scaleY(percentage), Blink and Gecko are optimising those operations
There was a problem hiding this comment.
Will look into it for a future change
For element-hq/element-web#1358
Reviewer: This is probably reviewable commit-by-commit, probably.
Video of me forever immortalizing my testing on the internet at nearly midnight:
2021-04-27.23-42-14.mp4