0

Don't use DoEvents(). Use threads!

That mantra is roaming around the internet including SO. Okey so I created a short proof of concept where I tried to use only Threads. So basically what button should do is trigger moving the blue box downwards.

It runs in a separate thread YET windows form is unresponsive (I can't move it or click the button again) until it finishes moving downwards.

Question is, what did I mess up? If I shouldn't use DoEvents(), what instead? (If you uncomment the Application.DoEvents() line, it'll become responsive).

CODE

using System;
using System.Drawing;
using System.Threading;
using System.Windows.Forms;

namespace doevents
{
    public partial class Main : Form
    {
        // Use static so that I don't have to pass them over and over
        public static TableLayoutPanel movingBox;
        public static Form mainForm;

        // Initialize
        public Main()
        {
            InitializeComponent();

            Main.mainForm = this;
            Main.movingBox = tableLayoutPanel1;
        }

        // Button that runs thread which will move the blue box down
        private void button1_Click(object sender, EventArgs e)
        {
            new Thread(
                new ThreadStart(
                    () => 
                        {
                            SlideBoxDown();
                        }
                    )
                ).Start();
        }

        // Delegate
        delegate void SlideBoxDownCallback();

        // Slide box down
        private static void SlideBoxDown()
        {
            if (Main.movingBox.InvokeRequired)
            {
                SlideBoxDownCallback d = new SlideBoxDownCallback(SlideBoxDown);
                Main.mainForm.Invoke(d, new object[] { });
            }
            else
            {
                for (int i = 0; i < 20; i++)
                {
                    Main.movingBox.Location = new Point(Main.movingBox.Location.X, Main.movingBox.Location.Y + 2);
                    Thread.Sleep(100);
                    //Application.DoEvents();
                }
            }
        }       
    }
}

Application layout

enter image description here

H H
  • 263,252
  • 30
  • 330
  • 514
Mike
  • 842
  • 1
  • 9
  • 31
  • 2
    You really shouldn't be exposing internal controls of this form, such as your table layout panel, outside of this type, let alone to the entire program. It should be an internal implementation detail of this form. – Servy Aug 14 '14 at 16:54
  • 1
    @HenkHolterman Why? It's entirely appropriate to post tangentially related useful information that doesn't answer the question in comments. It'd be appropriate as an *answer* on CR, but that's another matter entirely. – Servy Aug 14 '14 at 17:02
  • I voted you up @HenkHolterman even tho you mock me : – Mike Aug 14 '14 at 17:56

3 Answers3

3

You are still calling Thread.Sleep(100) on the main Thread. Because SlideBoxDown is Invoking itself back to the GUI thread. And since that is the only thing happening in the Thread, your program is essentially single-threaded.

for (int i = 0; i < 20; i++) simply is not a good way to control an animation.
Use a Timer.

And about (If you uncomment the Application.DoEvents() line, it'll become responsive) - correct, but try closing the Form in the middle of that animation then. You won't like it.

H H
  • 263,252
  • 30
  • 330
  • 514
1

You create a new thread, and then in the method you call from your new thread you marshal back to the UI thread and then run all of the rest of your code in the UI thread. This pretty well defeats the purpose of starting a new thread in the first place.

To perform an operation every X interval of time, in this case to move an object every 100 milliseconds, use a Timer.

Servy
  • 202,030
  • 26
  • 332
  • 449
-1

Changes to an application's UI require the UI thread. That's really all there is to it. Your code is creating a background thread for the sole purpose of passing a message to the application's message pump, which is consumed by the UI thread. So, everything happening in the "else" clause, including the Thread.Sleep() call, is being executed on the main thread.

I would instead refactor this code to replace the Sleep with a Timer. The Timer will run on a background thread, and can trigger a handler on its "Tick" event (spaced 100ms apart) that can move the rectangle by one increment. That way, the drawing happens based on consuming messages on the UI thread, but the waiting is done by a background thread, keeping the UI responsive.

KeithS
  • 70,210
  • 21
  • 112
  • 164
  • Timer's *don't use another thread at all*. They don't just create a background thread that sits there doing nothing for a set period of time. They actually avoid the use of another thread entirely. – Servy Aug 14 '14 at 16:53
  • Depends on the implementation, and there are at least four of them that I know of in .NET. System.Windows.Forms.Timer might use some other mechanism, but System.Threading.Timer definitely does its thing with another thread. – KeithS Aug 14 '14 at 16:56
  • 1
    No, it doesn't. It does it by creating OS hooks that will fire after a set period of time. No new threads are created. A thread pool thread is used to actually execute the event handlers, but that's an entirely different matter. Those threads are only ever doing actual work (namely executing the handlers). They're never sitting around doing nothing because the timer needs a thread to wait. – Servy Aug 14 '14 at 17:02
  • Just because you can't see the implementation doesn't mean it isn't there. .NET Timers may not use *managed* threads, but the basic function of a timer is to continually check the condition that a certain number of milliseconds has passed, and when that condition is true, fire off whatever logic was supposed to happen. The Windows API may be used to keep the condition-checking lightweight, but it remains that *something* has to compare the system clock to a defined exit condition at intervals, and that logic requires an execution unit. – KeithS Sep 15 '14 at 19:07
  • Yes, there is functionality somewhere to compare the current time to scheduled events, to see if they should fire, but it's not a thread, it doesn't exhibit the properties of threads, it doesn't have the costs associated with threads, etc. – Servy Sep 15 '14 at 19:13