Cast error when using Ctrl+#

[Stefan Glienke 2002]

The code in TGotoModificationBaseExpert.HandleOnMenuPopup is causing an error every time I use Ctrl+# in 10.4.1

 

That is because there is an unconditional soft cast of _Sender to TPopupMenu which is not true all the time. Adding if _Sender is TPopupMenu around it fixes it.

Edited September 29, 2020 by Stefan Glienke

[dummzeuch 1505]

WTF? It's a bloody TMenuItem! That doesn't even have a common ancestor with TPopupMenu (apart from TComponent)! Who in their right mind would do that?

 

Fixed in revision #3325

[Arnaud Bouchez 407]

Unconditional cast should never be used on UI/Client side. Always uses "is" or "as".
It may be used only when performance really matters, e.g. on core server code, with a large test coverage, and only on proven bottlenecks - which are very rare.

The first is to write "as" to raise an exception. Then - maybe - use direct class casting. Only if really needed.

 

IMHO such direct class casting - or cascaded "if .. is .. then ... else if .. is .. then ..." may be a smell for potential breaking some of the SOLID principles.
In VCL events, we use Sender: TObject most of the time - it could be seen as a design flaw / legacy debt. But it is a reality.
We should be able to avoid direct class casting in most of our modern code.

Edited September 29, 2020 by Arnaud Bouchez

[dummzeuch 1505]

2 hours ago, Arnaud Bouchez said:

Unconditional cast should never be used on UI/Client side. Always uses "is" or "as".
It may be used only when performance really matters, e.g. on core server code, with a large test coverage, and only on proven bottlenecks - which are very rare.

The first is to write "as" to raise an exception. Then - maybe - use direct class casting. Only if really needed.

 

IMHO such direct class casting - or cascaded "if .. is .. then ... else if .. is .. then ..." may be a smell for potential breaking some of the SOLID principles.
In VCL events, we use Sender: TObject most of the time - it could be seen as a design flaw / legacy debt. But it is a reality.
We should be able to avoid direct class casting in most of our modern code.

Not sure what you are trying to tell us. The code in question was:

procedure TGotoModificationBaseExpert.HandleOnMenuPopup(_Sender: TObject);
var
  pm: TPopupMenu;
begin
  pm := _Sender as TPopupMenu;
  if Assigned(FOrigPopupEvent) then begin
    FOrigPopupEvent(_Sender);
  end;
  AppendMenuItem(pm);
end;

So this already was an 'as' operation which bombed out in Delphi 10.4 because somehow a TMenuItem was passed as Sender.

 

The new code looks like this:

procedure TGotoModificationBaseExpert.HandleOnMenuPopup(_Sender: TObject);
begin
  if Assigned(FOrigPopupEvent) then begin
    FOrigPopupEvent(_Sender);
  end;
  if _Sender is TPopupMenu then begin
    // sometimes Delphi 10.4.1 passes a TMenuItem here, no idea how this can happen, but it caused a runtime error
    AppendMenuItem(TPopupMenu(_Sender));
  end;
end;

So now I first call the original OnPopup-event and then I check whether the Sender is really a TPopupMenu before adding an item to it.

 

I can't see how this relates to what you wrote. There was no hard unconditional cast (OK, now there is, but only after an 'is'). There is no if .. is .. then else if .. is .. then.

[Arnaud Bouchez 407]

My remark was a general one. It was not about this explicit code.

Writing `TPopupMenu(_Sender)` is a hard-cast which should never appear in most code, unless you have a very vague/legacy event like a TNotifyEvent. The VCL requires it.
 

For UI code, I would write:

 if _Sender is TPopupMenu then begin
    // sometimes Delphi 10.4.1 passes a TMenuItem here, no idea how this can happen, but it caused a runtime error
    AppendMenuItem(_Sender as TPopupMenu);
  end;

The code is somewhat redundant, because "is" is actually doing the class hierarchy check that "as" is doing: calling internally InheritsFrom() twice
But it is only slightly slower - unnoticeable on a VCL app, since passing a Windows message will be slower than this "as" execution.
IMHO it is safer and easier to read and to maintain.
Having used "as" at first hand did indeed give a hint about the initial problem.

 

Hardcasting is like premature optimization on client side. 

Edited September 29, 2020 by Arnaud Bouchez

[Attila Kovacs 629]

Ctrl+# is the toggle comment menuitem on the editors context menu in the german IDE, not sure though if it has anything to do with whatever it is, but I'm sure you already checked where this "Sender" belongs to.

[dummzeuch 1505]

8 hours ago, Attila Kovacs said:

Ctrl+# is the toggle comment menuitem on the editors context menu in the german IDE, not sure though if it has anything to do with whatever it is, but I'm sure you already checked where this "Sender" belongs to.

I tried, but the call stack didn't give any clue where this call comes from. I guess the IDE is calling that event explicitly as MyPopup.OnPopup(Whatever) somewhere and the parameter isn't used in the original event implementation so it doesn't care what it is. The MenuItem neither has a name nor a caption, which is odd.