Unexpected behavior of command history unit

Question

I wrote such module to store there last changes of picture in my paint application " in Delphi

    unit HistoryQueue;

    interface

    uses
      Graphics;

   type
myHistory = class
  constructor Create(Size:Integer);
  public
    procedure Push(Bmp:TBitmap);
    function Pop():TBitmap;
    procedure Clean();
    procedure Offset();
    function isEmpty():boolean;
    function isFull():boolean;
    function getLast():TBitmap;
  protected
    historyQueueArray: array of TBitmap;
    historyIndex, hSize:Integer;
  end;

implementation

procedure myHistory.Push(Bmp:TBitmap);
var tbmp:TBitmap;
begin
  if(not isFull) then begin
      Inc(historyIndex);
      historyQueueArray[historyIndex]:=TBitmap.Create;
      historyQueueArray[historyIndex].Assign(bmp);
  end else begin
      Offset();
      historyQueueArray[historyIndex]:=TBitmap.Create;
      historyQueueArray[historyIndex].Assign(bmp);
  end;

end;

procedure myHistory.Clean;
var i:Integer;
begin
{  for i:=0 to hSize do begin
    historyQueueArray[i].Free;
    historyQueueArray[i].Destroy;
  end;        }

end;

constructor myHistory.Create(Size:Integer);
begin
  hSize:=Size;
  SetLength(historyQueueArray, hSize);
  historyIndex:=-1;
end;

function myHistory.isEmpty: boolean;
begin
  Result:=(historyIndex = -1);
end;

function myHistory.isFull: boolean;
begin
  Result:=(historyIndex = hSize);
end;

procedure myHistory.Offset; {to handle overflow}
var i:integer;
begin
  //historyQueueArray[0]:=nil;
  for i:=0 to hSize-1 do begin
    historyQueueArray[i]:=TBitmap.Create;
    historyQueueArray[i].Assign(historyQueueArray[i+1]);
  end;
end;

function myHistory.Pop: TBitmap;
var
  popBmp:TBitmap;
begin
  popBmp:= TBitmap.Create;
  popBmp.Assign(historyQueueArray[historyIndex]);
  Dec(historyIndex);
  Result:=popBmp;
end;

function myHistory.getLast: TBitmap; {this function I use when I need refresh the cnvas when I draw ellipse or rect, to get rid of traces and safe previous changes of the picture}
var
  tBmp:TBitmap;
begin
  tBmp:= TBitmap.Create;
  tBmp.Assign(historyQueueArray[historyIndex]);
  Result:=tBmp;
end;

end.

And thats how I use it

procedure TMainForm.FormCreate(Sender: TObject);
var
  cleanBmp:TBitmap;
begin
    {...}

    doneRedo:=false;

    redomode:=false; undomode:=false;
//init arrays
    picHistory:=myHistory.Create(10);   //FOR UNDO
    tempHistory:=myHistory.Create(10); //FOR REDO

    cleanbmp:=TBitmap.Create;
    cleanbmp.Assign(imgMain.Picture.Bitmap);
    picHistory.Push(cleanbmp);
    cleanbmp.Free;

    {...}

end;

 procedure TMainForm.btnUndoClick(Sender: TObject);
var redBmp:TBitmap;
begin

  undoMode:=true;
  //if there were some changes
  if(not picHistory.isEmpty) then begin
    redBmp:=TBitmap.Create;
    redBmp.Assign(picHistory.getLast);
    //clean canvas
    imgMain.Picture.Bitmap:=nil; 
    //get what was there before
    imgMain.Canvas.Draw(0,0, picHistory.Pop);
    //and in case if we will not make any changes after UNDO(clicked one or more times)
    //and call REDO then
    tempHistory.Push(redBmp);//we save what were on canvas before UNDOand push it to redo history
    redBmp.Free;
  end;

end;


procedure TMainForm.btnRedoClick(Sender: TObject);
var undBmp:TBitmap;
begin
  redoMode:=true;
  if(not tempHistory.isEmpty) then begin

    doneRedo:=True;

    undBmp:=TBitmap.Create;

    undBmp.Assign(tempHistory.getLast);
    imgMain.Picture.Bitmap:=nil;
    MainForm.imgMain.Canvas.Draw(0,0, tempHistory.Pop);

    //same history (like with UNDO implementation) here but reverse
    picHistory.Push(undBmp);
    undBmp.Free;
  end;
end;


{...}

procedure TMainForm.imgMainMouseUp(Sender: TObject; Button: TMouseButton;
  Shift: TShiftState; X, Y: Integer);
