Fix for PR 5754

[goplayoutside3]

See context for these changes in PR #5754's discussion, and particularly my comment here.

[kieftrav]

@goplayoutside3 - just tested out your PR and the bug I outlined is still there. Please see the attached video about how previous marks on different frames don't get pulled in the way I assume they should.

PrevMarks.Bug.mp4

[goplayoutside3]

Ah okay I see what's happening. The video is related to your question about previousInteractionTaskAnnotations() here. In the subject viewer, there are two different ways marks are grabbed from the store:

1. InteractionLayerContainer

InteractionLayerContainer gets its marks from classifierStore.workflowSteps.active.tasks. "active" is a Step type in this case, and tasks is an array of type DrawingTask. get marks() is a "view" on the DrawingTask model that loops over self.tools and returns all tool.marks. For example, this is relevant for a drawing task that requires a volunteer to draw one circle and one point. tool.marks gets updated when createMark() from the Tool type is called in a UI component. Therefore, when a volunteer clicks on the InteractionLayer, that interaction calls activeTool.createMark() with all the necessary data to add a new mark to tool.marks in the store.

Because InteractionLayerContainer is observing the store, it re-renders whenever tool.marks updates. Also, its child component InteractionLayer handles filtering per frame and renders the marks in DrawingToolMarks.

2. PreviousMarks

PreviousMarks gets its marks by calling previousInteractionTaskAnnotations() which grabs annotations from classifierStore.classification.annotations. That function also filters for annotations that do not match the active tasks's key (e.g T1). PreviousMarks observes the store too, so it should re-render whenever previousAnnotations updates, and handle filtering of those annotation marks by frame. There's a DrawingToolMarks component rendered in PreviousMarks for every annotation intentionally passing disabled as true.

You can inspect annotations by calling self.toSnapshot() inside previousInteractionTaskAnnotations(). While debugging, I found that annotations in PreviousMarks did contain expected data when going back and forth between workflow steps. So that narrows down the bug to PreviousMarks. I think that this line:

front-end-monorepo/packages/lib-classifier/src/components/Classifier/components/SubjectViewer/components/InteractionLayer/components/PreviousMarks/PreviousMarks.js

Line 49 in 6b421ed

const annotationValuesPerFrame = annotation.value.filter(value => value.frame === frame)

does not actually have access to frame on each Mark without more explicit handling of store snapshots. See the use of resolveIdentifier in DrawingAnnotation:

front-end-monorepo/packages/lib-classifier/src/plugins/tasks/drawing/models/DrawingAnnotation.js

Line 17 in 6b421ed

const actualTask = resolveIdentifier(DrawingTask, getRoot(self), self.task)

and my logging of the annotation snapshot in my latest commit.

To summarize, the reason InteractionLayerContainer and PreviousMarks grab annotation marks from the store in two different ways is because PreviousMarks is limited to only rendering marks created or edited by a volunteer and also needs to grab annotations from other steps. InteractionLayerContainer only cares about the active step.

I can't spend more time on this today, but will update if I think of a way to address this problem later this week.

[goplayoutside3]

This shows correctly stored marks while jumping between workflow steps. The issue is that PreviousMarks isn't actually filtering per frame yet.

[eatyourgreens]

Here, you should use getSnapshot(annotation) to debug the annotation. That's how I figured out that the annotation value didn't include all of the drawn marks.

annotation.toSnapshot() dereferences a drawing task's marks so that they can be saved to Panoptes. It doesn't give the same value as annotation.value (but #5923 should fix that by making sure that annotation.value always reflects the current value of task.marks.)

[goplayoutside3]

@kieftrav noting here that the bug in your video exists on production too. It can be replicated in the How Did We Get Here project that has multi-frame transcription subjects and two steps in the workflow.

[goplayoutside3]

I've asked Jim for input here, too.

[kieftrav]

@goplayoutside3 - I've incorporated Jim's one-liner into my PR. I don't think all of the other changes here make sense - they're focused on changing the frame on the container element instead of focusing on the mark itself having frame=0. Because modifying the frame is a data export issue decided in Issue #5493 and is confined to the mark itself, I think it's important to keep the comment about the GitHub issue next to the area that sets its value to 0 instead of creating a chain of assumptions about why we changed the frame itself to 0 for the whole InteractionLayer when all that matters is that the classification export registers the mark's frame as 0 upon creation. I think this is particularly poignant because allowing the frame number to be set to its real frame when multiImageCloneMarkers is true doesn't change behavior at all - it still works as expected cloning all marks across all frames.

Because we resolved the issue with losing track of marks in the PreviousMarks component (i.e. Jim's fix) the current PR works as expected as far as I can tell.