Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(PPDSC-2047): Resolve gaps in buffering seekbar by refactoring formatTrackData #206

Merged
merged 8 commits into from
May 25, 2022
Original file line number Diff line number Diff line change
Expand Up @@ -593,14 +593,12 @@ describe('Audio Player Composable', () => {
expect(mockGetTrackBackground).toHaveBeenCalledWith({
colors: [
`${seekBarIndicator.base!.backgroundColor}`, // indicator
`${seekBarBuffering.base!.backgroundColor}`, // buffered
`${seekBarTrack.base!.backgroundColor}`, // track background
`${seekBarBuffering.base!.backgroundColor}`, // buffered
`${seekBarBuffering.base!.backgroundColor}`, // buffered// track background
`${seekBarTrack.base!.backgroundColor}`, // track background
],
max: 6610,
min: 0,
values: [10, 14, 20, 20],
values: [10, 14],
});

// Audio player snapshot last (so that buffering is included)
Expand Down
20 changes: 17 additions & 3 deletions src/audio-player-composable/__tests__/utils.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -57,18 +57,32 @@ describe('formatTrackData', () => {
});
});

test('when multiple buffered sections exist', () => {
test('when multiple buffered sections take the nearest to the curser time (start time is closest)', () => {
// This can occur when a user moves ahead then back again, multiple disparate buffered sections can exist.
// Example here is buffered sections exist between 0-125 and 216-228. The current play position is at 32.
// It has been diecided to only show the first buffer section similar to how youtube does it.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lil typo diecided

Copy link
Contributor Author

@tbradbury tbradbury May 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤯 I'll sort 👍

expect(
formatTrackData('track', 'indicator', 'buffer', [32], {
length: 2,
start: (i: number) => [0, 216][i],
end: (i: number) => [125, 228][i],
}),
).toEqual({
colors: ['indicator', 'buffer', 'track', 'buffer', 'track'],
values: [32, 125, 216, 228],
colors: ['indicator', 'buffer', 'track'],
values: [32, 125],
});
});

test('when multiple buffered sections take the nearest to the curser time (end time is closest', () => {
expect(
formatTrackData('track', 'indicator', 'buffer', [122], {
length: 2,
start: (i: number) => [0, 216][i],
end: (i: number) => [125, 228][i],
}),
).toEqual({
colors: ['indicator', 'buffer', 'track'],
values: [122, 125],
});
});

Expand Down
37 changes: 29 additions & 8 deletions src/audio-player-composable/components/seek-bar/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,26 +11,47 @@ export const formatTrackData = (

const time = timeArr[0];
const bufferPositions = [];
tbradbury marked this conversation as resolved.
Show resolved Hide resolved
const closestBuffer = [];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this should just be a single value not an array?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will take a look

Copy link
Contributor Author

@tbradbury tbradbury May 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm adding a single value to an array because it enable me to use the spread operator to add the correct value to the colors array (line 48) and the values array (line 54). If the array is empty I don't need to add a ternary operator to colors or values to ensure they have the right content.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Up to you. I'd prefer using a ternary operator later on but I don't feel strongly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I've changed it


const {length} = buffered;
for (let i = 0; i < length; i += 1) {
const startPosition = buffered.start(i);
if (startPosition > time) {
bufferPositions.push(startPosition);
// get all buffer segments
bufferPositions.push({type: 'start', value: buffered.start(i)});
bufferPositions.push({type: 'end', value: buffered.end(i)});
}

if (bufferPositions.length) {
// get closest buffer segment to curser
const closest = bufferPositions.reduce((prev, curr) =>
Math.abs(curr.value - time) < Math.abs(prev.value - time) ? curr : prev,
);
// get index of closest buffer in array
const pos = bufferPositions.map(a => a.value).indexOf(closest.value);

let value;
if (closest.type === 'start') {
// if closest buffer is type start set the next end as value
value = bufferPositions[pos + 1].value;
}

const endPosition = buffered.end(i);
if (endPosition > time) {
bufferPositions.push(endPosition);
if (closest.type === 'end' && closest.value > time) {
// if closest buffer is type end and greater then current time set as value
value = bufferPositions[pos].value;
}

if (value) {
// set value for buffer
closestBuffer.push(value);
}
}

const colors = [
indicatorColor,
...bufferPositions.map((x, i) => (i % 2 ? trackColor : bufferColor)),
...closestBuffer.map(() => bufferColor),
trackColor,
];
const values = [time, ...bufferPositions];

const values = [time, ...closestBuffer];
return {colors, values};
};

Expand Down