Board index » cppbuilder » How to set the color of all forms correctly (Screen->Forms[...]->Color)

How to set the color of all forms correctly (Screen->Forms[...]->Color)


2004-07-20 11:09:11 PM
cppbuilder104
hi dear builders,
i want to change the color of all forms of my application at runtime when i'm
click on the button ' cmdApply_FormsColorClick '..the button stores first the
color-value into the registry and than it sets all the forms->color's via:
for(int i = 0; i < Screen->FormCount; ++i)
{
Screen->Forms[i]->Color =
StringToColor(reg->ReadString("Forms_Color"));
}
but something here doesn't work properly, i'm calling in the constructor of
my mainform the function: " Read_FormsColor() " to read the color value
from the registry (that the button stores) and stepping through all forms via
Screen->Forms..to set the color to each form..
I get an AccessViolation error when i'm clicking the button
' cmdApply_FormsColorClick ' WHY ? WHAT is wrong...?
can someone please help me..?
Oren
/***********************************************************/
// mainform constructor...
__fastcall TForm1::TForm1(TComponent* Owner):TForm(Owner)
{
frmOptionen->Read_FormsColor(); // set all forms->color..
}
void __fastcall TfrmOptionen::Read_FormsColor()
{
TRegistry* reg = new TRegistry;
try
{
reg->OpenKey(RegistryKey, true);
if(reg->ValueExists("Forms_Color"))
{
for(int i = 0; i < Screen->FormCount; ++i)
{
Screen->Forms[i]->Color =
StringToColor(reg->ReadString("Forms_Color"));
}
}
else
{
for(int i = 0; i < Screen->FormCount; ++i)
{
Screen->Forms[i]->Color = clSilver; // default color if registry-value
is NOT existing..
}
}
reg->CloseKey();
}
__finally {delete reg;}
}
// HERE something is causing the AccessViolation...
void __fastcall TfrmOptionen::cmdApply_FormsColorClick(TObject *Sender)
{
TRegistry* reg = new TRegistry;
try
{
reg->OpenKey(RegistryKey, true);
reg->WriteString("Forms_Color", ColorToString(shpFormColor->Brush->Color));
reg->CloseKey();
delete reg;
}
__finally
{
delete reg;
}
// i guess HERE something seems not to be correct...but what..?
for(int i = 0; i < Screen->FormCount; ++i)
{
Screen->Forms[i]->Color = shpFormColor->Brush->Color;
}
}
// storing the color in a TShape (shpFormColor)
void __fastcall TfrmOptionen::cmdSelectColorClick(TObject *Sender)
{
if(ColorDialog1->Execute())
{
shpFormColor->Brush->Color = ColorDialog1->Color;
}
}
 
 

Re:How to set the color of all forms correctly (Screen->Forms[...]->Color)

You ought to step through the code in the de{*word*81} to see which line causes
the AV.
Also check what reg->ReadString("Forms_Color"); returns outside the loop as
this could be a problem (just a guess)
The de{*word*81} is your friend in this case :)
HTH Pete
"Oren Halvani" < XXXX@XXXXX.COM >wrote in message
Quote
hi dear builders,

i want to change the color of all forms of my application at runtime when
i'm
click on the button ' cmdApply_FormsColorClick '..the button stores first
the
color-value into the registry and than it sets all the forms->color's via:

for(int i = 0; i < Screen->FormCount; ++i)
{
Screen->Forms[i]->Color =
StringToColor(reg->ReadString("Forms_Color"));
}

but something here doesn't work properly, i'm calling in the constructor
of
my mainform the function: " Read_FormsColor() " to read the color value
from the registry (that the button stores) and stepping through all forms
via
Screen->Forms..to set the color to each form..

I get an AccessViolation error when i'm clicking the button
' cmdApply_FormsColorClick ' WHY ? WHAT is wrong...?
 

Re:How to set the color of all forms correctly (Screen->Forms[...]->Color)

sorry guys :-) i found the problem...
try
{
reg->OpenKey(RegistryKey, true);
reg->WriteString("Forms_Color", ColorToString(shpFormColor->Brush->Color));
reg->CloseKey();
delete reg; // THIS IS OF COURSE WRONG :-)
}
__finally
{
delete reg;
}
Oren
 

{smallsort}

Re:How to set the color of all forms correctly (Screen->Forms[...]->Color)

Oren Halvani wrote:
Quote
reg->CloseKey();
delete reg; // THIS IS OF COURSE WRONG :-)
No. That is valid code.
You cannot say "THIS IS OF COURSE WRONG" whithout
giving a reason.
Hans.
 

Re:How to set the color of all forms correctly (Screen->Forms[...]->Color)

"Hans Galema" < XXXX@XXXXX.COM >wrote in message
Quote
No. That is valid code.

You cannot say "THIS IS OF COURSE WRONG" whithout
giving a reason.
I guess you didn't study the code more closely. You would see that the code
is deleting the TRegistry instance twice:
try
{
reg->OpenKey(RegistryKey, true);
reg->WriteString("Forms_Color",
ColorToString(shpFormColor->Brush->Color));
reg->CloseKey();
delete reg; // <- deleted once - this line needs to be removed
}
__finally
{
delete reg; // <- deleted again even if already done above
}
Gambit
 

Re:How to set the color of all forms correctly (Screen->Forms[...]->Color)

