feat(annotations): Handle annotations staged change event #1247

mxiao6 · 2020-09-03T22:51:57Z

Annotations PR: feat(eventing): Emit new annotations_staged_change event box-annotations#570
Unit tests

src/lib/AnnotationControls.ts

src/lib/viewers/BaseViewer.js

src/lib/AnnotationControls.ts

src/lib/AnnotationFSM.ts

src/lib/AnnotationControlsFSM.ts

src/lib/viewers/BaseViewer.js

ConradJChan · 2020-09-09T16:48:19Z

Might be worth writing the unit tests for the fsm so that we can make sure all the transitions are covered even though this is just a draft right now

ConradJChan

Does the type still need to be passed into the transition call here: 6118793#diff-c7f4ce178012f15dc209092ae0c317f9R1269?

ConradJChan · 2020-09-09T17:12:47Z

src/lib/AnnotationControlsFSM.ts

@@ -33,7 +39,7 @@ export const stateModeMap = {
 export default class AnnotationControlsFSM {
    private currentState = AnnotationState.NONE;

-    public transition = (input: AnnotationInput, mode: AnnotationMode): AnnotationMode => {
+    public transition = (input: AnnotationInput, mode: AnnotationMode = AnnotationMode.NONE): AnnotationMode => {


Having mode default to none seems ripe for bugs to be introduced. Would it work if it was just optionally undefined?

The state in FSM cannot be undefined, so if mode is undefined and state transition depends on mode, the state has to be none which is the same as default mode to none.

mxiao6 · 2020-09-09T18:17:40Z

Does the type still need to be passed into the transition call here: 6118793#diff-c7f4ce178012f15dc209092ae0c317f9R1269?

type is needed to determine next state is HIGHLIGHT_TEMP or REGION_TEMP.

src/lib/AnnotationControlsFSM.ts

src/lib/__tests__/AnnotationControlsFSM-test.js

ConradJChan · 2020-09-09T19:23:41Z

src/lib/__tests__/AnnotationControlsFSM-test.js

+            state: AnnotationState.NONE,
+            input: AnnotationInput.CLICK,
+            mode: AnnotationMode.HIGHLIGHT,
+            nextState: AnnotationState.HIGHLIGHT,


Does nextState and output ever differ?

nextState could be highlight_temp and output is highlight

src/lib/__tests__/AnnotationControlsFSM-test.js

mxiao6 added the do-not-merge label Sep 3, 2020

mxiao6 force-pushed the staged-change-event branch 2 times, most recently from 099951d to 553ff81 Compare September 4, 2020 00:25

jstoffan reviewed Sep 4, 2020

View reviewed changes

src/lib/AnnotationControls.ts Outdated Show resolved Hide resolved

src/lib/AnnotationControls.ts Outdated Show resolved Hide resolved

src/lib/viewers/BaseViewer.js Outdated Show resolved Hide resolved

mxiao6 force-pushed the staged-change-event branch 3 times, most recently from 89d0a72 to 4d0e9a6 Compare September 8, 2020 20:56

mxiao6 removed the do-not-merge label Sep 8, 2020

mxiao6 force-pushed the staged-change-event branch from 4d0e9a6 to 8b266be Compare September 8, 2020 22:02

feat(annotations): Handle annotations staged change event

7f8a098

mxiao6 force-pushed the staged-change-event branch from 8b266be to 7f8a098 Compare September 9, 2020 00:14

mxiao6 requested a review from ConradJChan September 9, 2020 00:27