Part of code doesn't execute without delay

Question

My app detect is ip changed or not. If ip is different than previous it saves new ip to file. Seems pretty easy.

Delay function:

public void MyDelay(int milliseconds)
{
    Stopwatch stopwatch = Stopwatch.StartNew();
    while (stopwatch.ElapsedMilliseconds <= (long)milliseconds)
    {
        Application.DoEvents();
    }
}

Button click event:

private void button1_Click(object sender, EventArgs e)
  {
    pictureBox1.BackColor = Color.Green;

    while (true)
    {
        while (true)    //force obtain new_ip
        {

            try

            {
                new_ip = new System.Net.WebClient().DownloadString("https://api.ipify.org");

            }

            catch
            {
                MyDelay(1000);
                continue;

            }
            break;


        }


        if (old_ip != new_ip)
        {
            pictureBox1.BackColor = Color.Red;
            File.AppendAllText(@"C:\users\pc\Desktop\ip.txt", new_ip + "\r\n");
            old_ip = new_ip;
            MessageBox.Show("New ip saved");
        }
        pictureBox1.BackColor = Color.Green;

    }
 }

The thing is if I left the code like this the part: pictureBox1.BackColor = Color.Green; is not going to execute.

I have to change from this:

pictureBox1.BackColor = Color.Green;

To this:

pictureBox1.BackColor = Color.Green;
MyDelay(1);

And now picturebox colour is changing. Why does that Delay repair that? If I put MessageBox insted of Picturebox, problem does not even exist. It work as a charm without MyDelay too. So what is the point?

Another question: it should check the ip every 1 second. So why when I change the ip I have to wait about 5 second for change detection? It should save new ip after 1 second when connection appears, but it takes about 5 second to recognize change

We can't tell you why `MyDelad` causes a behavior change unless you show us the code for `MyDelad`. Also, WebClient is a disposeable class, you should be disposing it after use. — Scott Chamberlain, Jul 22 '16 at 14:17
Sounds like you want to implement some proper threading / background workers. — CompuChip, Jul 22 '16 at 14:20
exactly what I thought. the problem lies on Application.DoEvents(); Try replacing your delay with this line and it should work — Steve, Jul 22 '16 at 14:21
@Steve what about his `MyDelay(1000)` then? Maybe `Thread.Sleep(1000)` is safe here — Khalil Khalaf, Jul 22 '16 at 14:22
@FirstStep I would say its bad code but it won't affect the result — Steve, Jul 22 '16 at 14:23
It is a grossly inappropriate way to call `pictureBox1.Update()`. [Read this](http://stackoverflow.com/a/5183623/17034). — Hans Passant, Jul 22 '16 at 14:26

score 1 · Answer 1 · answered Jul 22 '16 at 14:22

You are doing all your work in the main thread. Also the UI is being updated in the main thread. This means that you're not allowing the UI to update itself in any way.

When you call your delay function it calls Application.DoEvents() which lets the UI to update itself.

You should refactor the code to use a timer, asynchronous methods or a background worker thread so that the UI can stay responsive.

Jeroen van Langen · Accepted Answer · 2016-07-22T14:47:14.903

You should never create an endless while (true) in a button click which blocks the gui for updating. Thats why your PictureBox isn't repainted (but the code is executed). If you place your sleep there (containing Application.DoEvents()), the paint messages are handled and your PictureBox is repainted. But still the GUI thread is blocked.

Using the Application.DoEvents() this way is bad practice, because other buttons/formcloses are also handled here which makes your application very unstable. So get rid of the Application.DoEvents()..

Use a Timer instead to check the site for your external IP!

Something like: PSEUDO

public class Form1: Form
{
    private Timer _timer;

    public Form1()
    {
        InitializeComponent();
        _timer = new Timer(.....);
        _timer.Tick += Timer_Tick;
        // etc!!!
    }

}

private void Timer_Tick(object sender, EventArgs e)
{
    string new_ip;
    try
    {
        new_ip = new System.Net.WebClient().DownloadString("https://api.ipify.org");

    }
    catch 
    { 
        return;
    }

    if (old_ip != new_ip)
    {
        pictureBox1.BackColor = Color.Red;
        File.AppendAllText(@"C:\users\pc\Desktop\ip.txt", new_ip + "\r\n");
        old_ip = new_ip;
        MessageBox.Show("New ip saved");
    }
    else
        pictureBox1.BackColor = Color.Green;
}

score 0 · Answer 3 · edited May 23 '17 at 11:51

0

inside a snipped .net cannot update the interface, you can use Application.DoEvents() to solve the problem. Maybe this question can help further.

edited May 23 '17 at 11:51

Community

1
1

answered Jul 22 '16 at 14:21

Massimiliano Carosi

284
4
20

score 0 · Answer 4 · answered Jul 22 '16 at 15:01

If I understood your intentions correctly, you can try async await (not sure why it requires a while loop and try catch to get the result of the url though):

    private async void button1_Click(object sender, EventArgs e)
    {
        pictureBox1.BackColor = Color.Green;

        var newIp = await GetIpAsync();

        if (oldIp != newIp)
            pictureBox1.BackColor = Color.Red;
    }

    private async Task<string> GetIpAsync()
    {
        using (var httpClient = new HttpClient())
        {
            return await httpClient.GetStringAsync(@"https://api.ipify.org");
        }
    }

I added while loop, because I couldnt obtain ip when it was changing to another one. — audiophonic, Jul 22 '16 at 16:43

Part of code doesn't execute without delay

4 Answers4