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

track and manage video state #890

Merged
merged 14 commits into from Jan 6, 2018

Conversation

Projects
None yet
4 participants
@daovist
Copy link
Contributor

daovist commented Dec 21, 2017

This feature tracks play and pause actions on a video element. Below is a model of the parts.

blue = state
red = actions
purple = reducers
light blue = selectors
green = component lifecycle hooks
orange = distant component which can alter state
black = not React/Redux

lbry_video_state_model

@liamcardenas
Copy link
Contributor

liamcardenas left a comment

Great stuff! I see you are tracking time when the video pauses, I was hoping that you would update the time using this listener instead https://www.w3schools.com/tags/av_event_timeupdate.asp

i.e. mediaElement.addEventListener("timeupdate", doUpdatePlayback);

@daovist

This comment has been minimized.

Copy link
Contributor Author

daovist commented Dec 21, 2017

Are we using the time for something other than starting back where the user left off?

@liamcardenas

This comment has been minimized.

Copy link
Contributor

liamcardenas commented Dec 22, 2017

@daovist yes, exactly. There are going to be several features that depend on this

@liamcardenas
Copy link
Contributor

liamcardenas left a comment

Overall excellent PR! I tested it locally and it works well, but there are a few small changes before it can be merged.

@@ -0,0 +1,15 @@
import * as settings from "constants/settings";

This comment has been minimized.

@liamcardenas

liamcardenas Dec 27, 2017

Contributor

I noticed that /selectors/video.js has not been deleted. Are we still using it?

@@ -71,6 +72,7 @@ const reducers = combineReducers({
shapeShift: shapeShiftReducer,
subscriptions: subscriptionsReducer,
video: videoReducer,

This comment has been minimized.

@liamcardenas

liamcardenas Dec 27, 2017

Contributor

do we still need video: videoReducer,?


export const makeSelectMediaPositionForUri = uri =>
createSelector(_selectState, state => {
const id = uri.split("#")[1];

This comment has been minimized.

@liamcardenas

liamcardenas Dec 27, 2017

Contributor

will this work when the uri does not contain a # such as in the case of lbry://one?


export type MediaState = {
paused: Boolean,
positions: Object,

This comment has been minimized.

@liamcardenas

liamcardenas Dec 27, 2017

Contributor

can you define a type more specifically than Object? Maybe { [string]: number }?

https://flow.org/en/docs/types/objects/#toc-objects-as-maps

@@ -93,6 +96,9 @@ class Video extends React.PureComponent {
}
const poster = metadata.thumbnail;

const mediaId = uri.split("#")[1];

This comment has been minimized.

@liamcardenas

liamcardenas Dec 27, 2017

Contributor

as I said in a comment below, does this work for lbry://one?

@@ -93,6 +96,9 @@ class Video extends React.PureComponent {
}
const poster = metadata.thumbnail;

const mediaId = uri.split("#")[1];
console.log("mediaId:", mediaId);

This comment has been minimized.

@liamcardenas

liamcardenas Dec 27, 2017

Contributor

please remove when ready to merge :)

});

const perform = dispatch => ({
play: uri => dispatch(doPlayUri(uri)),
cancelPlay: () => dispatch(doSetPlayingUri(null)),
changeVolume: volume => dispatch(doChangeVolume(volume)),
setVideoPause: val => dispatch(setVideoPause(val)),
// setVideoPause: val => dispatch(setVideoPause(val)),

This comment has been minimized.

@liamcardenas

liamcardenas Dec 27, 2017

Contributor

please remove this comment when ready to merge

@@ -15,7 +16,11 @@ import {
} from "redux/selectors/file_info";
import { makeSelectCostInfoForUri } from "redux/selectors/cost_info";
import { selectShowNsfw } from "redux/selectors/settings";
import { selectVideoPause } from "redux/selectors/video";
// import { selectVideoPause } from "redux/selectors/video";

This comment has been minimized.

@liamcardenas

liamcardenas Dec 27, 2017

Contributor

please remove this comment when ready to merge

@@ -3,7 +3,8 @@ import { connect } from "react-redux";
import { doChangeVolume } from "redux/actions/app";
import { selectVolume } from "redux/selectors/app";
import { doPlayUri, doSetPlayingUri } from "redux/actions/content";
import { setVideoPause } from "redux/actions/video";
import { doPlay, doPause, savePosition } from "redux/actions/media";
// import { setVideoPause } from "redux/actions/video";

This comment has been minimized.

@liamcardenas

liamcardenas Dec 27, 2017

Contributor

please remove this comment when ready to merge :)

@kauffj

This comment has been minimized.

Copy link
Member

kauffj commented Dec 29, 2017

@daovist outpoint not claimid a8988d9

@kauffj
Copy link
Member

kauffj left a comment

Some minor code feedback, as well as the feedback given in office that it would be excellent if the video showed the same frame it was on prior to hitting play.

setVideoPause: val => dispatch(setVideoPause(val)),
doPlay: () => dispatch(doPlay()),
doPause: () => dispatch(doPause()),
savePosition: (id, position) => dispatch(savePosition(id, position)),

This comment has been minimized.

@kauffj

kauffj Jan 5, 2018

Member

what kind of id? I assume this is a claimId, let's call it that

doPause,
savePosition,
mediaPaused,
mediaPosition,

This comment has been minimized.

@kauffj

kauffj Jan 5, 2018

Member

The fact that Video is receiving so many properties simply to pass them on to VideoPlayer makes me think VideoPlayer should just be a first-class component. Although perhaps the names should be clarified if this is the case.

This comment has been minimized.

@daovist

daovist Jan 5, 2018

Author Contributor

I was also thinking it should be its own component. Maybe as a separate issue along with displaying a paused video instead of the image wrapper when position has a value?

This comment has been minimized.

@hackrush01

hackrush01 Jan 6, 2018

Member

I agree. #825 can work as a reference as the media component has been separated there. Also I think that Video and VideoPlayer names are very specific, we use the same component to render all other types of media as well, so IMO the variables and components should be refactored to reflect the same(also in the PR).
Thoughts?

@liamcardenas
Copy link
Contributor

liamcardenas left a comment

this looks good, im just going to check this out locally, but otherwise im happy with it

componentWillReceiveProps(next) {
const el = this.refs.media.children[0];
if (!this.props.paused && next.paused && !el.paused) el.pause();
// if (this.props.paused && !next.paused && el.paused) el.play();

This comment has been minimized.

@liamcardenas

liamcardenas Jan 5, 2018

Contributor

remove comment please

@liamcardenas liamcardenas merged commit eed68aa into lbryio:master Jan 6, 2018

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.