11

I have a vb.net based windows application, where when "GO" button is clicked a bunch of data is loaded into DB. So in my application as soon as "GO" button is clicked I want to just disable it and would like to enable it back when the uploading has completed. Now in my specific method for btnGo_Click() I have:

btnGo.Enabled = False

as first line and

btnGo.Enabled = True

as last line in the same method.

But I fail to understand why the "GO" though appears as being disabled still allows click when processing is going on. Also if I remove the last line, it gets disabled permanently and doesn't allow the click event.

Kindly suggest what am I doing wrong?

Edit (Dated: 25th Jan 2012): I made changes as suggested by our collegues, but I am facing a new issue here. I am facing an issue where the textbox gets updated but not always. I have updated my textbox in "_ProgressChanged" event of the background worker thread. In my case if there is 10 records uploaded. Then there are 10 lines of updates that are expected in the texbox. But only few lines are shown in the textbox. Is it the repaint issue again? Kindly suggest...Because all other things are done as per your suggestion

Justin Samuel
  • 1,063
  • 4
  • 16
  • 30

8 Answers8

19

You're not doing anything wrong. The problem is that the UI doesn't get updated until the code inside of your event handler method finishes executing. Then, the button is disabled and immediately enabled in rapid sequence.

That explains why if you forget to reenable the button control at the end of the event handler method, it is still disabled—because you told it to disable the button in the first line of the method.

This is a classic case of why you should never perform long-running computational tasks inside of an event handler method, because it blocks the UI from being updated. The computation actually needs to happen on a separate thread. But don't try to create the thread manually, and definitely don't try to update your UI from a separate thread. Instead, use the BackgroundWorker component to handle all of this for you automatically. The linked MSDN documentation has a great sample on how to use it.

Disable the button before starting the BackgroundWorker, and then re-enable it in its Completed event, signaling the completion of your database load.

Cody Gray - on strike
  • 239,200
  • 50
  • 490
  • 574
  • 1
    I agree. Using a Backgroundworker is the way to go here. It will keep the UI reponsive. – BenR Jan 23 '12 at 17:14
  • Hi Cody, Nice article. I have a query here: I have a textbox which gets updated which the uploading(DB update) is happening. So should I move this particular code into "backgroundWorker1_ProgressChanged" event handler? Because otherwise in "backgroundWorker1_DoWork" event handler it throws me an exception of "Cross Thread usage..." – Justin Samuel Jan 24 '12 at 16:02
  • 1
    @Justin: Yes, if you want to respond to progress updates, that code needs to go into the `ProgressChanged` event handler. The `DoWork` event handler method is where you do what should happen on the background thread. The thing you have to remember is that you can only interact with your UI from a single thread (the primary one, designated the UI thread). Everything else can't touch it, or you'll get an exception about cross-threading. The `BackgroundWorker` makes this separation easy, because you can just put code in the event handlers and let it worry about raising it in the correct thread. – Cody Gray - on strike Jan 24 '12 at 18:32
  • @Cody - I am facing an issue here where the textbox gets updated but not always. Like in my case if there is 10 records uploaded. Then there are 10 lines of updates that are expected in the texbox. But only few lines shown in the textbox. Is it the repaint issue again? Kindly suggest...Bcos all other things are done as per your suggestion and the supporting URL. – Justin Samuel Jan 25 '12 at 10:56
  • @Justin: No, that doesn't sound like a painting problem. Any painting problem would be corrected as soon as everything finished, and you'd be able to see all 10 records. If that's not happening, something's wrong with the code you're using to add them to the textbox. Perhaps an off-by-one error in the loop? Try debugging it by using plain numbers instead of records so you can see what order the numbers are being added to the textbox. If that fails, update your question with the actual code that you're using. – Cody Gray - on strike Jan 25 '12 at 17:07
  • I will try one more time and otherwise will post a separate question. In any case thanks for your support. It was indeed very helpful. I will post my new link also here! :) – Justin Samuel Jan 25 '12 at 17:17
  • @Cody: I could solve the issue by adding Thread.Sleep(25). But is this the correct way of doing this? I have raised a separate question for this on stackoverflow: http://stackoverflow.com/questions/9019475/vb-net-winform-application-based-on-backgroundworker-thread-concept-ui-update . Kindly suggest. – Justin Samuel Jan 26 '12 at 14:27
  • @Cody - Hey Cody- Got the solution. As Chris suggested in the other link I posted, the problem was with the same UIUpdater object being used across all of these ProgessUpdates calls. Which indirectly nullifies threading concept the way I have written my code. Cheers! :) – Justin Samuel Jan 26 '12 at 15:24
  • @Justin: Very glad to hear you found something other than `Thread.Sleep`. That might be useful for testing, but it definitely isn't a "solution" to a problem. I upvoted Chris's answer to the other question! – Cody Gray - on strike Jan 26 '12 at 17:53
  • @Cody - Yes very correct. Infact I knew that Thread.Sleep was not the proper methodology. Hence I asked whether what I did was correct! Saw the upvote and now I got to know it was you who upvoted! Thanks :) – Justin Samuel Jan 27 '12 at 11:22
