views:

341

answers:

3

Here is code:

procedure DisableContrlOL(const cArray : array of string; ReEnable : boolean = False);
// can be called from VKP / RAW / Generation clicks
var
  AComponent: TComponent;
  CompListDis, CompListEna : TStringList;
begin
  CompListDis := TStringList.Create;
  CompListEna := TStringList.Create;
  for i := Low(cArray) to High(cArray) do begin
    AComponent := FindComponent(cArray[i]);
    if Assigned(AComponent) then
      if (AComponent is TControl) then begin
        if TControl(AComponent).Enabled then
          CompListEna.Add(TControl(AComponent).Name)
        else
          CompListDis.Add(TControl(AComponent).Name);
        ShowMessage(TControl(AComponent).Name);
        if ReEnable then begin // if reenabling needed, then all whi
          if not TControl(AComponent).Enabled then
            TControl(AComponent).Enabled := True;
        end else if (TControl(AComponent).Enabled) then
          TControl(AComponent).Enabled := False;
      end;
  end;
end;

I think no more explanations are needed. The ShowMessage correctly shows name of each component, but nothing is added in StringLists. Why?


UPDATE: As question has gone pretty wild, I did confirm answer, which a bit helped me.

I understand that I did write things pretty unclear, but I am very limited, because these code lines is part of commercial project, and my hobby and heart thing. The main problem was found already 6h ago, but Rob just wanted to extend this whole question :D No, no offense, mate, it's OK. I am happy to receive so willing and helpful posts. Thanks again.

A: 

That sure looks like it ought to work. This is the sort of thing that the debugger can probably help with more than we can here.

Try breaking the problematic line down into multiple lines, like so:

  if TControl(AComponent).Enabled then
    CompListEna.Add(TControl(AComponent).Name)
  else CompListDis.Add(TControl(AComponent).Name);

Rebuild with the "Use Debug DCUs" option on, and place a breakpoint on the if statement. Then use F7 to trace your way through the logic and see what's going on.

Mason Wheeler
Nop, does not work. Nothing at all happens to any of stringlists. FindComponenet(); just takes the controls name and thats all...P.S. I used dcu's ...
HX_unbanned
+4  A: 

How do you know that nothing is added to the lists? You create them in this code and the only references to them are in local variables. The objects are leaked when this function returns, so you never actually use the lists anywhere.

You've said you have code for "modular testing." Since that code isn't here, I must assume the code is not part of this function. But if you have external code that's supposed to check the contents of the lists, then the lists can't be just local variables. No other code can access them. You need to either return those lists or accept lists from outside that you then fill. Here's an example of the latter:

procedure DisableContrlOL(const cArray: array of string;
                          Reenable: Boolean
                          CompListDis, CompListEna: TStrings);
// can be called from VKP / RAW / Generation clicks
var
  AComponent: TComponent;
  AControl: TControl;
  i: Integer;
begin
  for i := Low(cArray) to High(cArray) do begin
    AComponent := FindComponent(cArray[i]);
    if not Assigned(AComponent) or not (AComponent is TControl) then
      continue;

    AControl := TControl(AComponent);
    if AControl.Enabled then
      CompListEna.Add(AControl.Name)
    else
      CompListDis.Add(AControl.Name);
    ShowMessage(AControl.Name);

    AControl.Enabled := Reenable;
  end;
end;

The caller of this function will need to provide a TStrings descendant for each list. They could be TStringList, or they could be other descendants, such as TMemo.Lines, so you can directly observe their contents in your program. (They can't be just TStrings, though, since that's an abstract class.)


As you can see, I made some other changes to your code. All your code using the Reenable parameter can be simplified to a single statement. That's because enabling a control that's already enabled, and disabling a control that's already disabled, are no-ops.

Also, Name is a public property of TComponent. You don't need to type-cast to TControl before reading that property, but since you're type-casting so often elsewhere, it made sense to introduce a new variable to hold the type-casted TControl value, and that can make your code easier to read. Easier-to-read code is easier-to-understand code, and that makes it easier to debug.

