This is my previous question : Why when updating the label to display the timer counting backward the time in the label is jumping: 50 then 40 then 50 then 39 then 50 then 38?
I marked the answer and then made some changes in my code.
This is the Radar class. in the bottom of the class inside the method ProcessDownloadedImagesAsync i’m getting the downloaded files form the hard disk , there are 35 files images downloaded files and then I’m checking if the files names are not containing the string "_Processed" because I want to process each time only new downloaded files.
but the progressBar when he is getting to the end in a label I added in form1 that display the number of processed images show the number 70 twice double of the downloaded and processed images.
why it’s showing 70 ?
the second problem is when it’s first downloading the files when the dowlnoad is finished the progressbar is keep going on to 100% i mean there is sync/match between the progressbar progress percentages and the number of downloaded files. it should show the progress of only successfully downloaded files. there are 200 links of files but only 35 downloaded so it should show the progress of the 35 downloaded and not the whole 200.
this is the part in the Radar class where it’s doing the processing:
private async Task ProcessDownloadedImagesAsync()
{
Progress = 0;
List<string> filesToProcess = Directory.GetFiles(Folder, "*.png").ToList();
// Check if there are any files to process
if (filesToProcess.Count > 0)
{
for (int i = 0; i < filesToProcess.Count; i++)
{
if (!filesToProcess[i].Contains("_Processed"))
{
var filePath = Path.Combine(Folder, filesToProcess[i]);
State = RadarState.ImageProcessing;
// Process the downloaded image
RadarImagesConvertor convertor = new RadarImagesConvertor(filePath, Folder);
// Signal that the image has been processed
State = RadarState.ImageProcessed;
// Increment the progress based on the number of images processed
Progress = (int)((double)(i + 1) / filesToProcess.Count * 100);
}
// Add an await here
await Task.Delay(50);
}
}
}
and this is in form1 how I’m starting the downloading and the processing:
private void StartDownload()
{
_radar.PropertyChanged += (sender, args) =>
{
switch (args.PropertyName)
{
case nameof(_radar.State):
// Update the title bar
BeginInvoke((MethodInvoker)delegate
{
if (_radar.State == RadarState.ImageProcessed)
{
// Text = $"Radar - {_radar.State}";
successfulDownloadsCount++; // Increment the counter
lblImagesCounter.Text = $"Images Count: {successfulDownloadsCount}"; // Update the label
}
else if (_radar.State == RadarState.ImageProcessing)
{
Text = "Radar - ImagesProcessing";
}
else
{
Text = $"Radar - {_radar.State}";
}
});
break;
case nameof(_radar.Progress):
// Update the progress bar
if (!Disposing) BeginInvoke((MethodInvoker)delegate
{
_downloadProgress.Value = _radar.Progress;
});
break;
}
};
// Start timer
Task task = updateLabelAsync();
Disposed += async (sender, eventArgs) =>
{
await task;
task.Dispose();
};
}
in form1 this is the part where I update the label with the ocunting of the processed files: lblImagesCounter
if (_radar.State == RadarState.ImageProcessed)
{
// Text = $"Radar - {_radar.State}";
successfulDownloadsCount++; // Increment the counter
lblImagesCounter.Text = $"Images Count: {successfulDownloadsCount}"; // Update the label
}
and the radar class
using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.IO;
using System.Linq;
using System.Net.Http;
using System.Reflection.Emit;
using System.Runtime.CompilerServices;
using System.Text;
using System.Threading.Tasks;
using System.Windows.Forms;
using static Weather_Images.Form1;
namespace Weather_Images
{
public class Radar : INotifyPropertyChanged
{
public Radar(string inputFolder)
{
Folder = inputFolder;
}
public string Folder { get; }
public string DefaultLink { get; } = "https://ims.gov.il/sites/default/files/ims_data/map_images/IMSRadar4GIS/IMSRadar4GIS_";
public event PropertyChangedEventHandler PropertyChanged;
public RadarState State
{
get => _state;
set
{
if (!Equals(_state, value))
{
_state = value;
OnPropertyChanged();
}
}
}
RadarState _state = default;
public int Progress
{
get => _progress;
set
{
if (!Equals(_progress, value))
{
_progress = value;
OnPropertyChanged();
}
}
}
int _progress = default;
int successfulDownloadsCount = 0;
private void OnPropertyChanged([CallerMemberName] string propertyName = null)
{
PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(propertyName));
}
public async Task ExececuteAsync()
{
State = RadarState.Initializing;
PrepareLinks();
await DownloadImagesAsync();
await ProcessDownloadedImagesAsync();
State = RadarState.Waiting;
}
public List<string> Links { get; } = new List<string>();
public Dictionary<string, string> LinksAndFileNames { get; } = new Dictionary<string, string>();
public void PrepareLinks()
{
LinksAndFileNames.Clear();
Links.Clear();
GenerateRadarLinks();
// Exclude links for files that already exist in the folder
foreach (var existing in Directory.GetFiles(Folder, "*.png").Select(Path.GetFileNameWithoutExtension))
{
var link = Links.FirstOrDefault(_ => _.Contains(existing));
if (link != null)
{
Links.Remove(link);
}
}
}
private void GenerateRadarLinks()
{
DateTime current = RoundDown(DateTime.Now, 1);
if (!Directory.Exists(Folder + "\\Dates"))
{
Directory.CreateDirectory(Folder + "\\Dates");
}
using (StreamWriter w = new StreamWriter(Folder + "\\Dates\\" + "dates.txt"))
using (StreamWriter ww = new StreamWriter(Folder + "\\Dates\\" + "datesTime.txt"))
{
for (int i = 0; i < 200; i++)
{
var date = current.ToString("yyyyMMddHHmm");
ww.WriteLine(current.ToString());
w.WriteLine(date);
var link = DefaultLink + date + "_0.png";
Links.Add(link);
LinksAndFileNames.Add(link, current.ToString("yyyy_MM_dd_HH_mm"));
current = current.AddMinutes(-1);
}
}
}
private DateTime RoundDown(DateTime dt, int NearestMinuteInterval) =>
new DateTime(dt.Year, dt.Month, dt.Day, dt.Hour, dt.Minute / NearestMinuteInterval * NearestMinuteInterval, 0);
private async Task DownloadImagesAsync()
{
Progress = 0;
int totalFiles = Links.Count;
int completedFiles = 0;
if (Links.Any())
{
// Signal that the actual download has started
State = RadarState.Downloading;
using (HttpClient client = new HttpClient())
{
foreach (var link in Links)
{
var fileName = LinksAndFileNameshttps://stackoverflow.com/questions/77627775/why-the-progressbar-progress-is-not-accurate-with-the-number-of-files-and-how-to;
var filePath = Path.Combine(Folder, fileName + ".png");
try
{
// Check if the file already exists
if (!File.Exists(filePath))
{
// Download the file
byte[] fileData = await client.GetByteArrayAsync(link);
// Save the file
File.WriteAllBytes(filePath, fileData);
}
}
catch (Exception ex)
{
// Handle download errors if necessary
Console.WriteLine($"Error downloading file: {ex.Message}");
}
completedFiles++;
Progress = (int)((double)completedFiles / totalFiles * 100);
}
}
}
// Notify completion
State = RadarState.DownloadCompleted;
Progress = 100;
await Task.Delay(2000);
}
private async Task ProcessDownloadedImagesAsync()
{
Progress = 0;
List<string> filesToProcess = Directory.GetFiles(Folder, "*.png").ToList();
// Check if there are any files to process
if (filesToProcess.Count > 0)
{
for (int i = 0; i < filesToProcess.Count; i++)
{
if (!filesToProcess[i].Contains("_Processed"))
{
var filePath = Path.Combine(Folder, filesToProcess[i]);
State = RadarState.ImageProcessing;
// Process the downloaded image
RadarImagesConvertor convertor = new RadarImagesConvertor(filePath, Folder);
// Signal that the image has been processed
State = RadarState.ImageProcessed;
// Increment the progress based on the number of images processed
Progress = (int)((double)(i + 1) / filesToProcess.Count * 100);
}
// Add an await here
await Task.Delay(50);
}
}
}
}
}
Even if I weren’t already familiar with the sample from the previous answer, there are two problems that jump out from the code that you are showing in this post.
Problem 1: Incorrect basis for Progress calculation
You want progress to report as 0-100, but you’re basing the calculation on the total number of files in Folder. You say that having “_Processed” in the name disqualifies it as a “file to process”. So you would want to make sure to filter that up front in order to get the correct number of files to process. Then the bindable Progress property will report the correct percentage.
private async Task ProcessDownloadedImagesAsync()
{
Progress = 0;
List<string> filesToProcess =
Directory.GetFiles(Folder, "*.png")
.Where(_ => !_.Contains("_Processed"))
.ToList();
// Check if there are any files to process
if (filesToProcess.Any())
{
for (int i = 0; i < filesToProcess.Count; i++)
{
var filePath = filesToProcess[i]; // Already a fully-qualified path
State = RadarState.ImageProcessing;
// Process the downloaded image
RadarImagesConvertor convertor = new RadarImagesConvertor(filePath, Folder);
// Signal that the image has been processed
State = RadarState.ImageProcessed;
// Increment the progress based on the number of images processed
Progress = (int)((double)(i + 1) / filesToProcess.Count * 100);
}
}
}
Problem 2: Multiple event subscription.
Every time you call this method, you’re subscribing again (and again) to this event.
private void StartDownload()
{
_radar.PropertyChanged += (sender, args) =>
.
.
.
}
So let’s look at the trouble that’s going to cause. This code is designed to increment the button text by one on every click. But then, pathologically, we subscribe to the click event 3 times. Now, every click increments the count by 3.
public partial class MainForm : Form
{
public MainForm() =>InitializeComponent();
int _count = 0;
protected override void OnLoad(EventArgs e)
{
base.OnLoad(e);
buttonClickCounter.Click += onButtonClickCounter;
buttonClickCounter.Click += onButtonClickCounter;
buttonClickCounter.Click += onButtonClickCounter;
}
private void onButtonClickCounter(object? sender, EventArgs e)
{
_count++;
var pluralize = _count == 1 ? "time" : "times";
buttonClickCounter.Text = $"Clicked {_count} {pluralize}";
}
}
From your comment:
I’m calling the StartDownload only once in the button click event
“Subscribing in the click event” makes it closer to this snippet:
public partial class MainForm : Form
{
public MainForm() =>InitializeComponent();
int _count = 0;
protected override void OnLoad(EventArgs e)
{
base.OnLoad(e);
buttonClickCounter.Click += onButtonClickCounter;
}
private void onButtonClickCounter(object? sender, EventArgs e)
{
_count++;
buttonClickCounter.Click += onButtonClickCounter;
var pluralize = _count == 1 ? "time" : "times";
buttonClickCounter.Text = $"Clicked {_count} {pluralize}";
}
}
And if it makes things any clearer here are two expressions that do the same thing.
Subscribe with anonymous delegate
private void StartDownload()
{
_radar.PropertyChanged += (sender, args) =>
{
// Do something
};
}
Subscribe to named method
private void StartDownload()
{
_radar.PropertyChanged += onRadarPropertyChanged;
}
void onRadarPropertyChanged(object? sender, EventArgs e)
{
// Do something
}
In the first case, it might look like something that would “go away” when the method goes out of scope. It isn’t.
35 files of 200 is 17.5 %. If you want the progess to go from 0% to 17.5 % only, then count the number of sucessfully processed files and base the percentage calculation on this counter instead of
i.Please reduce the code to the minimum necessary to reproduce the problem.
I would recommend following the standard approach for background work. 1) Use a Task.Run to run it on a background thread. 2) Use
IProgress<T>to report progress, I prefer to use a double in the 0-1 range, so the caller does not need to know the number of items. 3) use a cancellation token to allow the work to be cancelled. Tightly coupling your worker methods to the UI the way you have done seem like a bad idea.You’re basing it on the total number of files, not the number of “files to process”. Do your filter up front:
List<string> filesToProcess = Directory.GetFiles(Folder, "*.png").Where(_=>!_.Contains("_Processed")).ToList();You also subscribed again to the
PropertyChangedevent inStartDownload. Terrible, terrible. Don’t do that. I happen to know (but not from the code you posted here) that you subscribed to that event in theMainForm.OnLoad. That is sufficient and that’s why you’re doubling up on some things.Show 4 more comments