The only one noticeable issue happens when using views/colorbox. If the Video Style is set to autoplay (which would be cool when launching it in colorbox) the sound plays before you actually click the thumbnail preview. I'm guessing that also means the video is loading too. I've only tested Vimeo so far
- mrtalloy from #1336672: Load videos in colorbox.
I'm creating a new issue here so we can work on this separately. My first thought is to create a callback to populate the colorbox when it is opened. Definitely open to options here.
| Comment | File | Size | Author |
|---|---|---|---|
| #16 | 1387020-remove-session-4.patch | 4.39 KB | jec006 |
| #15 | 1387020-remove-session-3.patch | 4.45 KB | jec006 |
| #14 | 1387020-remove-session-2.patch | 4.51 KB | jec006 |
| #10 | 1387020-remove-session.patch | 5.49 KB | jec006 |
| #3 | patch_commit_967fda17eea4.patch | 3.85 KB | jdelaune |
Comments
Comment #1
jdelaune commentedYup it's the issue you get with any lightbox type feature and loading in videos. The only way would be to create a callback like you say and load it in after a click.
Comment #2
jdelaune commentedHere's the patch. Errr wrong one sorry see below.
Comment #3
jdelaune commentedHere's the real patch.
Comment #4
jec006 commentedThis looks good - committed here: http://drupalcode.org/project/video_embed_field.git/commit/d3f844f
Thanks (again) jdelaune
Comment #5
jec006 commentedComment #6
jec006 commentedI'd like to think about options for getting rid of the use of $_SESSION storage and making it more stateless - I think (though I don't know for sure) that this could become an issue for sites that are using a static cache or something of the nature as the $_SESSION wouldn't be properly populated.
I think the real rub here is that a user should only be able access videos that the site admin has embedded - so simply including the youtube/vimeo id in the link isn't ideal. I was thinking perhaps the id of the entity to which the field is attached, or something along those lines - so it can be loaded again and displayed when the callback is hit?
I think this would be a bit of an invasive change so for right now, I've committed the patch in #3 to fix the main issue. I'd like to ensure that this will work on large scale sites before we release another version of the module though. I'll try to do some work on this soon (hopefully this weekend).
Comment #7
jdelaune commentedYeah I do agree but it was the best solution I could find. I was going to go down the entity id, field name, delta route. This works great when using fields but not if you use an embed code. However the embed code doesn't currently support colorbox either so maybe we say this feature is purely for fields? I was going to work on the embed code next but I'm not sure how else we could do it.
It might be nice to make the embed code depend on a field, a bit like how the insert module works with image field? Then at least we will always have a field to reference. Maybe that's a way forward in the future.
Comment #8
jdelaune commentedAlso one bug in my previous patch. Line 83 of video_embed_field.module needs to read
'access callback' => TRUE,.Comment #9
jec006 commentedAlright, I can fix that. Also thanks for fixing the .info file issues.
Embed codes is a good point, we can think about that - but I agree about the fields.
Comment #10
jec006 commentedOk, here's a potential solution, wanted to get some feedback before I committed it though.
Basically I added a table to store url/style combinations and then load them for colorbox. It doesn't have any dependence on a field so Embeds should work ok.
The three things I'm semi worried about are:
1. The first view results in a db write - is this is a big deal?
2. Do we need more caching? Is this going to be slow on list pages or some such?
3. Do we need to clean up this table as fields are deleted? Are there other scenarios where we should clean up?
I think cleaning up is potentially a bad idea as the videos may not be stored on a field, or multiple field instances may reference the same video ect. It might actually be a good idea to use a cache table instead of a real table - thoughts?
I tried to add enough caching to make things good, I think perhaps if we need more we should load the whole store, and then cache it, so we only make one database call.
Comment #11
jdelaune commentedLooks good. I don't think:
1. Isn't really an issue.
2. We will have to do some tests.
3. It will be tricky as you say. Maybe we could add a manual flush option.
Use of a cache table might be sensible since this is what we are using it for really.
Comment #12
jec006 commentedYah, I agree on the cache table thing, I will try to redo using a cache table instead when I get a chance - I'm moving this weekend so I probably won't have time until next week.
Comment #13
jdelaune commentedhah ok. Good luck with the move :)
Comment #14
jec006 commentedThanks, i made it ... barely.
So, i did a little bit of refactoring and here's what i've got - this one uses cache_set and get instead of a custom table. Seems to work well, based on my very limited testing.
Comment #15
jec006 commentedDurrr, I left a debugging statement in there. here's a new patch.
Comment #16
jec006 commentedsecond durrr - one more, i think i got them all this time.
Take a look - i think this is good and works how we expect.
Comment #17
jec006 commentedCommitted here: http://drupal.org/commitlog/commit/25834/b3f8627c1c780f4250f05b23146166e...
Comment #18
jdelaune commentedWorks well from my testing.
Comment #19.0
(not verified) commentedUpdating link to other issue