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
4 changes: 2 additions & 2 deletions src/renderer/component/fileDownloadLink/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { makeSelectCostInfoForUri } from "redux/selectors/cost_info";
import { doFetchAvailability } from "redux/actions/availability";
import { doOpenFileInShell } from "redux/actions/file_info";
import { doPurchaseUri, doStartDownload } from "redux/actions/content";
import { setVideoPause } from "redux/actions/video";
import { doPause } from "redux/actions/media";
import FileDownloadLink from "./view";

const select = (state, props) => ({
Expand All @@ -25,7 +25,7 @@ const perform = dispatch => ({
openInShell: path => dispatch(doOpenFileInShell(path)),
purchaseUri: uri => dispatch(doPurchaseUri(uri)),
restartDownload: (uri, outpoint) => dispatch(doStartDownload(uri, outpoint)),
setVideoPause: val => dispatch(setVideoPause(val)),
doPause: () => dispatch(doPause()),
});

export default connect(select, perform)(FileDownloadLink);
4 changes: 2 additions & 2 deletions src/renderer/component/fileDownloadLink/view.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,12 @@ class FileDownloadLink extends React.PureComponent {
purchaseUri,
costInfo,
loading,
setVideoPause,
doPause,
} = this.props;

const openFile = () => {
openInShell(fileInfo.download_path);
setVideoPause(true);
doPause();
};

if (loading || downloading) {
Expand Down
18 changes: 14 additions & 4 deletions src/renderer/component/video/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove this comment when ready to merge :)

import {
makeSelectMetadataForUri,
makeSelectContentTypeForUri,
Expand All @@ -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";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove this comment when ready to merge

import {
selectMediaPaused,
makeSelectMediaPositionForUri,
} from "redux/selectors/media";
import Video from "./view";
import { selectPlayingUri } from "redux/selectors/content";

Expand All @@ -29,14 +34,19 @@ const select = (state, props) => ({
playingUri: selectPlayingUri(state),
contentType: makeSelectContentTypeForUri(props.uri)(state),
volume: selectVolume(state),
videoPause: selectVideoPause(state),
// videoPause: selectVideoPause(state),
mediaPaused: selectMediaPaused(state),
mediaPosition: makeSelectMediaPositionForUri(props.uri)(state),
});

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)),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove this comment when ready to merge

doPlay: () => dispatch(doPlay()),
doPause: () => dispatch(doPause()),
savePosition: (id, position) => dispatch(savePosition(id, position)),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

});

export default connect(select, perform)(Video);
19 changes: 14 additions & 5 deletions src/renderer/component/video/internal/player.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,10 @@ class VideoPlayer extends React.PureComponent {
this.togglePlayListener = this.togglePlay.bind(this);
}

componentWillReceiveProps(nextProps) {
if (nextProps.videoPause) {
this.refs.media.children[0].pause();
this.props.setVideoPause(false);
}
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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove comment please

}

