0

This is not a duplicate of Thread.sleep() in while loop doesn't work properly? Because that one the accepted answer is that he mixed up milliseconds and seconds.

Note- somebody suggested that Keep UI thread responsive when running long task in windows forms would solve my question. The example in that question is very complicated in that it has lots of things that aren't relevant and answers cater to that and include all that complexity. It's much harder to understand that question and answer. My question is far simpler and hence the answer is much simpler. The accepted answer here has no token.ThrowIfCancellationRequested(); It's very hard to adapt the complex answer at that link, to my simple question. I would certainly not accept the accepted answer to that question as an answer to mine. Hans has actually given a one line answer. I don't think a one line answer applies to that question. So my question is different to that one.


I have a form with a button and a label

I have this code when the button is clicked

A button click calls a function that does this

            int a, b;           
            int i=0;
            while(i<5)
            {
                i += 1;
                Thread.Sleep(1000);
                label1.Text += "a";               
            }
            MessageBox.Show("done");

I'd 'expect'/want, that that should write each of 5 'a's, pausing for a second between each one.

But it doesn't, it buffers them. it waits 5 seconds then writes all the 'a's in one go and prints 'done'.

Why is it buffering?

I don't want to use a timer because I want it to be sequential. I don't want 'done' to print till that timed task is done.

How can get the code I have to work as I want?

There is an accepted answer on this question C# - How to pause application until timer is finished? I don't quite understand it, I would test it but but I can't even see how to test it, because I can't really see where i'd put the code from the accepted answer, how i'd adapt it https://i.imgur.com/gjRDEiX.png It seems maybe that accepted answer has subclassed timer or something, and that doesn't seem necessary to my problem/question.

barlop
  • 10,225
  • 7
  • 63
  • 94
  • It isn't buffering, the Label.OnPaint() method cannot run when you intentionally write "do not do anything else" code. Add `label1.Update();` to this code, learning how to use a timer or a worker thread can wait until later. – Hans Passant Feb 14 '19 at 00:01
  • @HansPassant thanks, that's amazing. Adding that one line `label1.Update();` straight after `label1.text+="a";` . You can post that as an answer. What would count as "do not do anything else" code"? Sequential code within the same thread? – barlop Feb 14 '19 at 00:20
  • @barlop so you're ok with blocking the UI for a good 5 seconds? – noseratio Feb 14 '19 at 00:33
  • @noseratio i'd just be disabling the button while that code runs, so preventing the user from clicking the button, so blocking that aspect of the UI, the only aspect of the UI that executes that code . – barlop Feb 14 '19 at 00:40
  • side note but can of course use timer, so count the ticks `if(tmrblahtickcount==5) {timer1.enabled=false;tmrblahtickcount=0; thingtorunaftertimer(); }` – barlop Feb 18 '19 at 12:54

3 Answers3

2

Just a suggestion - use Microsoft's Reactive Framework (Rx) - NuGet System.Reactive.Windows.Forms and pop a using System.Reative.Linq; at the top of your code. Then you can write:

Observable
    .Interval(TimeSpan.FromSeconds(1.0))
    .Take(5)
    .ObserveOn(this)
    .Subscribe(_ => label1.Text += "a");
Enigmativity
  • 97,521
  • 11
  • 78
  • 153
1

The async approach is the cleanest in my opinion.

Here's another approach, though, demonstrating how to update the UI with Invoke() if you ever take on a more classic Task/Thread style:

    private void button1_Click(object sender, EventArgs e)
    {
        button1.Enabled = false;

        Task.Run(() => {
            int i = 0;
            while (i < 5)
            {
                i += 1;
                System.Threading.Thread.Sleep(1000);
                label1.Invoke((MethodInvoker) delegate {
                    label1.Text += "a";
                });

            }
            button1.Invoke((MethodInvoker) delegate {
                MessageBox.Show("done");
                button1.Enabled = true;
            });             
        });
    }
Idle_Mind
  • 33,113
  • 3
  • 25
  • 33
  • A fire-and-forget call like this to `Task.Run` is rarely a good idea. There's an `IProgress<>`-pattern for this, [example](https://stackoverflow.com/a/21357567/1768303), but I doubt the OP needs a background thread in his case. – noseratio Feb 14 '19 at 03:12
  • Thanks, that is really interesting, BTW I see that messagebox.show doesn't have to be in that .Invoke body – barlop Feb 14 '19 at 14:18
  • This answer also shows me that without that .Invoke body, the button1.enabled line causes an exception about it being invoked in a different thread.. which is interesting 'cos it led me to see that in the case of the async method it's the same thread hence no exception, and I see that from printing System.Threading.Thread.CurrentThread.ManagedThreadId Whereas here iti's a different thread. – barlop Feb 14 '19 at 14:20
  • 1
    I also see that one can do this.invoke or button1.invoke (for any of the controls), 'cos it "Executes the specified delegate on the thread that owns the control's underlying window handle." as mentioned https://stackoverflow.com/questions/14703698/invokedelegate – barlop Feb 14 '19 at 14:21
  • The MessageBox is in the Invoke body so that the button is not enabled until after the dialog is dismissed. – Idle_Mind Feb 14 '19 at 14:34
  • I don't mean running the MessageBox.Show after the button is enabled. If you put the MessageBox.Show outside, but before the Invoke body(rather than after), then the button is only enabled after the dialog has been dismissed. (so you still get that effect by virtue of the MessageBox.Show preceding the button being enabled) – barlop Feb 14 '19 at 14:40
