Feature/current source #2678

chemoish · 2015-10-06T17:34:07Z

Original PR: #2479

pam · 2015-10-06T17:41:37Z

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 4dc3e77
Build details: https://travis-ci.org/pam/video.js/builds/83938385

(Please note that this is a fully automated comment.)

chemoish · 2015-10-14T17:38:31Z

Addresses #2443

chemoish · 2015-10-26T18:27:53Z

@heff Any status updates?

gkatsev · 2015-11-09T19:37:30Z

Unfortunately, I don't think we'll get it in a 5.2 release but I'll definitely take a look at it next.

gkatsev · 2016-02-02T20:06:52Z

Looks like this needs to be rebased against master :)
Also, this mostly looks good. One thing that we should double check is that it's working correctly with things like source handlers. I don't foresee any issues but best to check it out.

…ource objects—when using `<source>` or `src()`. This is needed for source retrival for two use cases: - preroll - resolution switch (kind of) Currently there is no way of accessing the source object for any DRM specified content. Plugin access only has API methods to `currentSrc` and `currentType`. This will allow for `currentSource` and `currentSources` to return unverified source object(s). # Conflicts: # src/js/player.js # test/unit/player.js

…urce, have the current sources be set to null. When the `currentSources()` are called after `src('http://example.com')`, expect the `currentSources()` to only return the newly set source. This way sources, via `<source>` or `src([...])` list, should never be out of sync when redefining a single source. Updating tests to specify behavior changes. Remove autoplay from fixtures to bypass `loadTech()` ramifications. # Conflicts: # src/js/player.js # test/unit/player.js

chemoish · 2016-08-26T22:47:03Z

src/js/player.js

