track and manage video state #890

2017-12-21T22:18:42+01:00

daovist commented

2017-12-21 22:18:42 +01:00

(Migrated from github.com)

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

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](https://user-images.githubusercontent.com/34498824/34275187-1e6023ba-e66a-11e7-96f7-1281ef055b8e.jpg)

neb-b (Migrated from github.com) reviewed 2017-12-21 22:18:42 +01:00

liamcardenas (Migrated from github.com) requested changes 2017-12-21 22:35:44 +01:00

liamcardenas (Migrated from github.com) 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);

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 commented

2017-12-22 00:22:29 +01:00

(Migrated from github.com)

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

liamcardenas commented

2017-12-22 20:22:35 +01:00

(Migrated from github.com)

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

liamcardenas (Migrated from github.com) requested changes 2017-12-27 18:33:54 +01:00

liamcardenas (Migrated from github.com) 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.

src/renderer/component/video/index.js

					
				@ -6,2 +1,2 @@

				import { makeSelectMetadataForUri, makeSelectContentTypeForUri } from 'redux/selectors/claims';

				import { setVideoPause } from 'redux/actions/video';

				import React from "react";

				import { connect } from "react-redux";

liamcardenas (Migrated from github.com) commented

2017-12-27 18:27:16 +01:00

please remove this comment when ready to merge :)

src/renderer/component/video/index.js

					
				@ -15,3 +15,1 @@

				import { selectVideoPause } from 'redux/selectors/video';

				import Video from './view';

				import { selectPlayingUri } from 'redux/selectors/content';

				} from "redux/selectors/file_info";

liamcardenas (Migrated from github.com) commented

2017-12-27 18:27:01 +01:00

please remove this comment when ready to merge

src/renderer/component/video/index.js

liamcardenas (Migrated from github.com) commented

2017-12-27 18:26:40 +01:00

please remove this comment when ready to merge

src/renderer/component/video/view.jsx

liamcardenas (Migrated from github.com) commented

2017-12-27 18:24:11 +01:00

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

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

liamcardenas (Migrated from github.com) commented

2017-12-27 18:24:26 +01:00

please remove when ready to merge :)

src/renderer/redux/reducers/media.js

liamcardenas (Migrated from github.com) commented

2017-12-27 18:23:14 +01:00

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

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

can you define a type more specifically than `Object`? Maybe `{ [string]: number }`? https://flow.org/en/docs/types/objects/#toc-objects-as-maps

src/renderer/redux/selectors/media.js

liamcardenas (Migrated from github.com) commented

2017-12-27 18:19:06 +01:00

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

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

src/renderer/redux/selectors/media.js

					
				@ -0,0 +1,17 @@

				import * as settings from "constants/settings";

liamcardenas (Migrated from github.com) commented

2017-12-27 18:17:28 +01:00

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

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

src/renderer/store.js

					
				@ -68,3 +67,4 @@

				  media: mediaReducer,

				});

				const bulkThunk = createBulkThunkMiddleware();

liamcardenas (Migrated from github.com) commented

2017-12-27 18:18:00 +01:00

do we still need video: videoReducer,?

do we still need `video: videoReducer,`?

kauffj commented

2017-12-29 17:13:16 +01:00

(Migrated from github.com)

@daovist outpoint not claimid a8988d9

@daovist outpoint not claimid a8988d9

kauffj (Migrated from github.com) requested changes 2018-01-05 20:50:11 +01:00

kauffj (Migrated from github.com) 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.

src/renderer/component/video/index.js

kauffj (Migrated from github.com) commented

2018-01-05 20:47:16 +01:00

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

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

src/renderer/component/video/view.jsx

kauffj (Migrated from github.com) commented

2018-01-05 20:49:02 +01:00

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.

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.

daovist (Migrated from github.com) reviewed 2018-01-05 21:18:23 +01:00

src/renderer/component/video/view.jsx

daovist (Migrated from github.com) commented

2018-01-05 21:18:23 +01:00

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?

liamcardenas (Migrated from github.com) requested changes 2018-01-06 00:28:02 +01:00

liamcardenas (Migrated from github.com) left a comment

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

src/renderer/component/video/internal/player.jsx

liamcardenas (Migrated from github.com) commented

2018-01-06 00:25:36 +01:00

remove comment please

hackrush01 (Migrated from github.com) reviewed 2018-01-06 04:10:59 +01:00

src/renderer/component/video/view.jsx

hackrush01 (Migrated from github.com) commented

2018-01-06 04:10:58 +01:00

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?

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?

Sign in to join this conversation.

No reviewers

neb-b

kauffj

liamcardenas