-1

I have created a login form which holds buttons corresponding to users' names held in an Access file. The buttons are created in the OnCreate event because I don't want to have to create them each time the screen is shown.

The buttons display as expected, and I have created LogOn and LogOff procedures, which both work as I expected.

The next step was to only display the buttons for users who are currently logged on to the system. To do this, I have created the following code in the OnActivate event:

procedure TUserInForm.FormActivate(Sender: TObject);
var
   btn : TLBButton;
begin
   With UserQuery do begin;
      first;
      while (not eof) do begin
         BtnName := FieldByName('UserName').AsString;
         Btn := TLBButton(FindComponent(BtnName));
         if (Btn <> nil) then
            if (FieldByName('LoggedIn').AsBoolean = True) then Btn.Visible := True else Btn.Visible := False;
         next;
      end;
   end;
end;

However, it doesn't find any of the buttons - they are all nil. The code throws an Access Violation exception if I remove the nil check. But, at no point in the code do I destroy the buttons or the form itself. The buttons exist, because I can see them on the form. The BtnName variable is global within the unit. I've checked that the BtnName variable is populated correctly from the table.

I've used code similar to this to find components before, with no problems. In fact, I "stole" the code shown above from another procedure (with the obvious changes) in which it works fine. The logs show no errors.

Can anyone suggest some approach to fixing this problem? It's very frustrating!

Remy Lebeau
  • 555,201
  • 31
  • 458
  • 770
