Fix: Fix position of popover on drawing undo/redo (#343)

src/drawing/DrawingThread.js

@@ -241,51 +241,49 @@ class DrawingThread extends AnnotationThread {
     }
 
     /**
-     * Overturns the last drawing stroke if it exists. Emits thenumber of undo and redo
+     * Overturns the last drawing stroke if it exists. Emits the number of undo and redo
      * actions available if an undo was executed.
      *
      * @return {void}
      */
     undo() {
         const executedUndo = this.pathContainer.undo();
         if (executedUndo) {
-            this.draw(this.drawingContext, true);
-            this.updateBoundary();
-            this.regenerateBoundary();
-
-            if (this.pathContainer.isEmpty()) {
-                this.unmountPopover();
-            } else {
-                this.renderAnnotationPopover();
-            }
-
-            this.drawBoundary();
-            this.emitAvailableActions();
+            this.updateBoundaryAndPopover();
         }
     }
 
     /**
-     * Replays the last undone drawing stroke if it exists. Emits thenumber of undo and redo
+     * Replays the last undone drawing stroke if it exists. Emits the number of undo and redo
      * actions available if a redraw was executed.
      *
      * @return {void}
      */
     redo() {
         const executedRedo = this.pathContainer.redo();
         if (executedRedo) {
-            this.draw(this.drawingContext, true);
-            this.updateBoundary();
-            this.regenerateBoundary();
+            this.updateBoundaryAndPopover();
+        }
+    }
 
-            if (this.pathContainer.isEmpty()) {
-                this.unmountPopover();
-            } else {
-                this.renderAnnotationPopover();
-            }
+    /**
+     * Updates the position of the drawing boundary and popover after an undo/redo event.
+s    *
+     * @return {void}
+     */
+    updateBoundaryAndPopover() {
+        this.draw(this.drawingContext, true);
+        this.updateBoundary();
+        this.regenerateBoundary();
+
+        this.unmountPopover();
 
-            this.drawBoundary();
-            this.emitAvailableActions();
+        if (!this.pathContainer.isEmpty()) {
+            this.renderAnnotationPopover();
         }
+
+        this.drawBoundary();
+        this.emitAvailableActions();
     }
 
     /**
@@ -309,7 +307,7 @@ class DrawingThread extends AnnotationThread {
             return;
         }
 
-        /* OPTIMIZE (@minhnguyen): Render only what has been obstructed by the new drawing
+        /* OPTIMIZE: Render only what has been obstructed by the new drawing
          *           rather than every single line in the thread. If we do end
          *           up splitting saves into multiple requests, we can buffer
          *           the amount of re-renders onto a temporary memory canvas.
@@ -329,7 +327,7 @@ class DrawingThread extends AnnotationThread {
     }
 
     /**
-     * Emit an event containing thenumber of undo and redo actions that can be done.
+     * Emit an event containing the number of undo and redo actions that can be done.
      *
      * @return {void}
      */

src/drawing/__tests__/DrawingThread-test.js

@@ -192,77 +192,72 @@ describe('drawing/DrawingThread', () => {
 
     describe('undo()', () => {
         beforeEach(() => {
-            thread.draw = jest.fn();
-            thread.updateBoundary = jest.fn();
-            thread.regenerateBoundary = jest.fn();
-            thread.drawBoundary = jest.fn();
-            thread.emitAvailableActions = jest.fn();
-            thread.unmountPopover = jest.fn();
-            thread.renderAnnotationPopover = jest.fn();
-            thread.pathContainer = {
-                isEmpty: jest.fn(),
-                undo: jest.fn().mockReturnValue(false)
-            };
+            thread.updateBoundaryAndPopover = jest.fn();
+            thread.pathContainer.undo = jest.fn();
         });
 
         it('should do nothing when the path container fails to undo', () => {
+            thread.pathContainer.undo.mockReturnValue(false);
+
             thread.undo();
-            expect(thread.pathContainer.undo).toBeCalled();
-            expect(thread.draw).not.toBeCalled();
-            expect(thread.emitAvailableActions).not.toBeCalled();
-            expect(thread.updateBoundary).not.toBeCalled();
-            expect(thread.regenerateBoundary).not.toBeCalled();
-            expect(thread.drawBoundary).not.toBeCalled();
+
+            expect(thread.updateBoundaryAndPopover).not.toBeCalled();
         });
 
-        it('should draw when the path container indicates a successful undo', () => {
-            thread.pathContainer.undo = jest.fn().mockReturnValue(true);
-            thread.undo();
-            expect(thread.pathContainer.undo).toBeCalled();
-            expect(thread.draw).toBeCalled();
-            expect(thread.emitAvailableActions).toBeCalled();
-            expect(thread.updateBoundary).toBeCalled();
-            expect(thread.regenerateBoundary).toBeCalled();
-            expect(thread.drawBoundary).toBeCalled();
-            expect(thread.renderAnnotationPopover).toBeCalled();
+        it('should update the boundary and popover on a successful undo', () => {
+            thread.pathContainer.undo.mockReturnValue(true);
 
-            thread.pathContainer.isEmpty = jest.fn().mockReturnValue(true);
             thread.undo();
-            expect(thread.unmountPopover).toBeCalled();
+
+            expect(thread.updateBoundaryAndPopover).toBeCalled();
         });
     });
 
     describe('redo()', () => {
         beforeEach(() => {
+            thread.updateBoundaryAndPopover = jest.fn();
+            thread.pathContainer.redo = jest.fn();
+        });
+
+        it('should do nothing when the path container fails to redo', () => {
+            thread.pathContainer.redo.mockReturnValue(false);
+
+            thread.redo();
+
+            expect(thread.updateBoundaryAndPopover).not.toBeCalled();
+        });
+
+        it('should update the boundary and popover on a successful redo', () => {
+            thread.pathContainer.redo.mockReturnValue(true);
+
+            thread.redo();
+
+            expect(thread.updateBoundaryAndPopover).toBeCalled();
+        });
+    });
+
+    describe('updateBoundaryAndPopover()', () => {
+        it('should draw, update the boundary, and update the popover', () => {
             thread.draw = jest.fn();
             thread.emitAvailableActions = jest.fn();
             thread.unmountPopover = jest.fn();
             thread.renderAnnotationPopover = jest.fn();
             thread.pathContainer = {
-                isEmpty: jest.fn(),
-                redo: jest.fn().mockReturnValue(false),
+                isEmpty: jest.fn().mockReturnValue(true),
                 getAxisAlignedBoundingBox: jest.fn()
             };
-        });
 
-        it('should do nothing when the path container fails to redo', () => {
-            thread.redo();
-            expect(thread.pathContainer.redo).toBeCalled();
-            expect(thread.draw).not.toBeCalled();
-            expect(thread.emitAvailableActions).not.toBeCalled();
-        });
+            thread.updateBoundaryAndPopover();
 
-        it('should draw when the path container indicates a successful redo', () => {
-            thread.pathContainer.redo = jest.fn().mockReturnValue(true);
-            thread.redo();
-            expect(thread.pathContainer.redo).toBeCalled();
+            expect(thread.unmountPopover).toBeCalled();
             expect(thread.draw).toBeCalled();
             expect(thread.emitAvailableActions).toBeCalled();
-            expect(thread.renderAnnotationPopover).toBeCalled();
+            expect(thread.renderAnnotationPopover).not.toBeCalled();
 
-            thread.pathContainer.isEmpty = jest.fn().mockReturnValue(true);
-            thread.redo();
-            expect(thread.unmountPopover).toBeCalled();
+            thread.pathContainer.isEmpty.mockReturnValue(false);
+
+            thread.updateBoundaryAndPopover();
+            expect(thread.renderAnnotationPopover).toBeCalled();
         });
     });