3
\$\begingroup\$

I have a main form which does processing for an extended period of time. Normally, progress bars are used to show either the loading status or simply show the program is still running/hasn't freezed.

For this form, I wanted to give it a more snazzy look by reusing a common loading GIF found on the web that many users would be familiar with.

This is how I implement said GIF:

ShowLoadingGif();

              //Heavy lifting

HideLoadingGif();

private void ShowLoadingGif()
{
    this.Enabled = false;

    Point formCenterPt = new Point()
    {
        X = this.Location.X + (this.Width/2),
        Y = this.Location.Y + (this.Height/2)
    };

    loadingThread = new Thread(() => ShowLoading(formCenterPt));
    loadingThread.IsBackground = true;
    loadingThread.Name = "Loading gif";
    loadingThread.Start();
}   

private void HideLoadingGif()
{
    try
    {
        this.Enabled = true;
        if(loadingThread.IsAlive)
        loadingThread.Abort();
    }
    catch (Exception ex)
    {
        Log(String.Concat("Loading gif thread release failed: ",ex));
    }
}

private void ShowLoading(Point parentCenter)
{
    frmLoadingGif loading = new frmLoadingGif();

    Point offsetPt = new Point()
    {
        X = parentCenter.X - (loading.Width /2),
        Y = parentCenter.Y - (loading.Height/2)
    };

    loading.StartPosition = FormStartPosition.Manual;
    loading.Location = offsetPt;
    loading.ShowDialog();
}

What this does is produces a loading GIF (I grabbed mine from ajaxloader) in the center of the form while processing is ongoing, and also disables the form.

My question is, whether or not what I did was overkill for something as simple as an inprogresss status, or if using threads in this way is the best way to do it. I realize that using a control on the main form would have been easier, but I couldn't find a way to do so with a GIF while also making the background completely transparent.

Why have I chosen to use a form for showing the GIF?

The alternative would have been to add the picture box to the main form. However, this didn't seem the best solution because I wanted to center the GIF to the form itself, and with the TableLayoutPanel I was using, you can't simply change the location of a control in that manner (please correct me if I'm wrong here). I also wanted to keep the transparency, threading, and other attributes of the picturebox control separate from the main form so I could reuse it elsewhere.

Finally, I wanted to show the GIF loading on top of the current form, and not just in the corner/bottom in its own section like a progress bar would. As you'll see in the picture, the loading GIF is shown on top of the control behind it, a DataGridView.

enter image description here

\$\endgroup\$
1
\$\begingroup\$

I don't know much about threading, so I can't really comment on that, but there is an opportunity to extract some logic that I am sure is used elsewhere in your project. You have this in ShowLoadingGif()

Point formCenterPt = new Point()
{
    X = this.Location.X + (this.Width/2),
    Y = this.Location.Y + (this.Height/2)
};

And this in ShowLoading()

Point offsetPt = new Point()
{
    X = parentCenter.X - (loading.Width /2),
    Y = parentCenter.Y - (loading.Height/2)
};

I would create two functions that take a form as an argument and return a point representing the StartingPosition and center point respectively.


I'm not a fan of this If statement.

try
{
    this.Enabled = true;
    if(loadingThread.IsAlive)
    loadingThread.Abort();
}

First off, at the very least, loadingThread.Abort needs to be indented properly.

if(loadingThread.IsAlive)
    loadingThread.Abort();

But I still don't like it. Let's say down the road Mr. Maintainer realizes that something else should happen if(loadingThread.IsAlive. Then they do something like this.

if(loadingThread.IsAlive)
    loadingThread.Abort();
    foo.bar();

The problem here is, foo.bar() will always be executed, but it looks like it will only be executed if(loadingThread.IsAlive).

So, there are really only two options in my mind.

  1. Write it as a single line If statement.

    if (loadingThread.IsAlive) loadingThread.Abort();
    
  2. Or put the braces in.

    if (loadingThread.IsAlive)
    {
        loadingThread.Abort();
    }
    

This also makes me uncomfortable.

catch (Exception ex)
{
    Log(String.Concat("Loading gif thread release failed: ",ex));
}

You're swallowing every possible exception with nothing more than a message in a log file. You should figure out which exceptions you can gracefully handle and catch only those.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.