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

[Feature proposition] Allow to render a torrent into an existing element #425

Merged
merged 2 commits into from Dec 28, 2015

Conversation

5 participants
@valeriangalliat
Contributor

valeriangalliat commented Sep 10, 2015

Like file.appendTo, but taking an existing element as parameter, for example:

<img src="magnet:?xt=urn:btih:d2346c943f770eba4994e25fa7b478692bcf0064&amp;dn=test.png">
const WebTorrent = require('webtorrent')
const client = new WebTorrent()

Array.from(document.querySelectorAll('[src^=magnet:]')).forEach(elem =>
  client.add(elem.src, torrent =>
    torrent.files[0].renderTo(elem)
  )
)

See the Implement 'render-to' method commit to have a proper diff, because append-to.js was renamed to render.js and it's messing the full PR diff.

Related: #531.

@valeriangalliat

This comment has been minimized.

Contributor

valeriangalliat commented Nov 8, 2015

Any comment on this feature?

@madd512

This comment has been minimized.

madd512 commented Nov 9, 2015

I like it. I've wanted this myself.

@RomanEmelyanov

This comment has been minimized.

RomanEmelyanov commented Dec 22, 2015

I'm work under #531

render-to method does not include yet to main branch :(

Do you have the patch as standalone javascript file to include as the second after:
https://cdn.jsdelivr.net/webtorrent/latest/webtorrent.min.js
?

@valeriangalliat

This comment has been minimized.

Contributor

valeriangalliat commented Dec 22, 2015

@feross I rebased the PR, ready to merge in master! I'd like a review and approval of other WebTorrent contributors before merging, do you know who to ping? Thanks. :)

}
function fatalError (err) {
err.message = 'Error appending file "' + file.name + '" to DOM: ' + err.message

This comment has been minimized.

@madd512

madd512 Dec 22, 2015

This won't be accurate anymore, because "appending"

@madd512

This comment has been minimized.

madd512 commented Dec 22, 2015

It looks pretty good to me, but if you don't mind a little unsolicited feedback, I'd call handle.js something different, maybe render.js, just to make it easier for newcomers to know where to look when exploring the source tree.

@valeriangalliat

This comment has been minimized.

Contributor

valeriangalliat commented Dec 22, 2015

@devtristan Thanks for your comments, I fixed the error message and renamed handle.js to render.js, it indeed makes more sense.

Implement 'render-to' method
* Update 'handle' (previously 'append-to') to take a 'getElem' function
  instead of directly a root element, allowing to custimize the element
  injection method, and without directly manipulating the DOM.
* Reimplement 'append-to' using abstract 'handle' method.
* Implement a 'render-to' method to render a torrent inside an existing
  element.
if (err) return fatalError(err)
elem.src = url
elem.alt = file.name
cb(null)

This comment has been minimized.

@feross

feross Dec 28, 2015

Member

This should be cb(null, elem)

elem.play()
})
elem.autoplay = true // for chrome
elem.play() // for firefox

This comment has been minimized.

@feross

feross Dec 28, 2015

Member

createMedia needs to append to the DOM too. Right now, only createElem does.

@feross

This comment has been minimized.

Member

feross commented Dec 28, 2015

@valeriangalliat Thanks for the good PR! It has a few issues (mentioned as inline comments), but I can fix those up after merge.

You rock!

feross added a commit that referenced this pull request Dec 28, 2015

Merge pull request #425 from valeriangalliat/feature/render-to
[Feature proposition] Allow to render a torrent into an existing element

@feross feross merged commit 0210d55 into webtorrent:master Dec 28, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

feross added a commit that referenced this pull request Dec 28, 2015

@feross

This comment has been minimized.

Member

feross commented Dec 28, 2015

Released as 0.65.0.

@valeriangalliat valeriangalliat deleted the valeriangalliat:feature/render-to branch Dec 28, 2015

@valeriangalliat

This comment has been minimized.

Contributor

valeriangalliat commented Dec 28, 2015

Awesome, thanks for fixing the issues!

@feross

This comment has been minimized.

Member

feross commented Dec 28, 2015

No problem. Thanks for taking the initiative to fix the issue :) I really appreciate that.

@RomanEmelyanov

This comment has been minimized.

RomanEmelyanov commented Dec 29, 2015

Thanks a lot! Any plans to add "renderTo" method to "https://cdn.jsdelivr.net/webtorrent/latest/webtorrent.min.js" ?

@feross

This comment has been minimized.

Member

feross commented Jan 1, 2016

@RomanEmelyanov The new code should be available at that url. You might need to clear your cache to see it.

@RomanEmelyanov

This comment has been minimized.

RomanEmelyanov commented Jan 10, 2016

What I'm do wrong?
Tested on Google Chrome.

<html><head>
<script src="https://cdn.jsdelivr.net/webtorrent/latest/webtorrent.min.js"></script>
</head><body><script>
var client = new WebTorrent()
var torrentId = 'magnet:?xt=urn:btih:6a9759bffd5c0af65319979fb7832189f4f3c35d'
client.add(torrentId, function (torrent) {
var file = torrent.files[0]
file.renderTo('#V1', function (err, elem) { // does not work :(
// file.appendTo('#V2', function (err, elem) { // works fine
 if (err) throw err
 console.log('New DOM node with the content', elem)
})

})
</script>
<div id="V2"></div>
<video id="V1"></video>
</body></html>
@RomanEmelyanov

This comment has been minimized.

RomanEmelyanov commented Jan 10, 2016

SOLVED: I'm forget to add "controls" and "autoplay" for <video id="V1"> tag, sorry to trouble

@nextgenthemes

This comment has been minimized.

nextgenthemes commented Oct 2, 2016

So if I put controls and autoplay on a element and you renderTo its the same as if I use opts on renderTo.

Also what does it do when the torrent file is not a video and I set it to render to a video tag?

@feross

This comment has been minimized.

Member

feross commented Oct 6, 2016

So if I put controls and autoplay on a element and you renderTo its the same as if I use opts on renderTo.

The default is true, so you don't need to set these options. I would rely on webtorrent to autoplay for you, since there are some browser differences. On Firefox, you have to explicitly call video.play() after the metadata has been added to the tag.

Also what does it do when the torrent file is not a video and I set it to render to a video tag?

It will try to put the file data into the video tag which will just make the video tag fire an 'error' event. I haven't tried it, but you can just try it and see.

@nextgenthemes

This comment has been minimized.

nextgenthemes commented Oct 6, 2016

@feross Thanks. But I need to disable autoplay and I was thinking it would be convenient to use renderTo in a video that my php code already generates that also has a class that I wanted to use. Now I am thinking this entire renderTo thing is probably a not so good idea and just complicates things.

Between the lines you answered my question with "No". So the renderTo is not smart to read the video tag attributes as opts and then doing kind of the same thing as appendTo. I feel more comfortable with the appendTo and opts I will just read from data-attributes now.

@feross

This comment has been minimized.

Member

feross commented Oct 8, 2016

You can disable autoplay by passing an { autoplay: false } option to either method.

@nextgenthemes

This comment has been minimized.

nextgenthemes commented Oct 8, 2016

I know and I do this now.

            // Stream the file in the browser
            torrent.files[0].appendTo( $container, {
                autoplay: $wrapper.hasAttribute('data-arve-autoplay'),
                controls: $wrapper.hasAttribute('data-arve-controls')
            } );
@lock

This comment has been minimized.

lock bot commented May 4, 2018

This thread has been automatically locked because it has not had recent activity. To discuss futher, please open a new issue.

@lock lock bot locked as resolved and limited conversation to collaborators May 4, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.