-2

Your method must return control to the caller in order for the message pump to process the repaint events that will render the new text. You have two options.

The old way

void SomeHandler(object sender, EventArgs e)
{
    int a, b;           
    int i=0;
    while(i<5)
    {
        i += 1;
        Thread.Sleep(1000);
        Application.DoEvents(); //Process events in the event queue
        label1.Text += "a";               
    }
    MessageBox.Show("done");
}

The new way

For this to work, you have to make your handler async.

async void SomeHandler(object sender, EventArgs e)
{
    int a, b;           
    int i=0;
    while(i<5)
    {
        i += 1;
        await Task.Delay(1000);  //The await keyword yields control to the caller
        label1.Text += "a";               
    }
    MessageBox.Show("done");
}

The latter method is considered superior. DoEvents is almost universally frowned upon by developers.

John Wu
  • 44,075
  • 6
  • 37
  • 69
  • 1
    Or the other way - use a timer. – stuartd Feb 13 '19 at 23:53
  • I like the simplicity of the code. I just tested "The old way" it prints the last two a's simultaneously. There should be a pause between each and every 'a'. And I suppose no pause between the last 'a' and done. So the only issue there with that code is it is printing the last two 'a's simultaneously. Why would it print the last two 'a's simultaneously? your new way method doesn't have that bug. – barlop Feb 14 '19 at 00:04
  • `Application.DoEvents(); //Yield control to message pump` - technically, it doesn't "yield control to the message loop"; rather, [it runs a nested message loop](https://referencesource.microsoft.com/#System.Windows.Forms/winforms/Managed/System/WinForms/Application.cs,0880978c39425ff1) with the main UI unblocked (unlike a modal dialog), which is a very nasty thing. – noseratio Feb 14 '19 at 00:04
  • As to the `async/await` version, a care still should be taken to avoid re-entrancy, i.e., when `SomeHandler` is called again while the previous asynchronous logic inside it still going, FYI @barlop. – noseratio Feb 14 '19 at 00:08
  • NOOOOO! No!! No! Dont use DoEvents!!!! The old way you should use `Label1.Invalidate()` – Jeremy Thompson Feb 14 '19 at 00:12
  • @JeremyThompson alright he did say that was the old way and he said it wasn't recommended. And he proviided the new way – barlop Feb 14 '19 at 00:12
  • @noseratio How would I avoid that re-entrancy problem that you mention? If I had a boolean that got set when the thing completed, and tested a boolean before running the while. So even if it did re-enter, I guess it wouldn't cause a clash of code. I guess I'd probably disable the button while the code runs. So it shouldn't reenter at all, would that disabling of the button to prevent re-entry be sufficient(when the button is the only thing I have that runs that code anyway)? – barlop Feb 14 '19 at 00:13
  • @barlop, it depends on the UI workflow of your app. Let's say the user clicked the button that invokes `SomeHandler` twice. What would you like to do with the pending operation? E.g., ignore the new click (with a simple flag)? Or stop the old one and start a new one? Or let the old one finish, then start a new one? In the latter case, it can be as simple as using `SemaphoreSlim.WaitAsync`, check [this](https://blogs.msdn.microsoft.com/lucian/2014/03/03/async-re-entrancy-and-the-patterns-to-deal-with-it/). I myself usually take the second approach: https://stackoverflow.com/q/21424084/1768303 – noseratio Feb 14 '19 at 00:23
  • @noseratio thanks. In my actual case, UI workflow is very protective, they click the button it disables the button, until the function is done. So there would be no way a click or two clicks of the button would run two simultaneously so I guess i'm ok re that. – barlop Feb 14 '19 at 00:25
  • ...finally, you can block the main UI with a modal "please wait" dialog, which is very user unfriendly but is still an option sometimes, when dealing with legacy code: https://stackoverflow.com/a/47934541/1768303 – noseratio Feb 14 '19 at 00:25
  • @barlop, disabling the button might be OK as long as you are sure there're no other implicit paths to invoke `SomeHandler`. I'd use a simple `this._isSomeHandlerExecuting`-style check for re-entracy inside `SomeHandler`. – noseratio Feb 14 '19 at 00:27
  • 1
    @noseratio ah ok, so re-entrancy isn't necessarily a problem, only if that re-entrancy was unplanned / clashes / wasn't tested for. Good to know. Thanks – barlop Feb 14 '19 at 00:43
  • 1
    Please delete the bit about `Application.DoEvents()` - it was only the old way in VB6. It was never the way in .NET. – Enigmativity Feb 14 '19 at 02:59
  • 1
    @barlop - Re-entrancy is a serious problem with `DoEvents()` - it will manifest itself as a bug that appears in so seemingly unrelated code and you'll burn a whole day debugging before you realize it was `DoEvents()`. – Enigmativity Feb 14 '19 at 03:16
  • I suggest you update your answer 'cos it seems to be confusing people.. You could rename the old way to a bad way one shouldn't do, that was what people would do in VB6. Or even better, rather than say it's an old way, you could change it to `label1.Update();(as per han's comment)`then make a side note that using a DoEvents() line is an old way. – barlop Feb 14 '19 at 13:48