Remy Lebeau (TeamB) wrote:
Quote
I guess you didn't study the code more closely.
Of course I did.
Quote
You would see that the code
is deleting the TRegistry instance twice:
I had seen that of course. I know why it AV's.
I was only commenting on the the way Oren explained an error.
"delete reg; // THIS IS OF COURSE WRONG "
is not a good way to tell other people that things are
wrong. That code is perfectly ok until you try to delete
the instance for a second time.
I made the remark for Oren so Oren could realize that
he could express better what he meant and then retry.
Hans.
 

Re:How to set the color of all forms correctly (Screen->Forms[...]->Color)

"Oren Halvani" < XXXX@XXXXX.COM >wrote:
Quote

Gambit already found the problem but I have some comments.
Quote
reg->OpenKey(RegistryKey, true);
Here you're telling OpenKey to create the key if it doesn't
already exist. This function is to fetch the colors so why
would you want to do that?
Quote
reg->CloseKey();
The TRegistry's destructor automatically calles CloseKey so
since you're following this line with a delete it's not needed.
Your code presumes that all forms are autocreated and created
in a specific order - which is ok i guess if that's what's
actually happening. If not, or if you change the autocreation
of any of the forms in the future, you'll get unexpected results.
You can address this issue by simply changing the key for the
form's color to include the form's name so as you iterate the
Screen's forms, you dynamically buld the key that you're going
to try to read.
As for your choice of the value of the key, I wouldn't use the
string simply because it requires more space. IMHO, when one
has a choice, one should always choose the 'system friendly'
choice unless there's a good reason not to.
Finally, you don't need to use a try/finally. I try to avoid
try/catch/finally blocks when ever possible because they add
extra overhead and they add up.
Consider something more like:
//-------------------------------------------------------------
void __fastcall TfrmOptionen::Read_FormsColor()
{
TRegistry *Registry = new TRegistry;
if( Registry->OpenKey(RegistryKey, false) )
{
TColor C;
AnsiString FormKeyName;
for( int x = 0; x < Screen->FormCount; ++x )
{
FormKeyName = Screen->Forms[ x ]->Name + "_Color";
if( Registry->ValueExists(FormKeyName) )
{
Registry->ReadBinaryData( FormKeyName, &C, sizeof(int) );
Screen->Forms[ x ]->Color = C;
}
}
}
else
{
// do nothing because you set their colors in the IDE - right?
}
delete Registry;
}
//-------------------------------------------------------------
void __fastcall TfrmOptionen::cmdApply_FormsColorClick(TObject *Sender)
{
TRegistry *Registry = new TRegistry;
if( Registry->OpenKey(RegistryKey, true) )
{
// why are you not using the Form's Color?
TColor C = shpFormColor->Brush->Color;
AnsiString FormKeyName = Name + "_Color";
Registry->WriteBinaryData( FormKeyName, &C, sizeof(int) );
}
delete Registry;
}
//-------------------------------------------------------------
You could also change the code to deal with just one form at a
time and add it to each form's c'tor and d'tor so that it
loads the color when it's created and saves the color when
it's destroyed.
Taking it one step further, you could make you're own component:
newsgroups.borland.com/cgi-bin/dnewsweb&utag=
~ JD
 

Re:How to set the color of all forms correctly (Screen->Forms[...]->Color)

"JD" < XXXX@XXXXX.COM >schrieb im Newsbeitrag
Quote
Finally, you don't need to use a try/finally. I try to avoid
try/catch/finally blocks when ever possible because they add
extra overhead and they add up.
thanks for all the great infos JD..
i realy didn't know that try/catch are adding extra overhead..
is that realy true..? if so i'll need to make huge code changings
inside my app...
Oren
 

Re:How to set the color of all forms correctly (Screen->Forms[...]->Color)

"Hans Galema" < XXXX@XXXXX.COM >schrieb im Newsbeitrag
Quote
is not a good way to tell other people that things are
wrong. That code is perfectly ok until you try to delete
the instance for a second time.

I made the remark for Oren so Oren could realize that
he could express better what he meant and then retry.

Hans.
dear Hans,
Englisch is NOT my mother tongue..
so please forgive me, OK..?
Oren
 

Re:How to set the color of all forms correctly (Screen->Forms[...]->Color)

"Oren Halvani" < XXXX@XXXXX.COM >wrote:
Quote
i realy didn't know that try/catch are adding extra overhead..
is that realy true..?
Yes. It's been a while since I read about it so I can't speak
intelligently about how and why other than to say that it does.
Quote
if so i'll need to make huge code changings inside my app...
Don't get me wrong. I use it often enough - one has to when
using something that can throw - like presumming that a
connection still exists with the database for example.
But using it to trap logic errors - like Index out of bounds
is just plain bad programming and in your case it was simply
extra code.
~ JD
 

Re:How to set the color of all forms correctly (Screen->Forms[...]->Color)

"JD" < XXXX@XXXXX.COM >schrieb im Newsbeitrag
Quote
But using it to trap logic errors - like Index out of bounds
is just plain bad programming and in your case it was simply
extra code.

~ JD
OK..i got you..thanks for the hint JD...
Oren
 

Re:How to set the color of all forms correctly (Screen->Forms[...]->Color)

Oren Halvani wrote:
Quote
Englisch is NOT my mother tongue..
so please forgive me, OK..?
Apart from that I do not see your wrong comment caused by
you not being native english, it would bring more for us -and
for yourself too- if you had commented on your own and my
comments.
Hans.