views:

251

answers:

2

I have a ProgressBarWindow which has a progressbar and a cancel button on it which I use to report progress on file I/O. However, the UI Thread of the ProgressBarWindow and my main window both hang despite all the work being done in a backgroundworker. The progressbar is rendered, as is my main window, but does not update whilst the backgroundworker does its thing. The following code is called at the very end of the constructor of the main window:

iCountLogLinesProgressBar = new ProgressBarWindow();
iCountLogLinesProgressBar.cancelButton.Click += EventCountLogLinesProgressBarCancelButtonClicked;
iCountLogLinesProgressBar.Show();

iCountLogRecords = new BackgroundWorker();
iCountLogRecords.DoWork += EventCountLogLinesDoWork;
iCountLogRecords.ProgressChanged += EventCountLogLinesProgressChanged;
iCountLogRecords.RunWorkerCompleted += EventCountLogLinesRunWorkerCompleted;
iCountLogRecords.WorkerReportsProgress = true;
iCountLogRecords.WorkerSupportsCancellation = true;
iCountLogRecords.RunWorkerAsync(new BinaryReader(File.Open(iMainLogFilename, FileMode.Open, FileAccess.Read)));

EventCountLogLinesProgressChanged() looks like this:

private void EventCountLogLinesProgressChanged(object sender, ProgressChangedEventArgs e)
{
    iCountLogLinesProgressBar.Value = e.ProgressPercentage;
}

Here is a shortened version of ProgressBarWindow(the rest is just a couple of setters):

public partial class ProgressBarWindow : Window
{
    public ProgressBarWindow()
    {
        InitializeComponent();
        this.progressBar.Value = this.progressBar.Minimum = 0;
        this.progressBar.Maximum = 100;
    }

    public double Value
    {
        get
        {
            return progressBar.Value;
        }
        set
        {
            this.progressBar.Value = value;
        }
    }
} 

I've tried wrapping the value setter line in a dispatcher.invoke delegate but that gives me a stack overflow(I shouldn't have to have a dispatcher.invoke line anyway as backgroundworker calls ProgressChanged in the UI Thread, right?). I have checked msdn and googled but I can't seem to find anyone else with this problem.

EDIT Apologies, I did not realise my simplified code blocked the UI Thread, I get exactly the same behaviour despite using a backgroundworker so I erroneously assumed they were equivalent. I should have mentioned I was using a backgroundworker :P

+5  A: 

You're blocking the UI thread, so it won't re-render the UI until your loop has finished.

Move the background processing into a separate thread, and use appropriate Dispatcher calls (or BackgroundWorker) to marshal UI update calls back to the UI thread.

If your progress bar is really just meant to be a timer, you could use one of the Timer classes to update it.

EDIT: Okay, now you've changed the code, it looks okay to me. It should be thread-safe as you're only changing the UI on the UI thread. Is your background worker definitely reporting progress periodically?

Jon Skeet
I have a backgroundworker above, but do I also need a dispatcher.invoke call for my value setter?
teflon19
@teflon19: No, you don't need to do anything else - except make sure you're reporting progress in the background worker.
Jon Skeet
I'm reporting progress only when progress changes, too many progresschanged events were slowing things down, but the UI still freezes. Is there any reason why the UI Thread of a ProgressBarWindow would be affected by a backgroundworker?
teflon19
@teflon19: Well if you're updating your UI so often that it doesn't actually get a chance to repaint, that would certainly be a problem. Why not filter it so that you only report progress if (say) it's an actual change in percentage when rounded to an integer. That way you can have at most 100 updates.
Jon Skeet
Yeah I've just found out that my UI is only getting one progress change reported to it(progress == 100%). For some reason it only updates once. I'm gonna have a closer look at that. Looks like the issue above was a bit of a red herring. Thanks for the help anyway!
teflon19
+3  A: 

Listen to Jon Skeet, or you could sometimes call Application.DoEvents() to make all UI updates in the same thread.

Alexander
-1: Calling Application.DoEvents directly is bad practice. It causes the UI thread to enter its message pumping procedure recursively which can lead to some nasty problems. Much better to write your application correctly and allow the UI thread to pump for messages normally.
Alex Humphrey
It depends on the scale. If it's a small desktop app that does a simple update for example, you could spend 15 minutes writing it with Application.DoEvents() in one thread, or complicate it, change simple method calls to method invokes.I just enumerated one more option, specifying that Jon Skeet's option is better in most cases. So I got a down vote for enumerating an option. Nice.
Alexander
@Alexander: it is my opinion that in it's current form your answer could cause developers reading the question to develop bad habits by using Application.DoEvents for as a silver bullet for their UI update woes (as many do). I just would *never* use Application.DoEvents for the purpose in question. Yes, listening to Jon Skeet appears to be good advice in this instance, and you just enumerated one more option, but in my opinion it's a bad option. But that's just the opinion of one individual, which is reflected in your answer's score.
Alex Humphrey
Sorry. I'm just used that -1 goes for a bad, wrong answer as a whole. I just didn't know that enumerating one working (but far from best) option would qualify for this as well.
Alexander