Skip to content
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

Update RunWorkerCompletedEventArgs.xml #4548

Merged
merged 5 commits into from
Mar 20, 2018
Merged

Update RunWorkerCompletedEventArgs.xml #4548

merged 5 commits into from
Mar 20, 2018

Conversation

SimpleSamples
Copy link
Contributor

Title

What title? Is this the title? I am adding a note saying where the property comes from.

Summary

A few years ago I was irritated that the documentation is vague about where the property comes from.

Fixes no Issue_Number

Note: The "Fixes #nnn" syntax in the PR description causes
GitHub to automatically close the issue when this PR is merged.
Remove that line if you don't have issues associated with this
PR. Click on the Guidelines for Contributing link above for details.

Details

Vague documentation is useless. This makes this documentation useful.

Suggested Reviewers

If you know who should review this, use '@' to request a review.

@rpetrusha rpetrusha added the ✨ 1st-time docs contributor! Indicates PRs from new contributors to the docs repository label Mar 5, 2018
Copy link
Contributor

@rpetrusha rpetrusha left a comment

Choose a reason for hiding this comment

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

Thank you so much for contributing to the dotnet/docs repo and for adding additional detail about this type, @SimpleSamples. There were some issues with your proposed change, though, so I've proposed an alternative version.

@@ -26,6 +26,8 @@
<format type="text/markdown"><![CDATA[

## Remarks
This property is the object provided in the DoWorkEventArgs.Result member of the DoWork event.
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't accurate. RunWorkerCompletedEventArgs is a type rather than a property, and it's not provided in the DoWorkEventArgs.Result member. I'd propose instead the following: A RunWorkerCompletedEventArgs instance is provided to the <xref:System.ComponentModel.RunWorkerCompletedEventHandler> delegate when the <xref:System.ComponentModel.RunWorkerCompleted> event is fired.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I apologize for not thinking. I wish everyone writing documentation were as careful, there are some very serious problems with some VB documentation. Go ahead and make any improvents to what I submitted, I will try to figure out what I need to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I apologize if I misunderstand but if I understand then the suggested version is incomplete. Saying that an instance of RunWorkerCompletedEventArgs is provided does not specify where it is provided from. There should be something connecting the DoWork and the RunWorkerCompleted. I don't know if RunWorkerCompletedEventArgs is used anywhere else but if it is then that is the problem. It is difficult to say where the RunWorkerCompletedEventArgs comes from if it can come from multiple places.

In DoWork we have DoWorkEventArgs.Result that is then provided as RunWorkerCompletedEventArgs.Result. The two should be tied somehow. Even if it is part of the example then that would help. But the example is another frustrating example of being just a token example that does not help. It is my opinion however that the description of something should be complete enough that the example is not necessary, and that is rare in Microsoft documentation. Microsoft's typical solution is to make samples so complete that the relevant example is hidden among other stuff and that is where the name of my website SimpleSamoles.info comes from. The example should be relatively simple showing that the DoWork result is provided as the RunWorkerCompleted result.

@rpetrusha
Copy link
Contributor

I agree with you, @SimpleSamples, that this documentation is badly lacking in detail. In addition, although I'm not sure that the sample could can be simplified in a significant way, it certainly could be explained more thoroughly, and we could present a complete code example with relevant lines of code highlighted rather than a code snippet that is lacking in context. I'll open an issue to address these problems.

What I'd suggest doing is replacing the existing Remarks section with the following:

A Windows Forms form or control initiates an asynchronous operation by calling the <xref:System.ComponentModel.BackgroundWorker.RunWorkerAsyc%2A> method. This method in turn raises the <xref:System.ComponentModel.BackgroundWorker.DoWork%2A> event and passes it a <xref:System.ComponentModel.DoWorkEventArgs> instance. If the asynchronous operation returns a value, the <xref:System.ComponentModel.BackgroundWorker.DoWork%2A> event handler typically assigns it to the <xref:System.ComponentModel.DoWorkEventArgs.Result> property. When the asynchronous operation completes, the <xref:System.ComponentModel.BackgroundWorker.RunWorkerCompletedEvent> is fired and is passed a <xref:System.ComponentModel.RunWorkerCompletedEventArgs> instance that contains information about the status of the operation (whether it was cancelled, faulted, or completed successfully). If it completed successfully, its <xref:System.ComponentModel.RunWorkerCompletedEventArgs.Result> property contains the value returned by the asynchronous operation and previously assigned to the <xref:System.ComponentModel.DoWorkEventArgs.Result> property.

@Tanya-Solyanik, could you review this to ensure that it's accurate, please?

@SimpleSamples
Copy link
Contributor Author

I am still learning my way around this stuff.

Before proceeding I want to suggest that perhaps the thread Avoid program to be unresponsive is relevant. If it is relevant then it is because the Backgroundworker and related documentation is inadequate.

@SimpleSamples
Copy link
Contributor Author

SimpleSamples commented Mar 8, 2018

The latest version of the proposed Remarks section looks good to me. It is wonderfully inclusive compared to much of the Microsoft documentation.

Except I thought that System.ComponentModel.BackgroundWorker can be used in WPF too. The BackgroundWorker Class documentation certainly says Windows Forms so that excludes WPF.

I really like "Windows Forms form". I say that when relevant.

There is one more thing about BackgroundWorker that I consider inadequate about the documentation. For me when I first learned about BackgroundWorker it was not clear what executes where. The DoWork event executes as the other thread but everything else executes in the thread that created the BackgroundWorker and/or calls the DoWork, right? The proposed improvements do imply that DoWork is an "asynchronous operation" but I think that might not be clear enough for those not yet familiar with the details. These details are most appropriately covered in the BackgroundWorker class so that can be a separate issue. Perhaps the only modification that can be made here is, after saying "this method in turn raises the xref:System.ComponentModel.BackgroundWorker.DoWork%2A event" say "as the asynchronous operation" or something like that. I think that would link everything together and make it clear and explicit.

@rpetrusha
Copy link
Contributor

Thanks for responding, @SimpleSamples.

In terms of Avoid program to be unresponsive, it's important not to mix apples and oranges. The original question focuses on making a synchronous operation asynchronous. That can be done in a number of ways. The response to the question makes it asynchronous, but uses the newer task-based asynchronous programming rather than event-based asynchronous programming. The former doesn't make use of RunWorkerCompletedEventArgs. This does underscore, though, that the proposed replacement text needs to mention that this applies to the event-based asynchronous pattern.

So incorporating this along with your suggested change, the proposed text becomes:

When using the event-based asynchronous pattern for asynchronous operations, a Windows Forms form or control initiates an asynchronous operation by calling the xref:System.ComponentModel.BackgroundWorker.RunWorkerAsyc%2A method. This method in turn raises the xref:System.ComponentModel.BackgroundWorker.DoWork%2A event asynchronously and passes it a xref:System.ComponentModel.DoWorkEventArgs instance. If the asynchronous operation returns a value, the xref:System.ComponentModel.BackgroundWorker.DoWork%2A event handler typically assigns it to the xref:System.ComponentModel.DoWorkEventArgs.Result property. When the asynchronous operation completes, the xref:System.ComponentModel.BackgroundWorker.RunWorkerCompletedEvent is fired and is passed a xref:System.ComponentModel.RunWorkerCompletedEventArgs instance that contains information about the status of the operation (whether it was cancelled, faulted, or completed successfully). If it completed successfully, its xref:System.ComponentModel.RunWorkerCompletedEventArgs.Result property contains the value returned by the asynchronous operation and previously assigned to the xref:System.ComponentModel.DoWorkEventArgs.Result property.

@Tanya-Solyanik, does this look OK?

@Tanya-Solyanik
Copy link
Member

the xref:System.ComponentModel.BackgroundWorker.RunWorkerCompletedEvent is fired ---
Should not it say "raised"?

@Tanya-Solyanik
Copy link
Member

@rpetrusha - looks good to me. Definitely much more useful! It's up to you if you want to say "raised" or "fired", so I'm signing off on the latest iteration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ 1st-time docs contributor! Indicates PRs from new contributors to the docs repository
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants