Classifier: Refactor Video Subject Viewer #3645

goplayoutside3 · 2022-09-13T21:16:40Z

Package

lib-classifier

Linked Issue and/or Talk Post

Closes: #3610
Closes: #3611

Important Changes

SubjectViewer

I added a static translation key for "Loading".

SingleVideoViewerConnector

I deleted this file and its spec file. SubjectViewer already passes down the necessary subject prop, so this file was redundant.

SingleVideoViewerContainer

Added its features and props documentation to the README file.
Converted this component to a functional component.
Did a lot of variables renaming for clarity.
Added a static translation key for UI if missing the subject video source.
I removed the preload() setup that was copy pasted from SingleImageViewerContainer as it's not relevant to react-player.

SingleVideoViewerContainer Stories

The storybook now has two stories:
- Successfully loaded a subject with a source string
- Missing a subject source string

Unit Tests

I added unit tests to SingleVideoViewerContainer and one test to SingleVideoViewer using React Testing Library.
Because we're using an external library react-player to handle the subject's < video > element, there's not much testing we can do for that library's native methods. For instance, I tried to listen for onReactPlayerReady() in SingleVideoViewerContainer, but without knowledge of react-player's internal onReady method, it's difficult to predict when react-player's onReady will be invoked in our testing environment.
I'll note that more tests may be added in the feature when InteractionLayer in SingleVideoViewerContainer becomes important for FEM drawing tools. We do not have a live FEM project with them yet.

How to Review

The raw github diff comparison will be fairly messy, so I recommend reading the code like this is a complete re-write of the video subject viewer. I recommend these projects for testing the video subject viewer in a browser:

As noted in the Issues linked above, this rewrite is part of an effort to improve the video subject viewer code for migrating Woodpecker Cavity Cam to FEM. There will be follow-up PR's for the VideoController components as described in #3612.

Checklist

PR Creator - Please cater the checklist to fit the review needed for your code changes.
PR Reviewer - Use the checklist during your review. Each point should be checkmarked or discussed before PR approval.

General

Tests are passing locally and on Github
Documentation is up to date and changelog has been updated if appropriate
You can yarn panic && yarn bootstrap or docker-compose up --build and FEM works as expected
FEM works in all major desktop browsers: Firefox, Chrome, Edge, Safari (Use Browserstack account as needed)
FEM works in a mobile browser

General UX

Example Staging Project: i-fancy-cats

All pages of a FEM project load: Home Page, Classify Page, and About Pages
Can submit a classification
Can sign-in and sign-out
The component is accessible
- Can be used with a screen reader BBC guide to VoiceOver
- Can be used from the keyboard WebAIM guide to keyboard testing
- It is passing accessibility checks in the storybook

Refactoring

The PR creator has described the reason for refactoring
The refactored component(s) continue to work as expected

- Remove defaultProps - Rename some variables for clarity - Add Readme info

- install @storybook/testing-react - delete SingleVideoViewerConnector

…Container

coveralls · 2022-09-13T21:24:40Z

Coverage increased (+0.3%) to 82.96% when pulling a560fb0 on single-video-viewer-upgrade into 8469d77 on master.

eatyourgreens · 2022-09-13T21:28:24Z

It might be a good idea to fold the changes from #3632 into this branch.

...assifier/components/SubjectViewer/components/SingleVideoViewer/SingleVideoViewerContainer.js

eatyourgreens · 2022-09-13T21:58:47Z

...assifier/components/SubjectViewer/components/SingleVideoViewer/SingleVideoViewerContainer.js

+      setVideoHeight(reactPlayerVideoHeight)
+      setVideoWidth(reactPlayerVideoWidth)
+
+      const { width: svgClientWidth, height: svgClientHeight } = interactionLayerSVG.current?.getBoundingClientRect()


Destructuring here will crash with an error if the ref is null or undefined. Will that be a problem when drawing is disabled and there's no SVG element?

This is also relates to your comment about if the svg layer should be rendered even when drawing tools aren't used. I was definitely was wondering the same thing. However, I think the svg layer is always rendered so that onReady() can be called with four dimensions. That function is a passed-down prop of SubjectViewerStore's onSubjectReady(), and the dimensions are saved in the store:

front-end-monorepo/packages/lib-classifier/src/store/SubjectViewerStore/SubjectViewerStore.js

Lines 96 to 107 in de3b6cc

onSubjectReady (event) {

const { target = {} } = event || {}

const {

clientHeight = 0,

clientWidth = 0,

naturalHeight = 0,

naturalWidth = 0

} = target || {}

self.dimensions.push({ clientHeight, clientWidth, naturalHeight, naturalWidth })

self.rotation = 0

self.loadingState = asyncStates.success

},

A similar pattern happens in SingleImageViewerContainer:

front-end-monorepo/packages/lib-classifier/src/components/Classifier/components/SubjectViewer/components/SingleImageViewer/SingleImageViewerContainer.js

Lines 39 to 48 in de3b6cc

useEffect(function onImageLoad () {

if (src !== placeholder.src) {

const svgImage = subjectImage.current

const { width: clientWidth, height: clientHeight } = svgImage

? svgImage.getBoundingClientRect()

: {}

const target = { clientHeight, clientWidth, naturalHeight, naturalWidth }

onReady({ target })

}

}, [src])

Subject images are always rendered as SVG, even if there's no drawing in the workflow, so that makes sense. For video, I'd have expected the client height and width to come from the video player. If the SVG does have to be rendered, and there's no drawing involved, I'd recommend styling it with pointer-events: none; so that it doesn't interfere with clicking on the video. I think that, at the moment, it's always positioned over the visible video and will capture the pointer.

I guess the advantage of doing it this way is that the height and width of the player would include the controls, whereas the SVG seems to measure just the visible picture.

