Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add IProgress<int> to ProgressBar control #17521

Closed
vpenades opened this issue Nov 15, 2024 · 11 comments
Closed

Add IProgress<int> to ProgressBar control #17521

vpenades opened this issue Nov 15, 2024 · 11 comments

Comments

@vpenades
Copy link

vpenades commented Nov 15, 2024

Is your feature request related to a problem? Please describe.

ProgressBar control is used to display the progress of a task.

.Net has an interface, IProgress that is used as an abstracion of an UI object displaying the progress, so applications can report the progress of a task in a platform agnostic way.

So it would make sense that ProgressBar implements IProgress

Describe the solution you'd like

class ProgressBar : IProgress<ProgressChangedEventArgs>
{
    void IProgress<ProgressChangedEventArgs>.Report(ProgressChangedEventArgs progress)
    {
        // we may receive the progress report from another thread.
        Avalonia.Threading.Dispatcher.UIThread.Invoke(() => this.Value = progress.ProgressPercentage);
    }
}

Describe alternatives you've considered

Right now I'm using a wrapper:

class ProgressBarWithInterface : IProgress<int>
{
   private ProgressBar bar;

   void IProgress<int>.Report(int progress)
    {
        // we may receive the progress report from another thread.
        Avalonia.Threading.Dispatcher.UIThread.Invoke(() => bar.Value = progress);
    }
}

Additional context

.Net

Also has these types:

  • IProgress<T> interface
  • Progress<T> class
  • System.ComponentModel.ProgressChangedEventHandler
  • System.ComponentModel.ProgressChangedEventArgs

I think all these types are greatly underated, and ProgressBar could be made aware of them in some way.

No response

@robloo
Copy link
Contributor

robloo commented Nov 15, 2024

You are thinking about this in reverse I think. Any long running tasks or actions can report using IProgress.

However, the ProgressBar itself isn't a long running action. It is just a representation of progress and doesn't have any values of its own. It isn't doing any work, only displaying other work.

So ProgressBar can consume IProgress information. However, it doesn't report it itself, it needs to come from a lower layer.

If you application is pulling progress back out of a ProgressBar and using it elsewhere you have something designed wrong. There should be a common task in a lower layer that all code can consume progress information for. One of the consumers being the ProgressBar.

@vpenades
Copy link
Author

@robloo the IProgress is to be implemented in the controls displaying it, and consumed by the tasks.

the pseudocode would look like this:

View:

class ProgressBar : IProgress<int>
{
   // update the progress value of the progress bar
   public void Report(int value) { this.Value = value; }
}

MVVM

class SomeMVVM
{
   public async Task DoSomeExpensiveJob(IProgress<int> progressFeedback)
   {
       for(int i=0; i < 100; ++i)
       {
            progressFeedback.Report(i);
            await Task.Pause(1000);
       }
   }
}

that way, I can call DoSomeExpensiveWork, passing the ProgressBar control directly, to update the progress bar as the job progresses.

@rabbitism
Copy link
Contributor

that way, I can call DoSomeExpensiveWork, passing the ProgressBar control directly, to update the progress bar as the job progresses.

First of all, accessing visual element inside VM is bad practice.
Secondly you just need

class SomeMVVM
{
   public async Task DoSomeExpensiveJob(ProgressBar bar)
   {
       for(int i=0; i < 100; ++i)
       {
            bar.Value = i;
            await Task.Pause(1000);
       }
   }
}

@vpenades
Copy link
Author

DoSomeExpensiveJob(ProgressBar bar)

To do that you have to reference Avalonia in your MVVM project, with IProgress, you don't need to.

@rabbitism
Copy link
Contributor

passing the ProgressBar control directly

Hmm, that's you said passing the ProgressBar control directly, we usually update a number in ViewModel, and bind progressbar value to that value, no direct access.

@thevortexcloud
Copy link
Contributor

thevortexcloud commented Nov 15, 2024

I will also add that using a progress bar itself to track progress rather than the underlying value is very odd. As mentioned it also breaks MVVM by having the progress bar be passed to the view model, giving the view model knowledge of the view. Even if it's only an interface, it's still not great. Typically the view reads the view model, not the other way around.

@timunie
Copy link
Contributor

timunie commented Nov 16, 2024

@vpenades why not having a property Percentage which you update from your task and which is bound to the Progressbar as any other value?

@vpenades
Copy link
Author

Everybody answering me it's assuming I have full control over the code, it's outdated, closed source libraries I'm calling.

Also everybody is assuming mvvm while I'm actually doing this in code behind, that is, calling a background task that takes a iprogress interface as a parameter, on an event

So that forces me to write a proxy object that implements iprogrees, so i can pass it as an argument, and updates the progressbar

@thevortexcloud
Copy link
Contributor

thevortexcloud commented Nov 17, 2024

Also everybody is assuming mvvm

Well you did give an MVVM example.

So that forces me to write a proxy object that implements iprogrees, so i can pass it as an argument, and updates the progressbar

I don't see a problem with this. If you really want to do this, then this is the correct approach. With that said: You could even rewrite your own app's code to use MVVM, even with closed source code you can't modify. After all a view model is meant to wrap a model and not do very much on its own. You just need to pass a Progress and subscribe to the events from it, and then change your VM's progress property which will be bound to the progress bar.


In any case, there are some possible technical problems with this. IProgress takes a generic argument as it does not really care what the progress report value is. The actual percentage of ProgressBar is implemented as a double.

/// <summary>
/// Gets the overall percentage complete of the progress
/// </summary>
/// <remarks>
/// This read-only property is automatically calculated using the current <see cref="RangeBase.Value"/> and
/// the effective range (<see cref="RangeBase.Maximum"/> - <see cref="RangeBase.Minimum"/>).
/// </remarks>
public double Percentage

The actual value of a progress bar is also a double.

/// <summary>
/// Gets or sets the current value.
/// </summary>
public double Value
{
get => GetValue(ValueProperty);
set => SetValue(ValueProperty, value);
}

Your example explicitly shows an integer. Assuming that is what your third party code wants, then you might need to wrap it regardless.

Also what would the report value even be: Value or Percentage? Because those can be two very different numbers and there are situations where you would want one or the other.

This is also going to encourage "bad" code, especially for people using MVVM. As they will be tempted to just directly pass a reference to the progress bar to the VM instead of using the existing binding system which solves this problem already.

@robloo
Copy link
Contributor

robloo commented Nov 17, 2024

Note that even if you have a special case with existing code or other restraints, we cannot change the framework to accommodate this. It's against the MVVM design principles that the framework does need to follow here. If you want to deviate that's fine but it must be in your app only.

I think this should be closed as by design.

@vpenades
Copy link
Author

vpenades commented Nov 17, 2024

My point of view is this:

  • IProgress is an interface to be implemented by objects receiving progress status.
  • ProgressBar is a control that displays progress status, thus, it has a mechanism to receive the progress status, through the "value" property

So I was not seeing any harm for ProgressBar to implement an IProgress interface. To me, IProgress is almost an abstraction of a progress bar. Implementing it does not affect the current behavior and ony adds new ways of using the ProgressBar, its up to developers to use it or not.

But certainly I failed to make my point, so closing.

@AvaloniaUI AvaloniaUI locked and limited conversation to collaborators Nov 18, 2024
@timunie timunie converted this issue into discussion #17547 Nov 18, 2024

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Projects
None yet
Development

No branches or pull requests

5 participants