5

Since you're trying to execute a function that can take some time, I'd advise you to make use of threading. In .NET there's a BackgroundWorker component which is excellent for performing tasks asynchronous.

On button click, invoke the BackgroundWorker like this:

if not bgwWorker.IsBusy then
   btnGo.enabled = false
   bgwWorker.RunWorkerAsync()
end if 

And use the completed event to enable the button again:

Private Sub bgwWorker_DoWork(ByVal sender As System.Object, _
                 ByVal e As System.ComponentModel.DoWorkEventArgs) _
                 Handles bgwWorker.DoWork
' Do your things    
End Sub

Private Sub bgwWorker_RunWorkerCompleted(ByVal sender As System.Object, _
                         ByVal e As System.ComponentModel.RunWorkerCompletedEventArgs) _
                         Handles bgwWorker.RunWorkerCompleted
' Called when the BackgroundWorker is completed.
btnGo.enabled = true
End Sub

In the example above, I've used bgwWorker as the instance of a BackgroundWorker.

Rhapsody
  • 6,017
  • 2
  • 31
  • 49
  • Hi Rhapsody, Nice article. I have a query here: I have a textbox which gets updated which the uploading(DB update) is happening. So should I move this particular code into "backgroundWorker1_ProgressChanged" event handler? Because otherwise in "backgroundWorker1_DoWork" event handler it throws me an exception of "Cross Thread usage..." – Justin Samuel Jan 24 '12 at 16:02
  • Yes, set the WorkerReportsProgress property of the BackgroundWorker to true and use bgwWorker.ReportProgress in combination with the ProgressChanged event to update the User Interface. – Rhapsody Jan 24 '12 at 17:15
  • I am facing an issue here where the textbox gets updated but not always. Like in my case if there is 10 records uploaded. Then there are 10 lines of updates that are expected in the texbox. But only few lines shown in the textbox. Is it the repaint issue again? Kindly suggest...Bcos all other things are done as per your suggestion – Justin Samuel Jan 25 '12 at 10:58
  • I suggest you create a new question here at SO with some code. Like that it's easier for 'us' to help :-) (or you could update this question) – Rhapsody Jan 25 '12 at 11:05
  • I will try one more time and otherwise will post a separate question. In any case thanks for your support. It was indeed very helpful. I will post my new link also here! :) – Justin Samuel Jan 25 '12 at 17:16
  • I could solve the issue by adding Thread.Sleep(25). But is this the correct way of doing this? I have raised a separate question for this on stackoverflow: http://stackoverflow.com/questions/9019475/vb-net-winform-application-based-on-backgroundworker-thread-concept-ui-update . Kindly suggest. – Justin Samuel Jan 26 '12 at 14:28
  • Hey Rhapsody - Got the solution. As Chris suggested in the other link I posted, the problem was with the same UIUpdater object being used across all of these ProgessUpdates calls. Which indirectly nullifies threading concept the way I have written my code. Cheers! :) – Justin Samuel Jan 26 '12 at 15:23
1

The button click event is handled as soon as the UI thread has idle time. After you disable your button, the UI thread is keept busy by your code. At the end of your method, you re-enable the button, and after that you exit the method and allow for idle time. As a consequence, the button will already be enabled at the point in time where the click event is handled, so your click is "recognized".

The solution is, as others already suggested, to use a Backgroundworker.

Dont try to use doEvents() as a solution (never do), since this would be prone to introduce other subtle problems. That said, you can prove the above explanation with some experimental doEvents in your code. You will see that the click is discarded if a doEvents is performed before the button gets re-enabled. On the other hand, performing a doEvents directly after the button.disable (to "update the GUI") will not help if it is executed before the click.