Capfka
  • 353
  • 2
  • 6
  • 15
  • 2
    There are several other strange things going on here. First, never write `if X = True then`; simply write `if X then`. Hence, `if (FieldByName('LoggedIn').AsBoolean = True) then Btn.Visible := True else Btn.Visible := False;` can be written `if FieldByName('LoggedIn').AsBoolean then Btn.Visible := True else Btn.Visible := False;`. And, even more of an improvement: `Btn.Visible := FieldByName('LoggedIn').AsBoolean`. Isn't that MUCH easier to read? – Andreas Rejbrand Jan 23 '21 at 17:07
  • 1
    And almost always `FindComponent` is the wrong solution. And if you really must use it, you must also test its class before you use an unsafe cast (using the `is` operator). – Andreas Rejbrand Jan 23 '21 at 17:11
  • Show an example of "_The buttons are created in the OnCreate_". Explain us why one of your `begin` has a semicolon (and others don't). – AmigoJack Jan 23 '21 at 17:30
  • You create the component in the OnCreate event. Just save the values in variables that you can refer later. And check for nil if for some reason they are not always created.No need to "find" them, you have direct access. – fpiette Jan 23 '21 at 17:30
  • Using usernames as part of a component name just asks for trouble - it may work in trivial situations, but even a dot/hyphen/space can become a problem, let alone non-ASCII characters (speak: non-latin letters). I'd store the username in the button's `.Hint` and compare against that. – AmigoJack Jan 23 '21 at 20:36
  • Andreas, thanks for that. I will use the rearrangement of boolean values as you suggest - it makes a lot of sense. AmigoJack, the semi-colon was a typo - I removed a lot of comments from the code before posting it and obviously made a mistake. The button creation routine comes holus-bolus from something I found on the Borland site many moons ago - the parent is the form for all of them. Also, I don't allow spaces or special characters in the username Using .hint is interesting and I'll have a play around with that. At the moment I just put the userquery key in the .tag – Capfka Jan 23 '21 at 23:13
  • I'm surprised that the code even compiles! 'With UserQuery do begin;' - the semicolon should terminate the 'with' statement, so what does 'first' refer to? Normally one opens a query which automatically causes the first record to be 'loaded', thus there's no need for 'first'. – No'am Newman Jan 24 '21 at 05:38
  • @No'amNewman it works because the semicolon is a separator and "empty" commands like `;;;;` are allowed. That's also the reason why for a last command the semicolon is optional (as in `Writeln end;`). – AmigoJack Jan 24 '21 at 08:13
  • @No'amNewman: No, it shouldn't. `with UserQuery do; begin` should do that, but since you have already opened a block with `begin`, you simply get an empty statement inside this block. – Andreas Rejbrand Jan 24 '21 at 10:31
  • As I said above, the spurious semi-colon was an editing error when I pulled the code out of my program to paste here. It isn't actually there in the code. – Capfka Jan 24 '21 at 21:54

1 Answers1

8

FindComponent() searches the owned-components-list of the component that it is called on. I’m assuming your OnCreate handler creates the buttons with the Form as their Owner. But the with block will cause FindComponent() to be called on the UserQuery component instead of the Form. That would explain why the buttons are not being found.

So, you can either:

  • use Self.FindComponent() instead, since the OnActivate handler is being called on the Form, so Self will point at the Form:
procedure TUserInForm.FormActivate(Sender: TObject);
var
  Btn : TLBButton;
  BtnName : string;
begin
  with UserQuery do begin
    First;
    while (not Eof) do begin
      BtnName := FieldByName('UserName').AsString;
      Btn := TLBButton(Self.FindComponent(BtnName));
      if (Btn <> nil) then
        Btn.Visible := FieldByName('LoggedIn').AsBoolean;
      Next;
    end;
  end;
end;
  • get rid of the with block altogether, as it is generally considered to be bad practice to use with anyway in non-trivial cases (this situation is a good example of why):
procedure TUserInForm.FormActivate(Sender: TObject);
var
  Btn : TLBButton;
  BtnName : string;
begin
  UserQuery.First;
  while (not UserQuery.Eof) do begin
    BtnName := UserQuery.FieldByName('UserName').AsString;
    Btn := TLBButton(FindComponent(BtnName));
    if (Btn <> nil) then
      Btn.Visible := UserQuery.FieldByName('LoggedIn').AsBoolean;
    UserQuery.Next;
  end;
end;
  • if you want to keep using the with block, you can move the button search to a separate function that does not exist in the UserQuery component, so the with won’t be confused about which component to call the function on:
procedure TUserInForm.FormActivate(Sender: TObject);
var
  Btn : TLBButton;
  BtnName : string;
begin
  with UserQuery do begin
    First;
    while (not Eof) do begin
      BtnName := FieldByName('UserName').AsString;
      Btn := FindButton(BtnName);
      if (Btn <> nil) then
        Btn.Visible := FieldByName('LoggedIn').AsBoolean;
      Next;
    end;
  end;
end;

function TUserInForm.FindButton(const BtnName: string): TLBButton;
begin
  Result := TLBButton(FindComponent(BtnName));
end;

Now, that being said, it would be a better design to add the created buttons to a list that you manage for yourself, rather than a list that the VCL manages on your behalf. Then you will always know exactly where to find the buttons. For example:

type
  TUserInForm = class(TForm)
    UserQuery: TQuery;
    ...
    procedure FormActivate(Sender: TObject);
    procedure FormCreate(Sender: TObject);
    procedure FormDestroy(Sender: TObject);
    ...
  private
    Buttons: TList;
    function FindButton(const BtnName: string): TLBButton;
    ...
  end;

...

procedure TUserInForm.FormCreate(Sender: TObject);
var
  Btn : TLBButton;
begin
  Buttons := TList.Create;
  ...
  Btn := TLBButton.Create(Self);
  Btn.Name := ... 
  Buttons.Add(Btn);
  ...
end;

procedure TUserInForm.FormDestroy(Sender: TObject);
begin
  Buttons.Free;
end;

procedure TUserInForm.FormActivate(Sender: TObject);
var
  Btn : TLBButton;
  BtnName : string;
begin
  UserQuery.First;
  while (not UserQuery.Eof) do begin
    BtnName := UserQuery.FieldByName('UserName').AsString;
    Btn := FindButton(BtnName);
    if (Btn <> nil) then begin
      Btn.Visible := UserQuery.FieldByName('LoggedIn').AsBoolean;
    end;
    UserQuery.Next;
  end;
end;

function TUserInForm.FindButton(const BtnName: string): TLBButton;
var
 i: integer;
begin
  for i := 0 to Buttons.Count-1 do begin
    Result := TLBButton(Buttons[i]);
    if Result.Name = BtnName then Exit;
  end;
  Result := nil;
end;
Remy Lebeau
  • 555,201
  • 31
  • 458
  • 770
  • 3
    [Issues with `with` should be known](https://stackoverflow.com/q/23294552/4299358) to OP. – AmigoJack Jan 23 '21 at 20:44
  • Thanks to everyone who contributed to this. I'm really impressed, particularly with Remy, for the amount of work he put into his very complete answer. Just to let you know, I use "with" while developing bits of the program, then remove them later. David Heffernan pointed out a problem I was having was due to using "with" and I've stopped using them in the final product. The outfit I "work" for (I don't get paid!) is a charity which doesn't want to spend money on infrastructure, so training courses and software upgrades are out of the question. Again thanks for your great responses! – Capfka Jan 23 '21 at 22:58