+   * @method currentSource
+   */
+  currentSource() {
+    return this.cache_.source || { src: this.currentSrc() };


if this.currentSrc() returns undefined should this method return {} instead?

https://github.com/videojs/video.js/compare/master...gesinger:feature/current-source-obj?expand=1#diff-3b0266ff1c037b289ec624ab25b0272eR2092

That sounds fine to me.

My initial thought is to throw away all the code for currentSrc() since now is very duplicative and loses information such as type. Though this is a breaking change and there is probably a big hole in my logic.

Atm, currentSrc() returns a string or undefined…

I think it makes sense to translate that across those methods.

currentSrc() === undefined currentSource() === undefined currentSources() === undefined vs. currentSrc() === undefined currentSource() === { src: undefined } or {} currentSources() === [{ src: undefined }] or []

So I would recommend undefined or {} and [].

I lean toward all three methods returning undefined if there is no source.

My second preference would be {} or [] for the two new ones, but I feel like in that case, it would make more sense for currentSrc to return '' instead of undefined.

Unfortunately, undefined would be not backwards compatible but also would break away from what the video element does for currentSrc:

document.createElement('video').currentSrc // ""

As for the others, returning an empty object and empty array would make it consistent. Though, would be a bit annoying to check for whether it's empty or not.

Oh! I thought currentSrc returned undefined. In that case, I vote for currentSource to return undefined and currentSources to return [].

Having currentSource return undefined seems inconsistent to me. Both from it's internal type signature but also between the 3 methods.
currentSrc always returns a string and currentSources always returns an array, why should currentSource be different?

chemoish · 2016-08-26T22:48:22Z

@gesinger, does test/unit/tech/tech-faker.js require currentSrc() {}?

https://github.com/videojs/video.js/compare/master...gesinger:feature/current-source-obj?expand=1#diff-1291a8d3b0422be08a16485f63ff9ca3R39

chemoish · 2016-09-21T19:00:47Z

@gkatsev any update?

gkatsev

@gesinger do you have any changes that we should apply to this PR and merge?

gesinger · 2016-09-21T21:44:49Z

@gkatsev only thing I might consider is the check for no currentSrc from https://github.com/videojs/video.js/compare/master...gesinger:feature/current-source-obj?expand=1#diff-3b0266ff1c037b289ec624ab25b0272eR2092

gesinger · 2016-09-22T19:00:51Z

On second thought, I think it is fine either way.

…Source` to return `{}` when `src` is `''`. NOTE: these tests only pass because they are hitting the `cache_`. - Fix tests - Add method to `TechFaker`

chemoish

@gesinger @gkatsev @misteroneill Update per initial conversation, minor to change again.

(First time trying this review tool)

I don't know how to properly test src changes as there isn't any other tests that validate sources. It looks like I needed to update TechFaker for non-empty <source> tags with unspecified tech?, but in either case I don't think it properly tests the methods—only the cache_?

chemoish · 2016-09-22T22:25:34Z

src/js/player.js

+   * @return {Object[]} The current source objects
+   * @method currentSources
+   */
+  currentSources() {


updated to return [] when src is ''

chemoish · 2016-09-22T22:26:07Z

src/js/player.js

+    const sources = [];
+
+    // assume `{}` or `{ src }`
+    if (Object.keys(source).length !== 0) {


seemed like a fine isEmpty check

Agreed. I don't know if it's merged yet, but I introduced utils/obj in a PR recently. An isEmpty could be useful there. If not, this is fine, I think.

chemoish · 2016-09-22T22:26:29Z

src/js/player.js

+   * @return {Object} The current source object
+   * @method currentSource
+   */
+  currentSource() {


updated to return {} when src is ''

chemoish · 2016-09-23T05:28:20Z

I looked for an isEmpty and every permutation/util in between. Could have
missed it, link and i'll update.

On Thu, Sep 22, 2016 at 5:16 PM, misteroneill notifications@github.com
wrote:

@misteroneill commented on this pull request.

In src/js/player.js #2678:

@@ -2062,6 +2070,41 @@ class Player extends Component {
}

/**

* Returns the current source objects.

* @return {Object[]} The current source objects

* @method currentSources

*/

currentSources() {

const source = this.currentSource();

const sources = [];

// assume {} or { src }

if (Object.keys(source).length !== 0) {

Agreed. I don't know if it's merged yet, but I introduced utils/obj in a
PR recently. An isEmpty could be useful there. If not, this is fine, I
think.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#2678, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAhhxwTCjoDrtuABO6_i-LxdusFSLQO3ks5qsxpLgaJpZM4GJ8bx
.

gkatsev · 2016-09-27T18:43:48Z

src/js/player.js

+      sources.push(source);
+    }
+
+    return this.cache_.sources || sources;


should we do the work of setting up the sources variable if we're just going to return this.cache_.sources?

Do you know off the top of your head that sources will never get called because this.cache_ will always be present?

I most likely implemented this to follow suit of the original implementation—thinking that there will be a difference between <source> and src, but since the PR been open for some time I have no idea anymore.

I can investigate further if unknown.

I'm not sure this is a big concern. It's fine to leave as is and we can always profile and increase performance in the future.

gkatsev · 2016-09-27T18:44:01Z

src/js/player.js

+      source.src = src;
+    }
+
+    return this.cache_.source || source;


same question here.

gkatsev

Just tried it out locally. Works great.

chemoish mentioned this pull request Oct 6, 2015

Update current content video to be sourced from snapshot.source ins… videojs/videojs-contrib-ads#110

Closed

chemoish mentioned this pull request Oct 28, 2015

videojs 5.1 release proposal #2742

Closed

18 tasks

gkatsev added the minor This PR can be added to a minor release. It should not be added to a patch release. label Nov 24, 2015

chemoish mentioned this pull request Dec 29, 2015

clear out currentSource on loadstart #2957

Closed

gkatsev added the needs: LGTM Needs one or more additional approvals label Jan 26, 2016

gkatsev removed the needs: LGTM Needs one or more additional approvals label Feb 3, 2016

gkatsev added the needs: updates label May 11, 2016

gkatsev mentioned this pull request Aug 25, 2016

Method for Accessing Source Objects #3564

Closed

Carey Hinoki added 2 commits August 26, 2016 15:23

chemoish force-pushed the feature/currentSource branch from 4dc3e77 to 5248d86 Compare August 26, 2016 22:43

chemoish reviewed Aug 26, 2016
View reviewed changes

chemoish mentioned this pull request Sep 21, 2016

dynamic src - error when stop casting benjipott/video.js-chromecast#9

Closed

gkatsev reviewed Sep 21, 2016

View reviewed changes

refactor(player): Update currentSources to return [] and `current…

3a4b293

…Source` to return `{}` when `src` is `''`. NOTE: these tests only pass because they are hitting the `cache_`. - Fix tests - Add method to `TechFaker`

chemoish commented Sep 22, 2016

View reviewed changes

gkatsev reviewed Sep 27, 2016

View reviewed changes

gkatsev added this to the 5.13 milestone Sep 27, 2016

gkatsev approved these changes Oct 3, 2016

View reviewed changes

gkatsev added the confirmed label Oct 3, 2016

gkatsev removed the needs: updates label Oct 3, 2016

gkatsev merged commit 028559c into videojs:master Nov 3, 2016

mysuf mentioned this pull request Jul 24, 2018

snapshot stores only one source videojs/videojs-contrib-ads#408

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/current source #2678

Feature/current source #2678

chemoish commented Oct 6, 2015

pam commented Oct 6, 2015

chemoish commented Oct 14, 2015

chemoish commented Oct 26, 2015

gkatsev commented Nov 9, 2015

gkatsev commented Feb 2, 2016

chemoish Aug 26, 2016 •

edited

Loading

gkatsev Sep 21, 2016

chemoish Sep 22, 2016

misteroneill Sep 22, 2016

gkatsev Sep 22, 2016

misteroneill Sep 22, 2016

gkatsev Sep 22, 2016

chemoish commented Aug 26, 2016 •

edited

Loading

chemoish commented Sep 21, 2016

gkatsev left a comment

gesinger commented Sep 21, 2016

gesinger commented Sep 22, 2016

chemoish left a comment

chemoish Sep 22, 2016

chemoish Sep 22, 2016 •

edited

Loading

misteroneill Sep 23, 2016

chemoish Sep 22, 2016

chemoish commented Sep 23, 2016

@misteroneill commented on this pull request.

gkatsev Sep 27, 2016

chemoish Sep 29, 2016

gkatsev Sep 30, 2016

gkatsev Sep 27, 2016

gkatsev left a comment

Feature/current source #2678

Feature/current source #2678

Conversation

chemoish commented Oct 6, 2015

pam commented Oct 6, 2015

chemoish commented Oct 14, 2015

chemoish commented Oct 26, 2015

gkatsev commented Nov 9, 2015

gkatsev commented Feb 2, 2016

chemoish Aug 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chemoish commented Aug 26, 2016 • edited Loading

chemoish commented Sep 21, 2016

gkatsev left a comment

Choose a reason for hiding this comment

gesinger commented Sep 21, 2016

gesinger commented Sep 22, 2016

chemoish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chemoish Sep 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chemoish commented Sep 23, 2016

@misteroneill commented on this pull request.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkatsev left a comment

Choose a reason for hiding this comment

chemoish Aug 26, 2016 •

edited

Loading

chemoish commented Aug 26, 2016 •

edited

Loading

chemoish Sep 22, 2016 •

edited

Loading