Implement an "undo"-function for drwaing about annotorious-openseadragon HOT 9 OPEN

I wrote this function in RubberbandPolygon.js , which does already most of the job.
delLastPoint = () => { this.points.pop(); }
However, I am not sure, where to put best an eventlistener. I tryed to implement a keypress-event in RubberbandPolygonTool.js, yet this is triggered multiple times after pressing one time.
For now, I implemented an KeyListener in the ClickEvent:

onMouseUp = evt => { if (evt.ctrlKey) { this.rubberband.delLastPoint(); } else{ if (this.rubberband.isCollapsed) { this.emit('cancel', evt); this.stop(); } else { const { x , y } = this._toSVG(evt.layerX, evt.layerY); this.rubberband.addPoint([ x, y ]); } } }
But I am not happy with this solution. What do you think about it?

from annotorious-openseadragon.

To prevent ending up without any points, it might be a good idea to add this line:
delLastPoint = () => { if (this.points.length>2){ this.points.pop(); } }

from annotorious-openseadragon.

If you're looking at the OpenSeadragon plugin specifically, you could attach it to the same place where the Shift key is handled:

https://github.com/recogito/annotorious-openseadragon/blob/master/src/annotations/OSDAnnotationLayer.js#L78-L81

And maybe listen to a CTRL+Z to trigger .delLastPoint()?

from annotorious-openseadragon.

works fine there!
I added:

      if (evt.keyCode == 90 && evt.ctrlKey) {
        if (this.tools.getToolName()==="polygon"){
          this.tools.current.rubberband.delLastPoint();
        }
      };

in OSDAnnotationLayer's KeydownEvent
and to the DrawingToll:

  getToolName(){
    if (this._current===null){
      return ""
    }
    else if (this._current instanceof RubberbandPolygonTool) {
      return "polygon"
    }
    else if (this._current instanceof RubberbandRectTool) {
      return "rect"
    }
    else{return "error"}
  }

because I did not know how to check, which tool is active (you don't need this for boundingboxes)
do you want me to make an official pull request?

from annotorious-openseadragon.

Great. Yes, I think this would make a nice pull request! Can I make three small suggestions:

• To keep things generic, what do you think about renaming the method from delLastPoint to, simply, undo? That way, different tools in the future could handle CTRL+Z in different ways.

• What's "below" in the hierarchy from the _current tool shouldn't matter, or be visible to the annotation layer. Therefore, I would add an undo method to the tool, even if it doesn't do anything else then delegate to the (inner) rubberband instance.

• Instead of the tool sniffing workaround, you can simply do the following. (Assuming you already added the undo method to the tool.)

if (evt.keyCode == 90 && evt.ctrlKey) {
  this.tools.current?.undo && this.tools.current.undo();
}

In this case, the code will first check whether this a current drawing tool and whether it has an undo method. And then, only if that's true, it will execute undo().

from annotorious-openseadragon.

I rewrote everything, however, there is still a problem, which I just now recognized. The tool is working, yet there are still several errors, telling, that rubberband is undefined...

from annotorious-openseadragon.

Can you post a code sample of what you have? You definitely should avoid accessing rubberband (as it's internal to the tool, so the outside shouldn't need to be aware of it). That's why I think an undo method at the level of the tool would be a safer abstraction.

from annotorious-openseadragon.

In OSDAnnotationLayer:

    document.addEventListener('keydown', evt => {
      if (evt.keyCode == 90 && evt.ctrlKey) {
        this.tools.undo();
      }

this triggers an undo-function in DrawingTools:

  undo = () =>{
    //console.log(this);
    this._current.undo();
  }

from here, we come to rubberbandpolygontool:

  undo = () =>{
    console.log("this: ",this);    
    console.log("rubberband: ",this.rubberband);
    this.rubberband.undo();
  }

or to rubberbandrectool:
undo(){}
the funtion in rubberbandpolygontool triggers:

  undo = () => {
    if (this.points.length>2){
      this.points.pop();
    }
  }

deleting one point without the rubberband is not possible as far as I understood, isn't it?
I could add a simple test, is rubberband is not null. Should already clear the errors. Anyhows. I find it a bit strange, that a keypress-event is triggered several times. Do you have an idea why?

Sincerely,
Erik

from annotorious-openseadragon.

Hm, this looks ok overall. I'd leave the undo() method out of DrawingTools and, in OSDAnnotationLayer call this.tools.current.undo() directly instead. But that's just a matter of taste. Do you have the code available in a public fork, so I could try this out myself?

Another thing: the RubberbandPolygon is only active while you are drawing. As soon as you close the shape, the RubberbandPolygon will be destroyed. Or, in other words, CTRL+Z will no longer work as soon as the editor popup has opened. Perhaps this is what's happening here?

from annotorious-openseadragon.