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
andgetLast
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 assignQueue[I]
toQueue[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 callsClean
.
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 asResult := 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 theContnrs
unit which you could override/exploit. - There are also concerns with your MainForm code, but I politely leave that up to you to fix.