Rob Kennedy
Hmm, thank you for tip about stringlist, Rob! About that reenable parameter - nop, tyhere is a bit different reason. It would be pretty long to explain I would need to coppy some part of apps source. Also, thans for tip about Components. I knew this, but I was not sure, because in almost every example i find this code ...
HX_unbanned
I think you forgot to answer my question: *How do you **know** something is wrong with your code?* Clearly, your claim that "no more explanations are needed" was false.
Rob Kennedy
bi know it because when i step it with breakpoints, there are no items in stringlist pllus when i devug with dcus, thetre are no varable or memory address picked up and in the continuous code, if i use the code that i need (currently it is just for modular testing), i get runetime list out-of-bounds error. thats how i know there is something wrong.
HX_unbanned
and - it is procedure. It is meant to not provide return, because component status is detected on every execution. Only necessarry trigger is reenable parameter.
HX_unbanned
How does your "modular testing" inspect the lists? Perhaps you need to show that code, too. Like I said, those lists are *local variables*. External code can't see them. Do you understand what "scope" is?
Rob Kennedy
When you pause at a breakpoint, the debugger won't show you the contents of a `TStringList`. It can only show realy properties, not the "virtual" ones like `TStringList.Items`. Write code to display the `TStringList.Text` property instead.
Rob Kennedy
Just checked with Text property. Suprising ( for me ), but it really does add the name correctly. This means that continuous code has some bug.Thanks for the breakpoint tip, Rob!And - I know what scope is. I consider this question a bit humiliating and harrasing to me ...
HX_unbanned
HX, Rob's being very helpful. He cannot know what you know and what you don't know, so he asks. He started by asking how you know what's going on, and you responded (the second time he asked) by saying that there are no items in the stringlist. When you finally checked, there were. So, just accept the help gratefully, and **assume good faith** and don't get offended. You're not paying Rob.
Argalatyr
HX, since you say you're testing this code, and you haven't shown us the test, I figured you were doing what is common for many novices who don't understand scope, which is to declare a *separate variable with the same name* and expect that it will be the same as the original local variable elsewhere in the program. If that's not the case, then please show us what *is* the case.
Rob Kennedy
Hm, Rob, Ok, i cannot show case because it covers about three very large functions, witch is directly linked by others, so it would be hard for you to understand. The main function I vahe had already copied here.The idea was that I call this procedure from other procedure. cArray contains all components with whom procedure will operate ( disable / enable ). ReEnable parameter is "trigger" which dont enable back components that was disabled ( if it is False ), so no matter of count of involved components, all will stay disabled after procedures end. And-CompListDis for later program upgrades.
HX_unbanned
And - it is working OK now, although I will set your latest post as Comfired Answer, Rob ;)
HX_unbanned
+2  A: 

Emphasizing that this is largely based on Rob's excellent suggestions, it looks as though you could simplify the code to:

procedure DisableContrlOL(const cArray : array of string; 
                                ReEnable : boolean = False);
var
  AComponent: TComponent;
begin
  for i := Low(cArray) to High(cArray) do 
  begin
    AComponent := FindComponent(cArray[i]);
    if Assigned(AComponent) then
      if (AComponent is TControl) then 
      begin
        ShowMessage(TControl(AComponent).Name);
        TControl(AComponent).Enabled := ReEnable; 
      end;
  end;
end;

Not clear what the stringlists were for, since their contents were lost when execution left the scope of this procedure. If you want to return them, you should create and free them in the calling code.

Argalatyr
Yes, thank you, problem theoretically found and eliminated.Thank you for this to, but stringlists were for sorting - which is enabled in the time of execution and which comopoent is not. Thasthow I could determine by reenable parameter whenever to enable them or disable.This was not covered by my question because I just thought no value was saved in stringlists at all.Thanks again, sorry for my unreasonable attitude.At this point everybody has the correct answer. ;)
HX_unbanned
...and yet you haven't up-voted any of them! You should up-vote answers (to any question, not just your own) that you find helpful, and *Accept* the best answer to your own questions.
Argalatyr
If you're creating and freeing them in the calling code, then you should not make them `var` parameters. See my edited answer for an example.
Rob Kennedy
Rob - you're quite right! They're just pointers.
Argalatyr