tdanker
  • 41
  • 2
0

It's usually not a good idea to manage the state of a submit button. Instead, perform validation on submit.

I Stand With Russia
  • 6,254
  • 8
  • 39
  • 67
0

I had a slightly different issue not being able to call click. I have a routine that turns the UI on/off based on a validation routine.

  • i will say that I disagree w/ the suggestion to do validation in the submit. The button should not be enabled if we are able to tell the form is invalid.

My issue was that I was calling the validation from several places. One of the calls was the CustomCellDraw event of a grid which was firing very frequently.

So while it appeared that I was simply disabling/enabling the button a few times, I really was doing this almost continually.

I was able to trouble shoot by placing a label on the form and kind of doing a console.log thing. I immediately realized button.Enabled was flickering, which led me down the correct trouble shooting path.

I realize this addresses a different root cause than op described. But it does address the problem the op describes.

Eric Aya
  • 69,473
  • 35
  • 181
  • 253
greg
  • 1,673
  • 1
  • 17
  • 30
0

I would like to add 2 enhancements to the answer generally described here which is to 'do the work in another thread'.

  1. Ensure button.enable=true always gets called
    1.a. You should use a try block in button_click . If there is an error in launching the thread, CATCH should re-enable the button.

    1.b. The task complete call back should also ensure the button is enabled using try/catch/finally 1.c The task timeout should also re-enable the button

  2. A common error based on exactly the situation described here is rapid-clicker-person clicks the button twice in rapid succession. This is possible because its possible, even if unlikely, that 2 click messages get queued and processed before the button is disabled. You can not assume the events happen synchronously.

    IMHO a best practice is to use a static variable. Initialize it to 0. Set it to one as the very first statement and set it to 0 following the guidelines in POINT 1.

    The second statement in button click should simply RETURN/EXIT if the value > 0

    If you are using a worker thread, the static variable may have to be located in that code. I would not advise making it a form level variable.

greg
  • 1,673
  • 1
  • 17
  • 30
0

If your btnGo_Click() is ran inside main thread, UI could not be updated correctly inside a time-consuming task.
The best way you can do what you need is running your method in a BackgroundWorker.

Marco
  • 56,740
  • 14
  • 129
  • 152
  • I just don't understand why people are still suggesting `Application.DoEvents`. Yes, it might work here. No, it's not a good idea. No, it's not a good thing for someone who doesn't understand threading, re-entrancy, and Windows message loops to use in their code. Its apparent simplicity belies its truly complex and potentially difficult realities. It's particularly harmful to suggest it here, where people have a well-known tendency to copy paste code found in answers directly into their project without understanding how and why it works, or what the pitfalls are of using it. – Cody Gray - on strike Jan 23 '12 at 17:17
  • @CodyGray: I'm sorry for your comment: I know `Application.DoEvents` is not good, infact first think I suggested was the use of a Backgroundworker. My example was just to tell OP that can be done, even if it's not good... I gonna edit my post... – Marco Jan 23 '12 at 17:20
0

I just tried disabling a button, Updateing the form, Sleeping, and enabling it again. It still performed the click (A click that was done while it "slept" with the button disabled) after it was enabled.

I guess forms "remember" clicks.

(EDIT: I did this in C#.)

ispiro
  • 26,556
  • 38
  • 136
  • 291
  • @downvoter Care to explain why? My answer is the first to actually answer the question. (And not supply erroneous information blaming it on the UI not getting updated.) – ispiro Jan 23 '12 at 18:31
  • How does this answer the question? I don't understand how the "test" you performed proved anything at all, much less demonstrated that the problem is not a result of blocking the UI. – Cody Gray - on strike Jan 23 '12 at 19:03
  • The question was what is he doing wrong that the button "though appears as being disabled still allows click". The answer is (probably) – he's not doing anything wrong. That's how forms work. As for the UI, although that was my first thought as well – a) `Update` failed to rectify the situation. b) The OP writes: "appears as being disabled ". – ispiro Jan 23 '12 at 19:18
  • What do you think that `Update` does? It requires a message pump, and messages aren't getting pumped while the code in the event handler method is running. – Cody Gray - on strike Jan 23 '12 at 20:21
  • The `Update` causes the button to become gray. (Tested.) _That_ is the point. To _disable_ the button. The OP doesn't have any problem with a blocked UI. His problem is that the button _does_ receive messages, not that it doesn't. – ispiro Jan 23 '12 at 20:43