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

Clear model cues on playlist item in controller #3331

Merged
merged 6 commits into from Mar 15, 2019

Conversation

Projects
None yet
4 participants
@pajong
Copy link
Member

pajong commented Mar 14, 2019

This PR will...

Clear model cues on playlistItemchange in controller, rather than in timeslider

Why is this Pull Request needed?

Timeslider's model is the viewModel, which updates the playlistItem every time we enter/exit ads mode.
We only want to clear cues on the playlistItem when the actual content item changes.
Moving the model.set('cues', []); to be called with _model in controller (which is the actual player model, not viewModel).

PLEASE NOTE: model from model.set('cues, []) is different from _model in controller (https://github.com/jwplayer/jwplayer/compare/bugfix/reset-cues-on-playlistItem?expand=1#diff-a181c6e20596563a55e5710b8f4dbab7R250)

Are there any points in the code the reviewer needs to double check?

Are there any Pull Requests open in other repos which need to be merged with this?

Addresses Issue(s):

JW8-5672

Clear model cues on playlist item in controller
Time slider's onPlaylistItem triggers every time we enter/exit ads mode.
We should only clear cues on playlistItem when the actual content changes - in the controller.

JW8-5672

@pajong pajong requested a review from robwalch as a code owner Mar 14, 2019

@pajong pajong requested a review from waxidiotic Mar 14, 2019

@waxidiotic waxidiotic added this to the 8.8.0 milestone Mar 14, 2019

@jwplayer-robot

This comment has been minimized.

Copy link

jwplayer-robot commented Mar 14, 2019

⚠️ MULTI Build for commit 505ee16 is unstable (UNSTABLE).
🏗 jwplayer build SUCCESS
🏗 jwplayer browserstack tests SUCCESS
🏗 jwplayer-commercial build SUCCESS
🏗 jwplayer-commercial browserstack tests SUCCESS
🥒 Automated Tests UNSTABLE
🍆 Manual Tests
📺 Views

@pajong pajong requested a review from jnatalzia as a code owner Mar 14, 2019

@jwplayer-robot

This comment has been minimized.

Copy link

jwplayer-robot commented Mar 14, 2019

⚠️ MULTI Build for commit a27a68f is unstable (UNSTABLE).
🏗 jwplayer build SUCCESS
🏗 jwplayer browserstack tests SUCCESS
🏗 jwplayer-commercial build SUCCESS
🏗 jwplayer-commercial browserstack tests SUCCESS
🥒 Automated Tests UNSTABLE
🍆 Manual Tests
📺 Views

addressed

@@ -106,6 +106,12 @@ class TimeSlider extends Slider {
.change('buffer', this.onBuffer, this)
.change('streamType', this.onStreamType, this);

// Clear cues on player model's playlistItem change event
const { _model } = this._model;
_model.change('playlistItem', (model) => {

This comment has been minimized.

@robwalch

robwalch Mar 15, 2019

Member

this._model.player get's the player view model. You should not access the internal _model directly. PlayerViewModel only surfaces player model and media model attributes and changes, while ViewModel also adds instream when active.

We use viewModel.player in places where we don't want any ads updates such as the captionsrenderer.js.

This comment has been minimized.

@robwalch

robwalch Mar 15, 2019

Member

this._model.player.on('change:playlistItem', this.onPlaylistItem, this);

You'll need to update the 'Control Bar' tests as well since they use a mock SimpleModel that will need a player property added.

@robwalch
Copy link
Member

robwalch left a comment

We should not have a playlist callback for the view model and the model. This is adding too much code instead of replacing the problematic change callback.

Do not change the view model. Use viewModel.player to trigger this.onPlaylistItem. We can then also remove the playlistItem check at the beginning of the callback because the player view model always will have a playlistItem.

const playerModel = new SimpleModel();
playerModel.change = sinon.stub();
playerModel.change.returnsThis();
model._model = playerModel;

This comment has been minimized.

@robwalch

robwalch Mar 15, 2019

Member

Remove. Use .player

@@ -177,7 +183,7 @@ class TimeSlider extends Slider {
return;

This comment has been minimized.

@robwalch

robwalch Mar 15, 2019

Member

This check can be removed once you use the player view model.

const playerModel = new SimpleModel();
playerModel.change = sinon.stub();
playerModel.change.returnsThis();
model.player = playerModel;

This comment has been minimized.

@pajong

pajong Mar 15, 2019

Author Member

this is still needed, as SimpleModel does not have an instance of player model.

This comment has been minimized.

@robwalch
@jwplayer-robot

This comment has been minimized.

Copy link

jwplayer-robot commented Mar 15, 2019

❗️ MULTI Build for commit 1a55de0 did not complete (FAILURE).
🏗 jwplayer build SUCCESS
🏗 jwplayer browserstack tests SUCCESS
🏗 jwplayer-commercial build SUCCESS
🏗 jwplayer-commercial browserstack tests FAILURE
🥒 Automated Tests ${JW7_COMMERCIAL_CUCUMBER_BUILD_RESULT}
🍆 Manual Tests
📺 Views

@waxidiotic waxidiotic self-requested a review Mar 15, 2019

@waxidiotic
Copy link
Member

waxidiotic left a comment

I tested the various configs that I have used to reproduce this issue and the original issue, along with the API methods (setCues(), getCues(), addCues()) and everything looks good. 👍

this.reset();
model.set('cues', []);
this.updateCues(model, model.get('cues'));

This comment has been minimized.

@robwalch

robwalch Mar 15, 2019

Member

Where are cues being reset now?

This comment has been minimized.

@pajong

pajong Mar 15, 2019

Author Member

Thank you! It was working fine when I was because cues are being updated when they load.

@jwplayer-robot

This comment has been minimized.

Copy link

jwplayer-robot commented Mar 15, 2019

⚠️ MULTI Build for commit 0a465a1 is unstable (UNSTABLE).
🏗 jwplayer build SUCCESS
🏗 jwplayer browserstack tests SUCCESS
🏗 jwplayer-commercial build SUCCESS
🏗 jwplayer-commercial browserstack tests SUCCESS
🥒 Automated Tests UNSTABLE
🍆 Manual Tests
📺 Views

@robwalch robwalch merged commit fad5603 into master Mar 15, 2019

3 of 4 checks passed

jw7-pr-multi-opensource Build finished.
Details
Danger All green. Nice work.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.