Implement playCount database update

[cfollet]

Closes #160.

[YurySolovyov]

I don't quite get the purpose of the thresholdReached variable and all associated infra.

[cfollet]

If we don't do that it will increment each tick() call when Player.getAudio().currentTime >= Player.getAudio().duration * Player.getThreshold() so basically every 100 ms after the threshold.

Plus, this also prevent the incrementaion if the user scrolls back.

This is my very first pull request. Fell free to criticize me if I do wrong!

[YurySolovyov]

Maybe you can move Player.getAudio().currentTime >= Player.getAudio().duration * Player.getThreshold() into local variable?

[YurySolovyov]

Or even put this entire method out of component to lib/player.js, cause it fits there better IMO.

[martpie]

Thanks for the PR, I'll review it a soon as I have some free time.

I'll try to investigate why AppVeyor fails too.

[martpie]

Ok, the threshold reached check condition should be in a single method. So we have isThresholdReached that check if the action should be called, and move the check in Player.js indeed.

So in Player.js, we have:

// this function name is a bit misleading, any better idea ?
isThresholdReached() {
    if(!this.tresholdReached && this.audio.currentTime >= this.audio.duration * this.threshold) {
        this.thresholdReached = true;
        return true;
    }

    return false;
}

and in PlayingBar.react.js

if(Player.isThresholdReached()) {
    LibraryActions.incrementPlayCount(this.props.queue[this.props.queueCursor]._id);
}

I wrote this code quickly, there may be some mistakes.

There would be some variable names confusions though, you may need to choose more appropriate function or variable name to make it clear.

Once done, It should be ok to merge the PR.

[YurySolovyov]

if(!this.tresholdReached && this.audio.currentTime >= this.audio.duration * this.threshold) {
    this.thresholdReached = true;
    return true;
}

Side effect in method that should return boolean value does not seem like a good thing.

[martpie]

What do you suggest ?

[YurySolovyov]

Well, let's at least separate them and move out of components.

[cfollet]

There would be some variable names confusions though, you may need to choose more appropriate function or variable name to make it clear.

Maybe the attribute this.thresholdReached can bethis.thresholdReachedFlag

[YurySolovyov]

@cfollet I'd say we should mention that this is track duration threshold.

[cfollet]

@YurySolovyov thresholdReached to trackDurationThreshold ??

[YurySolovyov]

Sounds good.

[cfollet]

@YurySolovyov Calling it trackDurationThreshold means this variable is a threshold. In fact it is not.
It's a boolean value that flag when the threshold is reached.
The name trackDurationThreshold would fit for the value this.audio.duration * this.threshold. But we are a little pernickety here :)

[YurySolovyov]

just drop track part, durationTreshholdReached. It is player, so it is mostly about tracks anyway :)

[martpie]

side consistency note:

Player.js is meanly meant for audio manipulation, it is the interface between the audio system (Audio object for now) and the app. So we can easily switch to another system later if needed (WebAudio for example), without having to rewrite components.

This means we'll have to get rid of all Player.getAudio() one day to make things cleaner.

[cfollet]

it is about AudioContext #128 ?

[YurySolovyov]

@cfollet I think it is about moving code of this PR out of component(s) to the audio-related place.

[martpie]

@cfollet Yury is right. It was just to inform you about what the Player class is meant to be :) but this does not change anything about the changes required for this PR ;)

[cfollet]

I updated the PR

[martpie]

minor typos: it's 'Threshold', and not 'Threshhold' ;)

[cfollet]

I think it's ok now

[martpie]

My only concern was what if state.repeat === 'one', but it seems like the threshold is reset anyway when AppActions.player.previous or AppActions.player.next are called. Seems good to me, any last thoughts @YurySolovyov ?

[YurySolovyov]

Why not put it into timeupdate event handler? It would be a bit more efficient and we can extract the code from components entirely.

[cfollet]

@YurySolovyov Sounds great.

[YurySolovyov]

@KeitIG why do we use custom timeout and not timeupdate event in the first place?

[martpie]

@YurySolovyov you mean why there is a this.state.elapsed ?

@cfollet timeupdate looks great if perfs keep being fine.
If so, add your event there: AppActions.js:36.

[YurySolovyov]

I mean why do we need

componentDidMount() {
  this.timer = setInterval(this.tick, 100);
}

[martpie]

For this and components perfs reason.

Do you have a better idea ?

edit: it could be paused when there's no track playing.
re-edit: let's talk about that on Gitter

[YurySolovyov]

Ah, got it.

it could be paused when there's no track playing.

Sounds good.
Also, maybe increase timeout a bit? Say, 250 ms -> 4 times per second.

[martpie]

We can, I've chosen 150 because the progress bar was more fluid that way.

[YurySolovyov]

Maybe in another PR though.

[martpie]

Let's not pollute this PR too much indeed 😝

[YurySolovyov]

@cfollet This looks almost perfect, but can you please move Player.getAudio() calls to local variable? Should've done it earlier myself

[martpie]

It's ok for me. I've made it that way so we now where this come from. This will have to move elsewhere anyway.

[martpie]

Thanks for your work @cfollet !