Agreed with styling the pointer events. I think the height and width you mentioned actually exclude the controls because as of now, there are custom VideoController components rather than react-player's built-in controls.

Do you know why the "client" and "natural" dimensions are saved in SubjectViewerStore? There's no mention of it in the store README's.

I think I can refactor onReactPlayerReady() to use client dimensions of its < video > element instead of relying on an SVG overlay being present. I'll work on that and let you know when ready for a new review!

I refactored, and the drawing layers only render when enableInteractionLayer is true. It doesn't look like the passing of the enableInteractionLayer prop is built into the classifier yet, so I've set it as false while we're focused on Woodpecker Cavity Cam. onReady() is still called with the target expected in SubjectViewerStore.

Client height and width are sent back with each classification. I don’t know if anyone has ever used them in their data analysis.

enableInteractionLayer is built into the classifier. At least, it's used in transcription projects. I'm not 100% sure where it gets passed down. It might be one of the props that's passed in context.

...assifier/components/SubjectViewer/components/SingleVideoViewer/SingleVideoViewerContainer.js

…s true

goplayoutside3 added 9 commits September 12, 2022 16:05

Prepare SingleVideoViewer for adding unit tests

838c5eb

- Remove defaultProps - Rename some variables for clarity - Add Readme info

clean up a bit

f2e15e5

create default storybook for SingleVideoViewerContainer

5ad95ce

working toward handling loading states

2f61ab3

- install @storybook/testing-react - delete SingleVideoViewerConnector

more renaming for clarity

6d0d984

refactor SingleVideoViewerContainer as a functional component

0f1feb1

add translation to SubjectViewer and unit tests for SingleVideoViewer…

d49c09e

…Container

add SingleVideoViewer unit test

6735ede

clean up SingleVideoViewerContainer's readme

7be841c

goplayoutside3 requested a review from a team September 13, 2022 21:16

goplayoutside3 added refactor Refactoring existing code storybook Related to storybook component stories tests Related to unit tests, integration tests, etc labels Sep 13, 2022

eatyourgreens reviewed Sep 13, 2022

View reviewed changes

...assifier/components/SubjectViewer/components/SingleVideoViewer/SingleVideoViewerContainer.js Outdated Show resolved Hide resolved

eatyourgreens reviewed Sep 13, 2022

View reviewed changes

...assifier/components/SubjectViewer/components/SingleVideoViewer/SingleVideoViewerContainer.js Outdated Show resolved Hide resolved

goplayoutside3 added 2 commits September 15, 2022 12:34

use ternary for videoSrc in SingleVideoViewerContainer

9458a80

only render the drawing tools interaction layers when enableDrawing i…

36c692a

…s true

goplayoutside3 requested a review from eatyourgreens September 16, 2022 13:42

goplayoutside3 mentioned this pull request Sep 20, 2022

Classifier: Custom Video Controls #3684

Merged

12 tasks

goplayoutside3 added the video subject Issue related to the video subject viewer label Sep 20, 2022

goplayoutside3 and others added 2 commits September 20, 2022 17:49

Merge branch 'master' into single-video-viewer-upgrade

50d9211

Merge branch 'master' into single-video-viewer-upgrade

a560fb0

eatyourgreens approved these changes Sep 21, 2022

View reviewed changes

github-actions bot added the approved This PR is approved for merging label Sep 21, 2022

eatyourgreens merged commit 8266096 into master Sep 21, 2022

eatyourgreens deleted the single-video-viewer-upgrade branch September 21, 2022 13:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Classifier: Refactor Video Subject Viewer #3645

Classifier: Refactor Video Subject Viewer #3645

goplayoutside3 commented Sep 13, 2022

coveralls commented Sep 13, 2022 •

edited

Loading

eatyourgreens commented Sep 13, 2022

eatyourgreens Sep 13, 2022

goplayoutside3 Sep 15, 2022

eatyourgreens Sep 15, 2022

eatyourgreens Sep 15, 2022

goplayoutside3 Sep 15, 2022

goplayoutside3 Sep 15, 2022

goplayoutside3 Sep 15, 2022

eatyourgreens Sep 15, 2022

eatyourgreens Sep 15, 2022

eatyourgreens Sep 21, 2022 •

edited

Loading

	onSubjectReady (event) {
	const { target = {} } = event \|\| {}
	const {
	clientHeight = 0,
	clientWidth = 0,
	naturalHeight = 0,
	naturalWidth = 0
	} = target \|\| {}
	self.dimensions.push({ clientHeight, clientWidth, naturalHeight, naturalWidth })
	self.rotation = 0
	self.loadingState = asyncStates.success
	},

	useEffect(function onImageLoad () {
	if (src !== placeholder.src) {
	const svgImage = subjectImage.current
	const { width: clientWidth, height: clientHeight } = svgImage
	? svgImage.getBoundingClientRect()
	: {}
	const target = { clientHeight, clientWidth, naturalHeight, naturalWidth }
	onReady({ target })
	}
	}, [src])

Classifier: Refactor Video Subject Viewer #3645

Classifier: Refactor Video Subject Viewer #3645

Conversation

goplayoutside3 commented Sep 13, 2022

Package

Linked Issue and/or Talk Post

Important Changes

SubjectViewer

SingleVideoViewerConnector

SingleVideoViewerContainer

SingleVideoViewerContainer Stories

Unit Tests

How to Review

Checklist

General

General UX

Refactoring

coveralls commented Sep 13, 2022 • edited Loading

eatyourgreens commented Sep 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eatyourgreens Sep 21, 2022 • edited Loading

Choose a reason for hiding this comment

coveralls commented Sep 13, 2022 •

edited

Loading

eatyourgreens Sep 21, 2022 •

edited

Loading