componentDidMount() {
Expand All @@ -35,7 +34,10 @@ class VideoPlayer extends React.PureComponent {
mediaType,
changeVolume,
volume,
mediaId,
position,
} = this.props;

const loadedMetadata = e => {
this.setState({ hasMetadata: true, startedPlaying: true });
this.refs.media.children[0].play();
Expand Down Expand Up @@ -68,6 +70,12 @@ class VideoPlayer extends React.PureComponent {
document.addEventListener("keydown", this.togglePlayListener);
const mediaElement = this.refs.media.children[0];
if (mediaElement) {
mediaElement.currentTime = position;
mediaElement.addEventListener("play", () => this.props.doPlay());
mediaElement.addEventListener("pause", () => this.props.doPause());
mediaElement.addEventListener("timeupdate", () =>
this.props.savePosition(mediaId, mediaElement.currentTime)
);
mediaElement.addEventListener("click", this.togglePlayListener);
mediaElement.addEventListener(
"loadedmetadata",
Expand All @@ -93,6 +101,7 @@ class VideoPlayer extends React.PureComponent {
if (mediaElement) {
mediaElement.removeEventListener("click", this.togglePlayListener);
}
this.props.doPause();
}

renderAudio(container, autoplay) {
Expand Down
18 changes: 14 additions & 4 deletions src/renderer/component/video/view.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,11 @@ class Video extends React.PureComponent {
changeVolume,
volume,
uri,
videoPause,
setVideoPause,
doPlay,
doPause,
savePosition,
mediaPaused,
mediaPosition,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

} = this.props;

const isPlaying = playingUri === uri;
Expand Down Expand Up @@ -93,6 +96,9 @@ class Video extends React.PureComponent {
}
const poster = metadata.thumbnail;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

console.log("mediaId:", mediaId);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove when ready to merge :)


return (
<div
className={klasses.join(" ")}
Expand All @@ -112,8 +118,12 @@ class Video extends React.PureComponent {
downloadCompleted={fileInfo.completed}
changeVolume={changeVolume}
volume={volume}
videoPause={videoPause}
setVideoPause={setVideoPause}
doPlay={doPlay}
doPause={doPause}
savePosition={savePosition}
mediaId={mediaId}
paused={mediaPaused}
position={mediaPosition}
/>
))}
{!isPlaying && (
Expand Down
5 changes: 5 additions & 0 deletions src/renderer/constants/action_types.js
Original file line number Diff line number Diff line change
Expand Up @@ -167,3 +167,8 @@ export const HAS_FETCHED_SUBSCRIPTIONS = "HAS_FETCHED_SUBSCRIPTIONS";

// Video controls
export const SET_VIDEO_PAUSE = "SET_VIDEO_PAUSE";

// Media controls
export const MEDIA_PLAY = "MEDIA_PLAY";
export const MEDIA_PAUSE = "MEDIA_PAUSE";
export const MEDIA_POSITION = "MEDIA_POSITION";
23 changes: 23 additions & 0 deletions src/renderer/redux/actions/media.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
// @flow
import * as actions from "constants/action_types";
import type { Action, Dispatch } from "redux/reducers/media";
import lbry from "lbry";

export const doPlay = () => (dispatch: Dispatch) =>
dispatch({
type: actions.MEDIA_PLAY,
});

export const doPause = () => (dispatch: Dispatch) =>
dispatch({
type: actions.MEDIA_PAUSE,
});

export const savePosition = (id: String, position: String) => (
dispatch: Dispatch
) =>
dispatch({
type: actions.MEDIA_POSITION,
id,
position,
});
39 changes: 39 additions & 0 deletions src/renderer/redux/reducers/media.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
// @flow
import * as actions from "constants/action_types";
import { handleActions } from "util/redux-utils";

export type MediaState = {
paused: Boolean,
positions: Object,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

};

export type Action = any;
export type Dispatch = (action: Action) => any;

const defaultState = { paused: true, positions: {} };

export default handleActions(
{
[actions.MEDIA_PLAY]: (state: MediaState, action: Action) => ({
...state,
paused: false,
}),

[actions.MEDIA_PAUSE]: (state: MediaState, action: Action) => ({
...state,
paused: true,
}),

[actions.MEDIA_POSITION]: (state: MediaState, action: Action) => {
const { id, position } = action;
return {
...state,
positions: {
...state.positions,
[id]: position,
},
};
},
},
defaultState
);
15 changes: 15 additions & 0 deletions src/renderer/redux/selectors/media.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import * as settings from "constants/settings";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

import { createSelector } from "reselect";

const _selectState = state => state.media || {};

export const selectMediaPaused = createSelector(
_selectState,
state => state.paused
);

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return state.positions[id] || null;
});
2 changes: 2 additions & 0 deletions src/renderer/store.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import walletReducer from "redux/reducers/wallet";
import shapeShiftReducer from "redux/reducers/shape_shift";
import subscriptionsReducer from "redux/reducers/subscriptions";
import videoReducer from "redux/reducers/video";
import mediaReducer from "redux/reducers/media";
import { persistStore, autoRehydrate } from "redux-persist";
import createCompressor from "redux-persist-transform-compress";
import createFilter from "redux-persist-transform-filter";
Expand Down Expand Up @@ -71,6 +72,7 @@ const reducers = combineReducers({
shapeShift: shapeShiftReducer,
subscriptions: subscriptionsReducer,
video: videoReducer,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we still need video: videoReducer,?

media: mediaReducer,
});

const bulkThunk = createBulkThunkMiddleware();
Expand Down