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

Make inactivity timeout configurable #1409

Merged
merged 8 commits into from Sep 2, 2014

Conversation

Projects
None yet
4 participants
@knabar
Contributor

knabar commented Aug 11, 2014

This PR is for issue #1408 and makes the inactivity timeout configurable, so e.g. the controls can be hidden quicker or slower than the default 2000 ms:

<script>
    videojs.options.inactivityTimeout = 5000;
</script>
@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Aug 11, 2014

Member

Seems reasonable to me.

Member

gkatsev commented Aug 11, 2014

Seems reasonable to me.

@mmcc

This comment has been minimized.

Show comment
Hide comment
@mmcc

mmcc Aug 11, 2014

Member

Yep, seems reasonable to me too. Unless @heff has objections, we'll pull this one in soon.

Member

mmcc commented Aug 11, 2014

Yep, seems reasonable to me too. Unless @heff has objections, we'll pull this one in soon.

Show outdated Hide outdated src/js/player.js
@@ -1624,7 +1624,7 @@ vjs.Player.prototype.listenForUserActivity = function(){
if (!this.userActivity_) {
this.userActive(false);
}
}), 2000);
}), vjs.options['inactivityTimeout']);

This comment has been minimized.

@gkatsev

gkatsev Aug 11, 2014

Member

Actually, this should probably check the current player's options for inactivityTimeout so that you can have different players with different timeouts.

@gkatsev

gkatsev Aug 11, 2014

Member

Actually, this should probably check the current player's options for inactivityTimeout so that you can have different players with different timeouts.

This comment has been minimized.

@knabar

knabar Aug 11, 2014

Contributor

I have not dug into the code enough yet to know how to do that, but I'll take a look tomorrow.

@knabar

knabar Aug 11, 2014

Contributor

I have not dug into the code enough yet to know how to do that, but I'll take a look tomorrow.

This comment has been minimized.

@gkatsev

gkatsev Aug 11, 2014

Member

Should be something like this.options()['inactivityTimeout']

@gkatsev

gkatsev Aug 11, 2014

Member

Should be something like this.options()['inactivityTimeout']

@mmcc mmcc referenced this pull request Aug 14, 2014

Closed

Crashing IE8 #1213

@knabar

This comment has been minimized.

Show comment
Hide comment
@knabar

knabar Aug 14, 2014

Contributor

Completely disabling the timeout (see #1213) would be trivial to add, perhaps by setting inactivityTimeout to undefined?

Contributor

knabar commented Aug 14, 2014

Completely disabling the timeout (see #1213) would be trivial to add, perhaps by setting inactivityTimeout to undefined?

@knabar

This comment has been minimized.

Show comment
Hide comment
@knabar

knabar Aug 14, 2014

Contributor

Just realized undefined is probably not practical since JSON cannot express it. Alternatives: null, -1, ...?

Contributor

knabar commented Aug 14, 2014

Just realized undefined is probably not practical since JSON cannot express it. Alternatives: null, -1, ...?

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Aug 14, 2014

Member

To disable it, just set it to 0.

Member

gkatsev commented Aug 14, 2014

To disable it, just set it to 0.

@knabar

This comment has been minimized.

Show comment
Hide comment
@knabar

knabar Aug 15, 2014

Contributor

If inactivityTimeout is set to anything less than 1, the controls will no longer hide.

Contributor

knabar commented Aug 15, 2014

If inactivityTimeout is set to anything less than 1, the controls will no longer hide.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Aug 22, 2014

Member

This looks great, thanks Andreas! Any chance you want to try and write a test for this? There's not an existing one so it might be a challenge, but let me know if you want to try.

Member

heff commented Aug 22, 2014

This looks great, thanks Andreas! Any chance you want to try and write a test for this? There's not an existing one so it might be a challenge, but let me know if you want to try.

@heff heff added confirmed labels Aug 22, 2014

@knabar

This comment has been minimized.

Show comment
Hide comment
@knabar

knabar Aug 25, 2014

Contributor

@heff I added a test to check the different inactivityTimeout settings. Due to the nature of the tests, this adds about 8 seconds to the test execution time.

Contributor

knabar commented Aug 25, 2014

@heff I added a test to check the different inactivityTimeout settings. Due to the nature of the tests, this adds about 8 seconds to the test execution time.

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Aug 25, 2014

Member

Looks like we should pull in sinon's fake timers for this test. Asyncornous tests are always flaky. sinon's fake timers allow you to control time in your tests which will make them run faster and more reliably.

Member

gkatsev commented Aug 25, 2014

Looks like we should pull in sinon's fake timers for this test. Asyncornous tests are always flaky. sinon's fake timers allow you to control time in your tests which will make them run faster and more reliably.

@knabar

This comment has been minimized.

Show comment
Hide comment
@knabar

knabar Aug 25, 2014

Contributor

@gkatsev I was not aware of that library, thanks!

Contributor

knabar commented Aug 25, 2014

@gkatsev I was not aware of that library, thanks!

Show outdated Hide outdated test/unit/player.js
player = PlayerTest.makePlayer({});
html5Mock = { player_: player };
vjs.Html5.prototype.createEl.call(html5Mock);

This comment has been minimized.

@heff

heff Aug 27, 2014

Member

Is the html5mock part needed here?

@heff

heff Aug 27, 2014

Member

Is the html5mock part needed here?

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Aug 27, 2014

Member

Tests look great, thanks!

Member

heff commented Aug 27, 2014

Tests look great, thanks!

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Aug 27, 2014

Member

@gkatsev is zero the right setting here to disable it or should we use false? I'm worried there's a disconnect since setTimeout(fn, 0) still executes the function.

Member

heff commented Aug 27, 2014

@gkatsev is zero the right setting here to disable it or should we use false? I'm worried there's a disconnect since setTimeout(fn, 0) still executes the function.

@knabar

This comment has been minimized.

Show comment
Hide comment
@knabar

knabar Aug 28, 2014

Contributor

@heff removed unneeded html5Mock code

Contributor

knabar commented Aug 28, 2014

@heff removed unneeded html5Mock code

@heff heff merged commit c4b4ebe into videojs:master Sep 2, 2014

1 check passed

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

heff added a commit that referenced this pull request Sep 2, 2014

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Sep 2, 2014

Member

Thanks Andreas!

Member

heff commented Sep 2, 2014

Thanks Andreas!

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