Implement the Windows Task Dialog by kpreisser · Pull Request #1133 · dotnet/winforms

kpreisser · 2019-06-16T18:40:22Z

This PR implements the Windows Task Dialog as described in #146.

(Background info: This PR is based on my earlier implementation in the TaskDialog repo.)

Demo App

I have created a demo app (C# and VB) that consists of a Form with buttons, showing various Task Dialog scenarios. You can find it here: https://github.com/kpreisser/TaskDialogDemo

~~This app references System.Windows.Forms.dll and System.Windows.Forms.Primitives.dll built from this PR; you can simply place the app's repo folder near the winforms repo folder.~~

Known TODOs

Extract exception messages into resources
Have an accessibility assessment
Add tests - can be done separately

Implementation Notes

TaskDialog.ShowDialog() will only work if you have previously called Application.EnableVisualStyles() (or your app uses a manifest that enables common controls v6).
Otherwise, it will fail with an InvalidOperationException caused by an EntryPointNotFoundException.
When calling TaskDialog.ShowDialog() and specifying an owner window, other windows of the current UI thread will not be disabled.
This is different to the MessageBox.Show() behavior which also disables the other, non-owner windows (but they don't behave the same as the owner window, because when you click on them, the message box is not brought to the foreground; but clicking on the owner window brings the message box into foreground).
Because the handle of the task dialog is exposed by the TaskDialog.Handle property, in theory a user could send task dialog messages directly to the dialog and bypass the .NET implementation.
The implementation is not protected against that case, but this should be OK since the user cannot expect the implementation to handle that.
The native (OS) Task Dialog implementation seems to have some issues/bugs for which we need to implement work-arounds. These are described in markdown files in the Documentation/src/System/Windows/Forms/TaskDialog folder of this PR, including C++ samle code to reproduce them.

dnfclas · 2019-06-16T18:40:37Z

All CLA requirements met.

RussKie · 2019-06-16T22:59:02Z

Thank you, we'll start looking at it.

Just a heads up, before a merge the PR will need to be tidied up:

rebased on top of the master, and
not necessarily squashed into a single commit, but certainly collapsed into a smaller number of logical commits.

Fixesdotnet#146

kpreisser · 2019-06-17T06:27:32Z

Hi @RussKie,
thank you!

I rebased the PR with a single commit on top of master, as that was the easiest way.

RussKie · 2019-06-17T07:22:35Z

I rebased the PR with a single commit on top of master, as that was the easiest way.

Champion!

…etrieve the last error (and presumably the function also doesn't set it).

RussKie

The first sweep of reviews, I have probably missed things and may call them later (apologies in advance).

Overall it is quite impressive 👍

A number of comments of stylistic nature, I am not a big fan of line wrapping. I don't think we need to constrain ourselves to 80 char wide lines.
Here's how it looks on my screen:

Granted not everyone is blessed with a 4K screen, but ultra-wide monitors are far from being rare these days.
All exceptions must be moved to resources and all public API must have valid xml-docs. @merriemcgaw / @OliaG - I suppose we need to help here, please advise.
There are number of personal gists referenced in the code. We'll need to either remove them or migrate into docs (?). @merriemcgaw / @OliaG - please advise.

The big question, of course, is how do we test this. We'll need unit- and integration-level tests.
Have you given any thought to this?

Thank you

RussKie · 2019-06-17T07:52:37Z

+ /// Note: The dialog will not show until this handler returns (even if the
+ /// handler would run the message loop).
+ /// </remarks>
+ public event EventHandler Opened;


🤔 Would it be more appropriate to call this Opening since it is yet to actually open? ...Or Created?

/cc: @OliaG

Note that the current event model is illustrated in section "Event Cycle" in the initial post of #146.
(Also note that because this is part of the public API, it would also need to be changed in the API Proposal.)
Currently, on the TaskDialog there are events Opened+Shown (before and after show), and Closing+Closed (before and after close).
The Opened event could be renamed to Opening, but it is called at the same time with TaskDialogPage.Created, which means we might also need to rename that one.

For public surface area, @RussKie I am inclined to elevate any concerns to @OliaG and @terrajobst for API Review

I have had a chat with @OliaG about API, and we have a certain freedom and discretion to exercise.
A review has been scheduled but it is some time away.

…bclass of WindowSubclassHandler, use a separate virtual method for that. Otherwise, when another subclass overrides WndProc(), its exceptions would not be caught.

-Use longer lines -Use the default 4 instead of 8 spaces indentation after a line wrap -Add braces for single-statement blocks - Use "//" instead of "////" comments -Remove explicit base() calls

Co-Authored-By: Igor Velikorossov <RussKie@users.noreply.github.com>

kpreisser · 2019-06-19T20:46:06Z

Hi @RussKie,
thanks a lot for your feedback!

I have applied some of the changes and will do the remaining ones later.

Regarding testing, I must admit that I do not have much experience with unit/integration tests of GUI frameworks. I will probably need to look at how existing tests are done, and check what could be applied to the task dialog.

Thank you!

RussKie · 2019-06-20T03:18:38Z

Regarding testing, I must admit that I do not have much experience with unit/integration tests of GUI frameworks. I will probably need to look at how existing tests are done, and check what could be applied to the task dialog.

There are few things we'll need to have before we can merge it:

a demo app that will act as a demonstration of the functionality and a primer for other devs, and as a manual test for our QA team, and
a good unit test coverage - there is a lot of functionality we can test without needing a UI driver.

If we ask nicely @hughbe, he may be able to help with writing tests.

-Rename TaskDialog.DialogIsShown to .IsShown. -Rename TaskDialog.HandleAvailable to .IsHandleCreated.

…nsistency with Bind() and Unbind().

This ensures the task dialog con be used when you call Application.EnableVisualStyles() without having to add a manifest that enables common controls v6.

RussKie

This is one fine piece of software craftmanship! 🎉

Few nits. My eyes are aching after reviewing 10K lines of code...

kpreisser · 2020-04-17T08:21:29Z

What does "close while navigating" conceptually mean? Do we close the dialog instead of showing the next page? Or do we show the next page and then immediately close?

The dialog will close immediately before navigation is finished.
Technically, calling Navigate() means the dialog sets the caption of the new page as title, and prepares to create the controls of the new page and updates the dialog window height accordingly, and posts a message to the thread's message queue that, when processed, will finish navigation by actually displaying the new controls.
However, when calling Close() before this happens, the dialog window will close immediately and TaskDialog.ShowDialog() will return (the TaskDialogPage.Created event of the new page will not have been raised in that case).

Feels like a logic error on a dev's part...

Probably, but I don't see a reason to disallow closing the dialog in that case; it might complicate implementation of navigation for the user when they have to deal with the issue that they cannot call Close() before navigation is finished.

RussKie · 2020-04-17T09:00:48Z

Maybe worth add a remark to the Close docs stating this (if it's stating this).

… navigation.

…t that has been removed.

…ing method.

…ssHandler. This is to not rely on the implementation detail that Marshal.GetFunctionPointerForDelegate() returns the same pointer for the same delegate instance when called multiple times.

kpreisser · 2020-04-17T14:18:33Z

Maybe worth add a remark to the Close docs stating this (if it's stating this).

The docs of Close() already note that the method can be called while the dialog is waiting for navigation to finish. I also added docs to Navigate() to document that the page may not be updatable until the Created event occurs, but you can still call Close() during that time.

Thank you!

RussKie · 2020-04-18T11:50:56Z

I'm not @shanselman, and I'm not on a stage in front of hundreds of developers, but I'm merging this!
Thank you for your patience and commitment. We made the history my friend, the first major addition to Windows Forms in 15 years! Hope you have a champagne on ice, it is time to pop the cork.

kpreisser · 2020-04-18T13:22:41Z

Thanks a lot! 🎉

merriemcgaw · 2020-04-20T17:34:04Z

This is so exciting for WinForms developers around the world. @OliaG - time to brag about it for the next preview of .NET 5?

kpreisser · 2020-05-07T20:00:16Z

As an update, with VB support for WinForms announced for .NET 5, I have added a VB.NET project to the TaskDialogDemo repository which contains roughly the equivalent code of the C# project.

I had to make a some modifications to the VB code, because VB doesn't seem to support a number of recent C# language features (for example, pattern matching, local functions, async streams, collection initializer for existing collections within an object initializer), but it should have the same behavior.

paul1956 · 2020-05-07T23:40:25Z

@kpreisser thanks for the VB demo and doing this, besides cloning this Repos and running VS 2019 Version 16.6.0 Preview 6.0 what else do I need to do to run the demo?

RussKie · 2020-05-08T00:19:52Z

You need to have .NET 5.0 Preview 4 installed, that's where we merged it. Otherwise you need to build the source, and replace assemblies.

paul1956 · 2020-05-08T00:39:45Z

@RussKie I think I do

dotnet --list-sdks
1.1.13 [C:\Program Files\dotnet\sdk]
2.2.402 [C:\Program Files\dotnet\sdk]
3.1.100 [C:\Program Files\dotnet\sdk]
3.1.201 [C:\Program Files\dotnet\sdk]
3.1.300-preview-015135 [C:\Program Files\dotnet\sdk]
5.0.100-alpha1-015536 [C:\Program Files\dotnet\sdk]
5.0.100-preview.3.20216.6 [C:\Program Files\dotnet\sdk]
5.0.100-preview.4.20202.8 [C:\Program Files\dotnet\sdk]

If you mean the runtime, only 3 is available.

RussKie · 2020-05-08T01:15:36Z

Then you should all be set to run the demo

kpreisser · 2020-05-09T08:35:16Z

@paul1956 If the TaskDialogDemo isn't working for you, can you try to install the lastest Preview 4 build (5.0.100-preview.4.20258.7) from the daily builds page?

It could be that your version (5.0.100-preview.4.20202.8) is an older build that did not yet contain the changes of this PR.

paul1956 · 2020-05-09T22:03:28Z

@kpreisser thanks that fixed it.

kpreisser mentioned this pull request Jun 16, 2019
Windows Task Dialog #146
Closed

RussKie requested review from a team, OliaG and terrajobst June 16, 2019 22:55

kpreisser force-pushed the taskdialog branch from 90e5c51 to c53bcbcCompare June 17, 2019 06:24

Implement the Windows Task Dialog.
ce917c5
Fixesdotnet#146

kpreisser force-pushed the taskdialog branch from c53bcbc to ce917c5Compare June 17, 2019 06:25

kpreisser commented Jun 17, 2019
View reviewed changes

Comment threadsrc/Common/src/Interop/Interop.HRESULT.cs

kpreisser added 2 commits June 17, 2019 10:20

Don't specify SetLastError on TaskDialogIndirect() because we don't r…
08135a9
…etrieve the last error (and presumably the function also doesn't set it).

Follow-Up: Fix syntax error - sorry about that.
f2abf50

RussKie suggested changes Jun 19, 2019
View reviewed changes

kpreisserand others added 5 commits June 19, 2019 20:53

Instead of using a try-catch block in WndProc() in the task dialog su…
b850098
…bclass of WindowSubclassHandler, use a separate virtual method for that. Otherwise, when another subclass overrides WndProc(), its exceptions would not be caught.

PR feedback (code style)
36813a5
-Use longer lines -Use the default 4 instead of 8 spaces indentation after a line wrap -Add braces for single-statement blocks - Use "//" instead of "////" comments -Remove explicit base() calls

PR review: Fix typo.
95dba75

PR feedback: Invert ifs/reduce nesting.
606c421

PR review: Apply suggestions
e1e31c7
Co-Authored-By: Igor Velikorossov <RussKie@users.noreply.github.com>

RussKie assigned kpreisser Jun 20, 2019

kpreisser added 7 commits June 20, 2019 14:15

Use discards for unused out parameters.
591b6e8

PR feedback: Migrate gists to documentation.
b5779dc

PR feedback
f3f49bb
-Rename TaskDialog.DialogIsShown to .IsShown. -Rename TaskDialog.HandleAvailable to .IsHandleCreated.

PR feedback: Rename event "Help" to "HelpRequest".
0bd8057

PR feedback: Use Linq for checks (and remove outdated comment).
a92ae5a

PR feedback: Use a temporary variable in ApplyInitialization() for co…
e7eff58
…nsistency with Bind() and Unbind().

Activate a theming scope when showing the task dialog.
7dbc875
This ensures the task dialog con be used when you call Application.EnableVisualStyles() without having to add a manifest that enables common controls v6.

RussKie previously approved these changes Apr 17, 2020
View reviewed changes

kpreisser added 6 commits April 17, 2020 14:14

Add remarks about updateability of the dialog directly after starting…
f7063e3
… navigation.

Remove outdated comment which referred to the TaskDialog.Closing even…
ad3428e
…t that has been removed.

Remove unnecessary field.
859d0d9

PR feedback: Move static local functions to the bottom of the contain…
96f9149
…ing method.

Revert the change of not storing the function pointer in WindowSubcla…
6159bc4
…ssHandler. This is to not rely on the implementation detail that Marshal.GetFunctionPointerForDelegate() returns the same pointer for the same delegate instance when called multiple times.

PR feedback: Move exception messages to resources.
b6012e3

kpreisser dismissed RussKie’s stale review via b6012e3April 17, 2020 14:16

Remove unused parameter and improve code and comments.
f586a5f

RussKie approved these changes Apr 18, 2020
View reviewed changes

RussKie merged commit 5fbdbde into dotnet:masterApr 18, 2020

kpreisser deleted the taskdialog branch April 18, 2020 12:10

kpreisser mentioned this pull request Jun 16, 2020
Remove "#nullable enable" directives as NRT is now enabled in the project file. RussKie/winforms#1
Closed

RussKie mentioned this pull request Jul 6, 2020
Add Windows Forms TaskDialog API sample dotnet/samples#3702
Merged

kpreisser mentioned this pull request Jul 19, 2020
Remove unnecessary NRT directives from TaskDialog interop files #3602
Merged

RussKie mentioned this pull request Oct 28, 2020
API documentation debt - WinForms #4163
Closed

ghost locked as resolved and limited conversation to collaborators Feb 6, 2022

Uh oh!

Conversation

kpreisser commented Jun 16, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Demo App

Known TODOs

Implementation Notes

Uh oh!

dnfclas commented Jun 16, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RussKie commented Jun 16, 2019

Uh oh!

kpreisser commented Jun 17, 2019

Uh oh!

Uh oh!

RussKie commented Jun 17, 2019

Uh oh!

RussKie left a comment • edited by zsd4yr Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RussKieJun 17, 2019

Choose a reason for hiding this comment

Uh oh!

RussKieJun 20, 2019

Choose a reason for hiding this comment

Uh oh!

kpreisserJun 20, 2019

Choose a reason for hiding this comment

Uh oh!

zsd4yrJun 24, 2019

Choose a reason for hiding this comment

Uh oh!

RussKieJun 25, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kpreisser commented Jun 19, 2019

Uh oh!

RussKie commented Jun 20, 2019

Uh oh!

RussKie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kpreisser commented Apr 17, 2020

Uh oh!

RussKie commented Apr 17, 2020 via email • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kpreisser commented Apr 17, 2020

Uh oh!

RussKie commented Apr 18, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kpreisser commented Apr 18, 2020

Uh oh!

merriemcgaw commented Apr 20, 2020

Uh oh!

kpreisser commented May 7, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paul1956 commented May 7, 2020

Uh oh!

RussKie commented May 8, 2020

Uh oh!

paul1956 commented May 8, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

kpreisser commented Jun 16, 2019•
edited
Loading

dnfclas commented Jun 16, 2019•
edited
Loading

RussKie left a comment •
edited by zsd4yr
Loading

RussKie commented Apr 17, 2020 via email •
edited by ghost
Loading

RussKie commented Apr 18, 2020•
edited
Loading

kpreisser commented May 7, 2020•
edited
Loading

paul1956 commented May 8, 2020•
edited
Loading

kpreisser commented May 9, 2020•
edited
Loading