Classifier: Drawing Task's base "Mark" may need default .initialDrag() et al

[shaunanoordin]

Classifier Functionality Bug

Package: lib-classifier Affects: Drawing Tasks

The Drawing Task has a base "Drawing Mark" code (drawingTools/models/marks/Mark/Mark.js) that is extended into different drawing marks, e.g. Circle Mark, Polygon Mark, etc.

There are a few bits of code that assume EVERY type of Mark has certain default methods, but problem is, this isn't true in every case. Some "default methods" are actually only available in the extended Marks (like Circle Marks) but not the default Mark.

For example, in the base "Drawing Tool" code (drawingTools/models/tools/Tool/Tool.js), there's this code...

handlePointerMove(event, mark) {
  mark.initialDrag(event)
},

...which causes problems when an extended Mark doesn't explicitly include the missing "default" initialDrag(). (e.g., the Polygon Mark)

Context: this bug was discovered by me and @ErikOstlund alongside #2959 . This problem probably would have gone undetected if we didn't stumble into 2959's extremely niche trigger condition which allowed us to have mismatched Marks and Tools, so good news is that this problem probably isn't being experienced by any actual users at the moment.

Dev Notes

Suggested solution is simple: add an empty initialDrag(event) { /* Do nothing */ } to Mark.js

I'm not sure if the following things also need defaults in the base Mark.js code, but I suspect the answer is probably yes for two of them:

• mark.initialPosition() - used in InteractionLayer
• mark.move() - used in InteractionLayer
• mark.setCoordinates() - ehhh? Not sure if this should apply to all marks.

Status

Low priority. The niche trigger conditions means this only affects FEM projects that use the Polygon Tool, but it's good practice anyway to have default fallbacks since the code already assumes those defaults exist.

[eatyourgreens]

The drawing Readme already says that all marks should implement initialDrag. https://github.com/zooniverse/front-end-monorepo/tree/master/packages/lib-classifier/src/plugins/drawingTools#mark-models

[eatyourgreens]

Absolutely fine for that to do absolutely nothing, by the way.

[eatyourgreens]

The code could also be changed to make initialDrag optional (#2961):

mark.initialDrag?.(event)

[eatyourgreens]

I think I remember what initialDrag handles: you click to place your first point, realise you've clicked in the wrong place, move the pointer and then release it to place your first point in the correct location. TranscriptionLine and Line probably have implementations of this.

[lcjohnso]

@eatyourgreens Looks like this is resolved -- can you confirm and close?

[eatyourgreens]

@shaunanoordin did we fix this?

[eatyourgreens]

Parts of this were addressed by #3389 (mark.setCoordinates for polygons) and #3393 (mark.setCoordinates for freehand lines.) #3424 fixes mark.finish across all drawing tools and #3431 should remove all implicit, default behaviour completely.

[eatyourgreens]

Low priority. The niche trigger conditions means this only affects FEM projects that use the Polygon Tool, but it's good practice anyway to have default fallbacks since the code already assumes those defaults exist.

@shaunanoordin types.compose merges models using object.assign. Here the array of types are reduced down to a single new type: https://github.com/mobxjs/mobx-state-tree/blob/7b52c9cafe1a3473ee867c5f21a46c454b1c05cd/packages/mobx-state-tree/src/types/complex-types/model.ts#L817-L829

using the cloneAndEnhance method here: https://github.com/mobxjs/mobx-state-tree/blob/af93859502d039a6664f59bac4b2ed1c25b1775f/packages/mobx-state-tree/src/types/complex-types/model.ts#L369-L377

I didn't want empty, default actions to be accidentally composed into marks, which is why Mark doesn't define required actions. You must define them in any new mark models that you write.