TAdvMemo OnCustomContextMenuClick not working

When I added items to the context menu using OnCustomizeContextMenuClick, the OnCustomContextMenuClick failed to execute when those items were selected (clicked).

After reviewing the source code in AdvMemo.pas, I see that unless the user interrogates the items in the ContextMenu object, and obtains the pointer to the OnClick method of the internal menu items, and then assigns that method to the new menu items that are being added, the OnCustomContextMenuClick event will not execute.

It seems to me that if you assigned an OnChange event to the internal context PopupMenu, you would be able to assign the DoContextMenuClick event to a new item that was added in the OnCustomizeContextMenu.

Something like:
procedure TAdvCustomMemo.DoContextMenuChange(Sender: TObject; Source: TMenuItem; Rebuild: Boolean);
Var mi : TMenuItem ;
   If Sender = Nil Then Exit ;
   If TPopupMenu(Sender).Items.Count = 0 Then Exit ;
   mi := TPopupMenu(Sender).Items[TPopupMenu(Sender).Items.Count-1] ;
   If (mi.Caption <> '-') And (Not Assigned(mi.OnClick)) Then Begin
      mi.OnClick := DoContextMenuClick ;

However, I see some other problematic issues with the way OnCustomContextMenuClick is being called.  You are assigning the MenuItem.Tag so you can identify which item is clicked.  However, these tag numbers are too commonly used by developers and will be duplicated and inadvertently called.  For example, I'm using the Tag property to identify which bookmark is being referenced.  And since bookmarks are from 0 to 9, that becomes an issue.  I would suggest using very low negative numbers to identify the context menu items, and make sure that those numbers are documented so developers won't use them.  Somewhere around Low(Integer) through Low(Integer)+6

The other thing I noticed is, it appears that the PopupMenu is created each time the right mouse button is clicked.  This is fine, but I do not see where it is being freed (destroyed), with the exception of the application closing.  So there may be many popup's created during the course of the application, and they are only freed when the application closes.  If I am correct, isn't that a waste of resources?  Why not create a single global popup menu and reuse it?  You can clear the items and reinitialize it in the WMContextMenu method.  A single menu object could be shared among every AdvMemo within a single application, this would keep resources down to a minimum.

Sorry for the long winded post...


We have applied improvements to address all issues mentioned below. This will be included in the next update of TAdvMemo.