simple app needs optimization?

[giddy]

Hi ,

i made this simple little app that displays all .NET colours and
handles resize and mouse events.

But if one tries to resize the app , it mucks up and performs
disgustingly slow.

Heres the source:
http://gidsfiles.googlepages.com/ColorBox.cs
http://gidsfiles.googlepages.com/FrmColours.cs


Thanks so much

Gideon

 

[Nicholas Paldino [.NET/C# MVP]]

Gideon,

Well, a few things.

One, I would definitely set the style to have all painting done in
WmPaint and for the optimized double buffer.

Second, you are creating a lot of resources which you are not disposing
of correctly. Fonts, Brushes, Pens, Graphics all implement IDisposable and
you should be disposing of them properly when you are done with them.

Third, the event handlers for the mouse down and whatnot should not be
creating graphics instances and then drawing on them. Rather, you should
set some state that your paint event handler can use to determine what to
paint, and then call Invalidate.

Lastly, your paint event handler should draw on the Graphics image that
is being passed to OnPaint (through the PaintEventArgs instance) and not
call CreateGraphics. A device context is already created for you, there is
no need to create another one.

Hope this helps.

 

[Peter Duniho]

i made this simple little app that displays all .NET colours and
handles resize and mouse events.

But if one tries to resize the app , it mucks up and performs
disgustingly slow.

On what kind of hardware?

For what it's worth, I tried compiling and running your program just for
grins. While I agree wholeheartedly with everything Nicholas wrote, I
didn't see anything that I'd consider to be glaring performance problems
and the program runs fine speed-wise on my computer. Some of the things
he mentions may improve efficiency slightly, but there don't seem to be
any fundamental issues dealing directly with execution speed.

Maybe you can use more specific descriptions to explain the problem you're
having, rather than the rather vague "it mucks up and performs
disgustingly slow". Those aren't what I'd consider technically precise
terminology.

Pete

 

[Freiddie]

As a side note,
you don't need to Dispose the e.Graphics object (and this probably
raises an Exception too, I think).

I used to have similar problems with Graphics-intensive applications,
but now I've learned that the most important of all things you wrote
was to use e.Graphics and always Dispose things readily - I've noticed
before (my mistakes) the memory of my faulty applications can go up
really fast, but Dispose can resolve that, while e.Graphics is
absolutely necessary to ensure that there are no flashing forms, which
was due to the slowness of using CreateGraphics().

Freiddie
http://fei.yuanbw.googlepages.com/
http://freiddy.blogspot.com/
http://scilearn.blogspot.com/

 

[Guest]

Bravo, Peter!

--
Site: http://www.eggheadcafe.com
UnBlog: http://petesbloggerama.blogspot.com
Short urls & more: http://ittyurl.net

 

[giddy]

hi,

Thanks Nicholas. That definately improved a lot of things.

To Peter Duniho ,
umm My Pc : 1.8 GHz , 512 RAM. I'm sorry but i really thought you'd
notice. When i resized , the app would freeze and i could see the
Location of the colour boxes changing one by one. Oddly , i got the
program on my laptop [1.6Ghz 256RAM] and it performed ok , much like
what you *probably experienced. So i'm guessing it was either my pc
then or some code that i changed before posting here! lol

It performs *almost performs perfect now :

Somehow when i _resize_ the scrollable space doesnt adjust correctly.
My calculations are perfect. I Set AutoScrollMinSize.Width to the
current cient width , and the height to the number of rows X (space +
boxHeight).Once you resize , the scrollbar adjusts with this odd
additional space , if you scroll on the bottom you'll see a that
space.

Also , when i click a box , its selected property turns true and it
highlights.But when one selects a box i want the last one to
deselect , so what would my best option be? Setting up an event so
everytime a box is clicked i can store it and then set Selected=false
when another is selected??

Heres the source(updated):
http://gidsfiles.googlepages.com/ColorBox.cs
http://gidsfiles.googlepages.com/FrmColours.cs

Thanks so Much

Gideon

 

[Peter Duniho]

[...]
Somehow when i _resize_ the scrollable space doesnt adjust correctly.
My calculations are perfect.

I beg to differ.

I Set AutoScrollMinSize.Width to the
current cient width , and the height to the number of rows X (space +
boxHeight).Once you resize , the scrollbar adjusts with this odd
additional space , if you scroll on the bottom you'll see a that
space.

I saw a variety of problems with the code you posted. Key problems though
included *two* different ways for you to leave the OnResize() method
having called SuspendLayout() but without calling ResumeLayout(), as well
as inefficient and buggy calculations for the rows and columns. I didn't
rewrite it completely, but here's a version that at least corrects what I
felt were the most problematic issues:

protected override void OnResize(EventArgs e)
{
if (ClientSize.Width == 0)
{
return; //incase the form is minimised.
}

cols = (int)Math.Ceiling(((double)ClientSize.Width / (space
+ szClr.Width)));//the cols that can fit
rows = (int)Math.Ceiling((double)_clrBoxes.Count / cols);//the
number of rows NEEDED
//reposition all boxes
this.SuspendLayout();
Point pt = new Point(space, space);
int b = 0;//box count
int xMax = ClientSize.Width - szClr.Width;

while (b < _clrBoxes.Count)
{
if (pt.X >= xMax)
{
pt.X = space;
pt.Y += (space + szClr.Height);
}

_clrBoxes.Location = pt;
pt.X += space + szClr.Width;
b++;
}

this.ResumeLayout(true);
base.OnResize(e);
}

Note that, among other things, when you handle everything else correctly
the issue of recursive calls does not exist, and you do NOT need to set
the AutoScrollSizeMin explicitly. Because the things affecting the
minimum size are controls, the Form automatically updates the scroll bars
to account for changes in the location of the child controls.

Also, note that in the new code, layout of the child controls always
begins at (0,0) rather than at the scroll position. The Form's own layout
code corrected for the erroneous use of the scroll position while laying
out the child controls, but I believe that in the long run no good could
have come from doing it that way.

There are other subtle differences between the code above and the code you
wrote, but I don't feel a need to enumerate each and every difference.
Suffice to say, where a difference exist, it's my opinion that my code is
better.

Also , when i click a box , its selected property turns true and it
highlights.But when one selects a box i want the last one to
deselect , so what would my best option be? Setting up an event so
everytime a box is clicked i can store it and then set Selected=false
when another is selected??

I see at least two options. One is to maintain a form-level variable that
keeps track of the current selection, so that when you select a new child
control, you can deselect the old selection. The other, and IMHO the most
appropriate method given that you are using Control-derived contents for
the form in the first place, is to take advantage of the fact that
controls already have a concept of having "focus", and automatically deal
with acquiring and losing focus.

Using the second method, you would simply allow the form itself to deal
with actually maintaining the focus state and then check the Focused
property to determine which font to use to draw the control. You'll need
to add handlers for the GotFocus and LostFocus events to invalidate the
control so that it will be updated, but otherwise everything else is
handled for you. You don't have to handle mouse-tracking, using the tab
key to change focus from one control to the next works automatically, and
of course you don't need the _selected field or Selected property doing it
that way.

Pete

 

[giddy]

hi,

Sorry for the late reply.

your code works like a charm! And i love this line:
int xMax = ClientSize.Width - szClr.Width;

I dont need the cols and rows then. It took me a good lot of hours to
come up with my original calculation , dont know how you figrued it
out so quick! =O

I still have'nt completed the focus thing , this is just one of my pet
projects , but i'll update the link to the new code when i do. =P

Thanks so much

Gideon

 

[Peter Duniho]

your code works like a charm! And i love this line:
int xMax = ClientSize.Width - szClr.Width;

Me too. I'm happy you enjoyed it.

I dont need the cols and rows then. It took me a good lot of hours to
come up with my original calculation , dont know how you figrued it
out so quick! =O

The same way you get to Carnegie Hall.

("Practice, years of practice"...)

I still have'nt completed the focus thing , this is just one of my pet
projects , but i'll update the link to the new code when i do. =P

Thanks for letting me know it was helpful. Glad I could assist.

Pete