var bmp:TBitmap;
begin
//if mouse were down and then it's up this means we drew something
//and must save changes into history to be able to make UNDO
  {...}
    bmp:=TBitmap.Create;
    try
      bmp.Assign(imgMain.Picture.Bitmap);
      picHistory.Push(bmp);
      //if there are some changes added after redo then we clean redo history
     if (doneRedo) then begin
        tempHistory.Clean;
        doneRedo:=false;
     end;
    finally
      bmp.Free;
      //sor of refresh
      imgMain.Canvas.Draw(0,0, picHistory.getLast);
    end;
  {...}

But the problem is it works not way I expected. an example:

If I push undo button once - nothing happens. On twice - it does what it should at once.

And if I drew an ellipse, then click undo once and start draw new one - last drawn ellipse just dissaperas!

Here's the elipse draw method in case if it could be helpful to find out the problem

    procedure TMainForm.ellipseDraw(X, Y: Integer);
    begin
      imgMain.Canvas.Pen.Color:=useColor;
      imgMain.Canvas.Brush.Color:=scndColor;

      imgMain.Canvas.Pen.Width:=size;

      if(mouseIsDown) then  begin
        imgMain.Canvas.Draw(0,0, picHistory.getLast); //there gonna be no bizzare traces from figures
        imgMain.Canvas.Ellipse(dX, dY, X,Y);
      end;
    end;

Answer 1

Answer

If I push undo button once - nothing happens. On twice - it does what it should at once.

That is indeed exactly what your code does:

• In imgMainMouseUp you add the current picture to the undo list, and
• In btnUndoClick you retrieve the last bitmap from the undo list, which is the same as currently seen on the Image.

The solution - to this specific question - is to add the previous bitmap to the undo list instead of the current one.

Bonus

And to address David's comment concerning the leaking, your implementation leaks Bitmaps because:

• The routines Pop and getLast return a newly local created Bitmap. This places the responsibility for its destruction on the caller ot the routines. Your MainForm code does not destroy those Bitmaps, thus they are memory leaks. The solution is to simply return the item in the array, instead of creating a new Bitmap.
• In the Offset routine, you again create new Bitmaps and leak all previous ones. Just assign Queue[I] to Queue[I + 1].
• In the Push method, you forget to free the last item.
• The class does not have a destructor, which again places the responsibility for the destruction of all Bitmaps on the user of the object with the need to call Clean, which it does not. The solution is to add a destructor to your object which calls Clean.

Besides these leaks, there are more problems with your code. Here some fixes and tips:

• Since dynamic arrays are zero-based, your isFull routine does not return True when it should. It should be implemented as Result := historyIndex = hSize - 1;
• Your array is not a queue (FIFO), but a stack (LIFO).
• Your Pop routine does not check for an empty list.

Altogether, your history class could better look like:

uses
  SysUtils, Graphics;

type
  TBitmapHistory = class(TObject)
  private
    FIndex: Integer;
    FStack: array of TBitmap;
    procedure Offset;
  public
    procedure Clear;
    function Count: Integer;
    constructor Create(ACount: Integer);
    destructor Destroy; override;
    function Empty: Boolean;
    function Full: Boolean;
    function Last: TBitmap;
    function Pop: TBitmap;
    procedure Push(ABitmap: TBitmap);
  end;

implementation

{ TBitmapHistory }

procedure TBitmapHistory.Clear;
var
  I: Integer;
begin
  for I := 0 to Count - 1 do
    FreeAndNil(FStack[I]);
  FIndex := -1;
end;

function TBitmapHistory.Count: Integer;
begin
  Result := Length(FStack);
end;

constructor TBitmapHistory.Create(ACount: Integer);
begin
  inherited Create;
  SetLength(FStack, ACount);
  FIndex := -1;
end;

destructor TBitmapHistory.Destroy;
begin
  Clear;
  inherited Destroy;
end;

function TBitmapHistory.Empty: Boolean;
begin
  Result := FIndex = -1;
end;

function TBitmapHistory.Full: Boolean;
begin
  Result := FIndex = Count - 1;
end;

function TBitmapHistory.Last: TBitmap;
begin
  if Empty then
    Result := nil
  else
    Result := FStack[FIndex];
end;

procedure TBitmapHistory.Offset;
begin
  FStack[0].Free;
  Move(FStack[1], FStack[0], (Count - 1) * SizeOf(TBitmap));
end;

function TBitmapHistory.Pop: TBitmap;
begin
  if not Empty then
  begin
    Result := Last;
    Dec(FIndex);
  end;
end;

procedure TBitmapHistory.Push(ABitmap: TBitmap);
begin
  if Full then
    Offset
  else
    Inc(FIndex);
  FStack[Findex].Free;
  FStack[FIndex] := TBitmap.Create;
  FStack[Findex].Assign(ABitmap);
end;

Remarks:

• There also exists a specialized class TObjectStack for this in the Contnrs unit which you could override/exploit.
• There are also concerns with your MainForm code, but I politely leave that up to you to fix.