Skip to content

Added CancellationTokenSource to ProgressDialog #54

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

Merged
merged 2 commits into from
Sep 9, 2021
Merged

Added CancellationTokenSource to ProgressDialog #54

merged 2 commits into from
Sep 9, 2021

Conversation

vpenades
Copy link
Contributor

@vpenades vpenades commented Sep 4, 2021

Also Added IServiceProvider to ProgressDialog, so we can retrieve CancellationTokenSource from 'Sender' event argument.

@CLAassistant
Copy link

CLAassistant commented Sep 4, 2021

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@augustoproiete augustoproiete left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR 👍. Added some comments

@augustoproiete augustoproiete linked an issue Sep 5, 2021 that may be closed by this pull request
@augustoproiete augustoproiete added the enhancement New feature or request label Sep 6, 2021
@augustoproiete augustoproiete marked this pull request as draft September 6, 2021 21:13
@augustoproiete
Copy link
Member

LMK when it's ready for review again

@vpenades vpenades marked this pull request as ready for review September 7, 2021 07:27
Copy link
Member

@augustoproiete augustoproiete left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good and almost there. Added a couple comments

@augustoproiete augustoproiete marked this pull request as draft September 7, 2021 16:18
@vpenades
Copy link
Contributor Author

vpenades commented Sep 7, 2021

I've considered adding the cancellationToken to the Show / ShowDialog but there's quite a lot of methods like these, are you sure you want to add the CancellationToken to every one of these methods?

@augustoproiete
Copy link
Member

Yes... Because calling Show or ShowDialog causes the background worker to run, I see that as a "job", and each job could have its own CancellationToken, so having it as an argument for Show / ShowDialog seems like a good direction to go. The lifecycle of the CancellationTokenSource linked would also match the job execution lifecycle.

My initial thinking was to add new overloads (rather than include the CancellationToken with a default value) so that it doesn't break anyone that get the package transitively, but I'm also happy to release this as a major version.

So either:

Ookii.Dialogs.Wpf v3.2.0

Show()
+Show(CancellationToken cancellationToken)

Show(object argument)
+Show(object argument, CancellationToken cancellationToken)

ShowDialog()
+ShowDialog(CancellationToken cancellationToken)

ShowDialog(Window owner)
+ShowDialog(Window owner, CancellationToken cancellationToken)

ShowDialog(IntPtr owner)
+ShowDialog(IntPtr owner, CancellationToken cancellationToken)

ShowDialog(Window owner, object argument)
+ShowDialog(Window owner, object argument, CancellationToken cancellationToken)

ShowDialog(IntPtr owner, object argument)
+ShowDialog(IntPtr owner, object argument, CancellationToken cancellationToken)

or

Ookii.Dialogs.Wpf v4.0.0

-Show()
+Show(CancellationToken cancellationToken = default)

-Show(object argument)
+Show(object argument, CancellationToken cancellationToken = default)

-ShowDialog()
+ShowDialog(CancellationToken cancellationToken = default)

-ShowDialog(Window owner)
+ShowDialog(Window owner, CancellationToken cancellationToken = default)

-ShowDialog(IntPtr owner)
+ShowDialog(IntPtr owner, CancellationToken cancellationToken = default)

-ShowDialog(Window owner, object argument)
+ShowDialog(Window owner, object argument, CancellationToken cancellationToken = default)

-ShowDialog(IntPtr owner, object argument)
+ShowDialog(IntPtr owner, object argument, CancellationToken cancellationToken = default)

@vpenades vpenades marked this pull request as ready for review September 8, 2021 22:04
@augustoproiete augustoproiete merged commit bf03130 into ookii-dialogs:master Sep 9, 2021
@augustoproiete
Copy link
Member

@vpenades your changes have been merged, thanks for your contribution 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

Add CancellationToken support to ProgressDialog
3 participants