Paul Dardeau 4 Posted 6 hours ago Hi, I needed a mechanism for moveable and resizeable VCL labels and couldn't find exactly what I wanted so I wrote it. Link to github public repository: https://github.com/pauldardeau/delphi-sizeable-label I'm not new to programming, but I am new to Delphi and Windows customized UI controls. You will likely find strange or odd styles in my code as a result. I welcome any and all suggestions for improvement. Screen capture showing it in action: 1 1 Share this post Link to post
Anders Melander 1836 Posted 5 hours ago Very nice! Here's some suggestions from looking through the code: The default scope is published so you'll probably want to start your class declaration with private scope: TSizeableLabel = class(TCustomControl) private ... By convention member variables are prefixed with F (for field). private FInSizingHandle: boolean; FResizeInProgress: boolean; FMoveInProgress: boolean; ... The HostedLabel should be a property, with a setter, not a variable (reason comes below): private FHostedLabel: TLabel; procedure SetHostedLabel(Value: TLabel); public property HostedLabel: TLabel read FHostedLabel write SetHostedLabel; ... procedure TSizeableLabel.SetHostedLabel(Value: TLabel); begin if (Value = FHostedLabel) then exit; FHostedLabel := Value; end; When a component links to another component, it should be able to handle that the other component is deleted. You do this by asking to be notified when the other component is deleted. protected procedure Notification(AComponent: TComponent; Operation: TOperation); override; ... procedure TSizeableLabel.Notification(AComponent: TComponent; Operation: TOperation); begin inherited; if (Operation = opRemove) and (AComponent = FHostedLabel) then HostedLabel := nil; end; procedure TSizeableLabel.SetHostedLabel(Value: TLabel); begin if (Value = FHostedLabel) then exit; if (FHostedLabel <> nil) then FHostedLabel.RemoveFreeNotification(Self); FHostedLabel := Value; if (FHostedLabel <> nil) then FHostedLabel.FreeNotification(Self); end; Forget the above 🙂 I commented while I read the code and only now see that the label is owned by TSizeableLabel. In that case, the property should not have a setter. You don't need to initialize your booleans class vars to False. They have already been initialized. You don't need to test for nil before calling Free. Free already does that. You don't need to reference Self unless you need to pass a reference. The scope is Self by default. Use Pascal case: Result, False, etc. Otherwise very clean and readable code. I wish my team mates wrote code that pretty 🙂 Did you consider using 8 small controls for the handles instead? It would have made painting and the mouse and cursor handling much easier, I think. 1 1 Share this post Link to post
Paul Dardeau 4 Posted 5 hours ago 1 minute ago, Anders Melander said: Here's some suggestions from looking through the code: Thanks Anders! I appreciate the detailed suggestions. I will make all the stylistic and quick/easy changes you suggested. 2 minutes ago, Anders Melander said: Did you consider using 8 small controls for the handles instead? It would have made painting and the mouse and cursor handling much easier, I think. No, that thought never occurred to me! What type of control would you suggest for the handles? Share this post Link to post
Anders Melander 1836 Posted 5 hours ago Just now, Paul Dardeau said: I will make all the stylistic and quick/easy changes you suggested. If you search here for "Style guide" I think you can find some good ones. Be aware though that most of them will pretend to teach the One True Style and that is obviously false because I haven't published it yet 😉 4 minutes ago, Paul Dardeau said: What type of control would you suggest for the handles? TCustomControl is probably the easiest to work with. You'll be using a window handle per handle so it's not super optimal, but I wouldn't worry about it unless you'll be using hundreds of these at the same time. With the handles as separate controls you should also be able to generalize your control so it can attach itself to any TControl instead of embedding it (like I what I thought you were doing - good thing I didn't delete the comment 🙂 ). Share this post Link to post
Anders Melander 1836 Posted 4 hours ago Btw, you should let the TSizeableLabel own the HostedLabel so this: HostedLabel := TLabel.Create(AOwner); should be: HostedLabel := TLabel.Create(Self); - And there you have an example of when to use Self. Now since the HostedLabel is owned by the control, it will be destroyed automatically when the control is destroyed (the base class TComponent takes care of that), so you can must get rid of the HostedLabel.Free. Share this post Link to post
Paul Dardeau 4 Posted 4 hours ago 16 minutes ago, Anders Melander said: Btw, you should let the TSizeableLabel own the HostedLabel so this: HostedLabel := TLabel.Create(AOwner); should be: HostedLabel := TLabel.Create(Self); - And there you have an example of when to use Self. Now since the HostedLabel is owned by the control, it will be destroyed automatically when the control is destroyed (the base class TComponent takes care of that), so you can must get rid of the HostedLabel.Free. Thank you! Much appreciated! Share this post Link to post