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

modernize lib/file.js and calculate downloaded correctly #1476

Closed
wants to merge 4 commits into from

Conversation

@jimmywarting
Copy link
Contributor

jimmywarting commented Aug 18, 2018

this fixes the calculation of .files[i].downloaded

@request-info

This comment has been minimized.

Copy link

request-info bot commented Aug 18, 2018

👋 We would appreciate it if you could provide us with more information about this issue.

@welcome

This comment has been minimized.

Copy link

welcome bot commented Aug 18, 2018

🙌 Thanks for opening this pull request! You're awesome.

Copy link
Contributor

KayleePop left a comment

LGTM

lib/file.js Outdated
if (this._torrent.bitfield.get(index)) {
// verified data
downloaded += (index === this._endPiece) ? this._torrent.lastPieceLength : this._torrent.pieceLength
downloaded += index === this._endPiece ? this._torrent.lastPieceLength : this._torrent.pieceLength

This comment has been minimized.

Copy link
@KayleePop

KayleePop Aug 18, 2018

Contributor

I think these parentheses make the code more readable.

This comment has been minimized.

Copy link
@jimmywarting

jimmywarting Aug 18, 2018

Author Contributor

Hmm, standard didn't complain and it wasn't needed so i thought i could safely remove them

This comment has been minimized.

Copy link
@KayleePop

KayleePop Aug 18, 2018

Contributor

It's just a style thing. probably not important.

This comment has been minimized.

Copy link
@DiegoRBaquero

DiegoRBaquero Aug 18, 2018

Member

I'd rather keep them

@jimmywarting

This comment has been minimized.

Copy link
Contributor Author

jimmywarting commented Aug 19, 2018

I also notice file.download is calculated wrong.

Using the Sintel torrent as an example...
https://instant.io/#08ada5a7a6183aae1e09d831df6748d566095a10
(this can be tested in the console)

It has 6 srt files and 1 small piece of the mp4 file in the first piece
after the first piece is downloaded you can see that the first files are complete but the progress and downloaded are incorrect

const file = client.get('08ada5a7a6183aae1e09d831df6748d566095a10').files[0]
console.log(file.done) // true
console.log(file.progress) // 39.58777239709443 <- incorrect b/c of download
console.log(file.downloaded) // 65399 <- this file fits inside one chunk and it's too big
console.log(file.length) // 1652

I'm working on did a fix for that now also

@jimmywarting jimmywarting changed the title modernize lib/file.js modernize lib/file.js and calculate downloaded correctly Aug 19, 2018
@KayleePop

This comment has been minimized.

Copy link
Contributor

KayleePop commented Aug 19, 2018

That should be a separate PR I think

@DiegoRBaquero

This comment has been minimized.

Copy link
Member

DiegoRBaquero commented Aug 19, 2018

Correct, please separate into two PRs, let's keep this one about modernizing the code.

@jimmywarting

This comment has been minimized.

Copy link
Contributor Author

jimmywarting commented Aug 20, 2018

ok, closing this then, new pr: #1478

@lock lock bot locked as resolved and limited conversation to collaborators Nov 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.