Closed RussKie closed 4 years ago
I propose we start with modelling API from user perspective, i.e. how a user could/would create TaskDialog
and set buttons.
Let's concentrate on the static
entrypoint for now as it will likely be used by the majority of our users; but it should equally apply to the instance-based version.
A user could write something like:
TaskDialog.ShowDialog(owner, text, mainInstruction, caption,
buttons: [] { TaskDialogButtons.Yes, TaskDialogButtons.No },
icon
);
TaskDialog.ShowDialog(owner, text, mainInstruction, caption,
buttons: [] { TaskDialogButtons.Yes, new TaskDialogButton("&No") },
icon
);
TaskDialog.ShowDialog(owner, text, mainInstruction, caption,
buttons: []
{
new TaskDialogButton("&Save"),
new TaskDialogButton("Do&n't save"),
TaskDialogButtons.Cancel
},
icon
);
~In event where a user may want to select a button other than the first, I think we should introduce an additional argument similar to MessageBox
's MessageBoxDefaultButton
. Although unlike MessageBox
, where the number of buttons is capped at 3, we can create a similar enum (refer to https://github.com/kpreisser/winforms/issues/7).~
~This will avoid situations where several buttons are marked as Default
, and simplify the validation. If a user specifies a non-existing button - ignore (like MessageBox.Show(this, "text", "text", MessageBoxButtons.OK, MessageBoxIcon.Information, MessageBoxDefaultButton.Button3);
)~
I realised that it becomes very difficult to express for a dialog that has both command links and buttons. Open to ideas 🤔
TaskDialog.ShowDialog(owner, text, mainInstruction, caption,
buttons: []
{
new TaskDialogButton("&Beginner", "10 mines, 9 x 9 title grid", TaskDialogButtonStyle.CommandLinks),
new TaskDialogButton("&Intermediate", "10 mines, 1 x 16 title grid", TaskDialogButtonStyle.CommandLinks)
new TaskDialogButton("I&nsane", "259 mines, 16 x 30 title grid", TaskDialogButtonStyle.CommandLinks)
},
icon
);
// we could have a custom type for command links buttons, to make the API slightly terser
// which is a basically a subclass of TaskDialogButton taking 2 parameters - text and description
// and various additional parameters
TaskDialog.ShowDialog(owner, text, mainInstruction, caption,
buttons: []
{
new TaskDialogCommandLinksButton("&Beginner", "10 mines, 9 x 9 title grid"),
new TaskDialogCommandLinksButton("&Intermediate", "10 mines, 1 x 16 title grid")
new TaskDialogCommandLinksButton("I&nsane", "259 mines, 16 x 30 title grid")
},
icon
);
A user should also be in a position to write the following:
TaskDialog.ShowDialog(owner, text, mainInstruction, caption,
buttons: []
{
new TaskDialogCommandLinksButton("&Beginner", "10 mines, 9 x 9 title grid"),
new TaskDialogCommandLinksButton("&Intermediate", "10 mines, 1 x 16 title grid")
new TaskDialogCommandLinksButton("I&nsane", "259 mines, 16 x 30 title grid"),
TaskDialogButtons.Yes,
new TaskDialogButton("&No", default: true)
},
icon,
);
I think the constructor for button should remain similar to what you currently have though without the defaultButton
, i.e.
public TaskDialogButton(string? text,
string? descriptionText = null,
bool enabled = true,
bool allowCloseDialog = true,
TaskDialogButtonStyle.CommandLinks = TaskDialogButtonStyle.Default)
If we choose to create a special type for commandlinks, e.g. TaskDialogCommandLinksButton
, then I propose we do the following:
public TaskDialogButton(string? text,
bool enabled = true,
bool allowCloseDialog = true)
public TaskDialogCommandLinksButton(string? text,
string? descriptionText = null,
bool enabled = true,
bool allowCloseDialog = true,
bool showArrowIcon = true)
: TaskDialogButton(text, enabled, allowCloseDialog)
As far as the return type, I think you already have it figured in the instance-based version - we return the button that was clicked, and let the user decide how to handle it.
However there was feedback, which, I think, is significant, especially in multi-page dialog scenarios:
How do I handle the button click? Do I have to assert the text? Would be great to assign some enum or numeric value to handle the clicks
In a simple scenario a user could write this:
TaskDialogButton buttonSave = new TaskDialogButton("&Save");
TaskDialogButton buttonDontSave = new TaskDialogButton("Do&n't save");
TaskDialogButton buttonCancel = TaskDialogButtons.Cancel;
var result = TaskDialog.ShowDialog(owner, text, mainInstruction, caption,
buttons: []
{
buttonSave,
buttonDontSave,
buttonCancel
},
icon
);
if (result == buttonSave)
{
...
}
else
...
...or this:
var result = TaskDialog.ShowDialog(owner, text, mainInstruction, caption,
buttons: []
{
new TaskDialogButton("&Save"),
new TaskDialogButton("Do&n't save"),
TaskDialogButtons.Cancel
},
icon
);
In the last example asserting isn't as straight forward. Users may not like the idea of asserting against literals.
Perhaps we could allow assigning each button a TaskDialogResult
(🤔 and may be extend the enum to have Custom1
, Custom2
etc up to the limit, see #7).
Again I am may be overthinking it right now, and we can come back to it later.
Few questions up for discussion:
Q: Is it possible to supply an invalid combination of buttons? If so, should we throw or ignore? A: TBD
Q: What if a user supplies button in a interleaved or mixed order? E.g.:
TaskDialog.ShowDialog(owner, text, mainInstruction, caption,
buttons: []
{
new TaskDialogCommandLinksButton("&Beginner", "10 mines, 9 x 9 title grid"),
TaskDialogButtons.Yes,
new TaskDialogCommandLinksButton("&Intermediate", "10 mines, 1 x 16 title grid")
new TaskDialogButton("&No", default: true)
new TaskDialogCommandLinksButton("I&nsane", "259 mines, 16 x 30 title grid"),
},
icon,
);
A: I'd think we assume the order within each group, so the above should be equivalent to:
TaskDialog.ShowDialog(owner, text, mainInstruction, caption,
buttons: []
{
new TaskDialogCommandLinksButton("&Beginner", "10 mines, 9 x 9 title grid"),
new TaskDialogCommandLinksButton("&Intermediate", "10 mines, 1 x 16 title grid")
new TaskDialogCommandLinksButton("I&nsane", "259 mines, 16 x 30 title grid"),
TaskDialogButtons.Yes,
new TaskDialogButton("&No", default: true)
},
icon,
);
I'd try to merge TaskDialogButtons
and TaskDialogButton
, so that TaskDialogButton
can be instantiated and has static helpers with instances of the standard buttons:
TaskDialog.ShowDialog(owner, text, mainInstruction, caption,
buttons: [] { TaskDialogButton.Yes, new TaskDialogButton("&No") },
icon
);
Giving it more thought, it might actually result in a nicer API if we had static factory methods on TaskDialogButton
because it provides in a one-stop-shop for all kinds of buttons:
public partial class TaskDialogButton
{
public static TaskDialogButton Yes { get; }
public static TaskDialogButton No { get; }
public static TaskDialogButton OK { get; }
public static TaskDialogButton Cancel { get; }
public static TaskDialogButton Create(string text);
public static TaskDialogCommandLinkButton CreateCommandLink(string text);
}
Hi @RussKie and @terrajobst, thanks a lot for your thoughts on this!
However there was feedback, which, I think, is significant, especially in multi-page dialog scenarios:
How do I handle the button click? Do I have to assert the text? Would be great to assign some enum or numeric value to handle the clicks
Note that there is also the TaskDialogButton.Tag
property (like on WinForms controls) that can be used to store arbitrary objects, so that you could later compare the Tag
value on the returned button. Would that be an option (rather than extending the TaskDialogResult
with Custom1
etc.)?
A user should also be in a position to write the following:
TaskDialog.ShowDialog(owner, text, mainInstruction, caption, buttons: [] { new TaskDialogCommandLinksButton("&Beginner", "10 mines, 9 x 9 title grid"), new TaskDialogCommandLinksButton("&Intermediate", "10 mines, 1 x 16 title grid") new TaskDialogCommandLinksButton("I&nsane", "259 mines, 16 x 30 title grid"), TaskDialogButtons.Yes, new TaskDialogButton("&No", default: true) }, icon, );
Be aware that it's not possible to show both custom (push) buttons and command links at the same time - it's only one or the other (but in either case they can be combined with standard buttons). This is because the display behavior (push buttons vs. command links) for custom buttons is controlled by the TDF_USE_COMMAND_LINKS
/TDF_USE_COMMAND_LINKS_NO_ICON
flags that applies to all custom buttons on the page. (And I don't think we should give special treatment to fixed strings like "&No"
and replace it with a standard button - that would look very wrong to me. E.g. the OS localizes those buttons, so a No
standard button will show "&Nein"
on a german OS.)
So, it's possible to differentiate between push buttons and command links by having a subclass TaskDialogCommandLinkButton
that inherits from TaskDialogButton
; but when showing the dialog (or navigating it), the method would have to throw an InvalidOperationException
if the user specifies both command links as well as custom push buttons. This was also the behavior in the Windows API Code Pack 1.1.
I'd try to merge
TaskDialogButtons
andTaskDialogButton
, so thatTaskDialogButton
can be instantiated and has static helpers with instances of the standard buttons:Giving it more thought, it might actually result in a nicer API if we had static factory methods on
TaskDialogButton
because it provides in a one-stop-shop for all kinds of buttons:
I agree it feels tempting to apply the same mechanism as already used in TaskDialogIcon
to TaskDialogButton
, so that it hides the internal TaskDialogResult
value and there is only one class for button.
However, in order to correctly handle e.g. events of a standard button (Click
), call methods (PerformClick
) and set properties (Enabled
etc.), I think it will not be possible to use singleton instances of the standard buttons returned by the static properties. Otherwise, every task dialog would share the same button instances, so it wouldn't be possible to show multiple dialogs at the same time.
It would certainly be possible to make the static properties (e.g. Yes
, Cancel
etc.) behave like factory methods and return a new instance of TaskDialogButton
every time they are called, which would avoid the problem. However, then the user wouldn't be able to do the following (because the getter would return a new instance):
var result = TaskDialog.Show(..., new[] { TaskDialogButton.Yes, TaskDialogButton.No });
if (result == TaskDialogButton.Yes) { ... }
We can make that scenario work by overriding the Equals
method (and overloading the ==
operator) to return true
if the buttons are both standard buttons and have the same internal TaskDialogResult
value.
(That would mean that Equals
would still return true even if the button's Enabled
or ElevationRequired
properties are different; however, I think it would match the behavior of the native API which would also consider such instances to be equal, so it might be OK to do this.)
I started to implement this in branch taskdialogRefactorButton
(commit a991e82c683c54a8dcae3a8926aea1167bcda6c8). Most importantly, the enums TaskDialogButtons
and TaskDialogResult
have been removed from the public API.
I changed class TaskDialogButton
to either represent a standard (common) button (such instances can be retrieved from static properties like TaskDialogButton.Yes
), or a custom button (by calling the constructor and supplying a text). Additionally, there is class TaskDialogCommandLinkButton
that inherits from TaskDialogButton
.
In TaskDialogButton
, I overrode Equals
to return true
if both buttons are standard buttons with the same internal button value. That way, the user can write code like this:
var result = TaskDialog.ShowDialog("Hello World",
buttons: new[]
{
TaskDialogButton.Yes,
TaskDialogButton.No,
});
if (result == TaskDialogButton.Yes)
{
// ...
}
Or, to handle events or change properties (like Default
, Enabled
etc.), the button can be stored in a variable:
var buttonYes = TaskDialogButton.Yes;
buttonYes.Click += (s, e) =>
{
Console.WriteLine("'Yes' clicked");
};
var result = TaskDialog.ShowDialog("Hello World",
buttons: new[]
{
buttonYes,
TaskDialogButton.No,
});
if (result == buttonYes)
{
// ...
}
Also, you can now specify custom buttons or command links in the static ShowDialog
method:
var result = TaskDialog.ShowDialog("Hello World",
buttons: new[]
{
new TaskDialogCommandLinkButton("My Command Link")
{
Tag = 1
},
TaskDialogButton.Close
});
if (Equals(result.Tag, 1))
{
// ...
}
Note: The previous collections CustomButtons
and StandardButtons
in TaskDialogPage
have been replaced with a single Buttons
collection, but one thing to have in mind is that the button order in that collection is not necessarily the same as the order in which the native dialog actually displays them.
The native task dialog displays buttons from the collection in the following order:
OK
Yes
No
Abort
Retry
Cancel
Ignore
TryAgain
Continue
Close
Help
When specifying both custom buttons and command link buttons in the same collection, TaskDialog.Show()
(or the Page
setter for navigation) throws an InvalidOperationException
.
I did not yet implement factory methods like TaskDialogButton.Create
for creating custom buttons or command links.
Edit: The public API of the button classes now looks like this:
public class TaskDialogButton : TaskDialogControl
{
// "factory" properties for standard buttons
// (return a new instance on every call)
public static TaskDialogButton OK { get; }
public static TaskDialogButton Yes { get; }
public static TaskDialogButton No { get; }
public static TaskDialogButton Abort { get; }
public static TaskDialogButton Retry { get; }
public static TaskDialogButton Cancel { get; }
public static TaskDialogButton Ignore { get; }
public static TaskDialogButton TryAgain { get; }
public static TaskDialogButton Continue { get; }
public static TaskDialogButton Close { get; }
public static TaskDialogButton Help { get; }
public TaskDialogButton();
public TaskDialogButton(string? text, bool enabled = true, bool defaultButton = false, bool allowCloseDialog = true);
public bool AllowCloseDialog { get; set; }
public bool DefaultButton { get; set; }
public bool ElevationRequired { get; set; }
public bool Enabled { get; set; }
public bool Visible { get; set; }
public string? Text { get; set; } // setter will throw for standard buttons
public event EventHandler? Click;
public void PerformClick();
public override bool Equals(object? obj);
public override int GetHashCode();
public override string ToString();
public static bool operator ==(TaskDialogButton? b1, TaskDialogButton? b2);
public static bool operator !=(TaskDialogButton? b1, TaskDialogButton? b2);
}
public sealed class TaskDialogCommandLinkButton : TaskDialogButton
{
public TaskDialogCommandLinkButton();
public TaskDialogCommandLinkButton(string? text, string? descriptionText = null, bool enabled = true, bool defaultButton = false, bool allowCloseDialog = true);
public string? DescriptionText { get; set; }
}
Regarding the default button, alternatively to having the property TaskDialogButton.Default
, I think it might be possible to instead add an parameter TaskDialogButton? defaultButton
to the ShowDialog
method, which would be similar to the MessageBox.Show()
API, e.g.:
var result = TaskDialog.ShowDialog("Hello World",
buttons: new[]
{
TaskDialogButton.Yes,
TaskDialogButton.No,
},
defaultButton: TaskDialogButton.Yes);
What do you think?
Thanks!
Replying to https://github.com/kpreisser/winforms/issues/5#issuecomment-583828618, though some items have been revised in the follow up comment.
Note that there is also the
TaskDialogButton.Tag
property (like on WinForms controls) that can be used to store arbitrary objects, so that you could later compare theTag
value on the returned button. Would that be an option (rather than extending theTaskDialogResult
withCustom1
etc.)?
I'm not sure it is a good idea to force users to use Tag
property. It may cause unnecessary allocations.
Besides how does a user set Tag
s for standard buttons?
So, it's possible to differentiate between push buttons and command links by having a subclass
TaskDialogCommandLinkButton
that inherits fromTaskDialogButton
; but when showing the dialog (or navigating it), the method would have to throw anInvalidOperationException
if the user specifies both command links as well as custom push buttons. This was also the behavior in the Windows API Code Pack 1.1 .
I think it is totally fine.
I agree it feels tempting to apply the same mechanism as already used in
TaskDialogIcon
toTaskDialogButton
, so that it hides the internalTaskDialogResult
value and there is only one class for button. However, in order to correctly handle e.g. events of a standard button (Click
), call methods (PerformClick
) and set properties (Enabled
etc.), I think it will not be possible to use singleton instances of the standard buttons returned by the static properties. Otherwise, every task dialog would share the same button instances, so it wouldn't be possible to show multiple dialogs at the same time.
Theoretically we could create these buttons per dialog instance. Not sure how much benefit vs complexity in this scenario. But maybe worth considering.
It would certainly be possible to make the static properties (e.g.
Yes
,Cancel
etc.) behave like factory methods and return a new instance ofTaskDialogButton
every time they are called, which would avoid the problem. However, then the user wouldn't be able to do the following (because the getter would return a new instance):var result = TaskDialog.Show(..., new[] { TaskDialogButton.Yes, TaskDialogButton.No }); if (result == TaskDialogButton.Yes) { ... }
We can make that scenario work by overriding the
Equals
method (and overloading the==
operator) to returntrue
if the buttons are both standard buttons and have the same internalTaskDialogResult
value. (That would mean thatEquals
would still return true even if the button'sEnabled
orElevationRequired
properties are different; however, I think it would match the behavior of the native API which would also consider such instances to be equal, so it might be OK to do this.)
Great assessment 👍
Throwing a spanner into works here 😈, maybe we shouldn't abandon the idea of TaskDialogResult
afterall?
Each button (standard or custom) would have its own TaskDialogResult
, and TaskDialog.Show(...) : TaskDialogResult
. For standard buttons it's straight forward, for custom buttons we could have Custom1...CustomN
(see https://github.com/kpreisser/winforms/issues/7).
Right now users will be quite familiar with the following implementations:
var result = MessageBox.Show(.....);
switch (result)
{
case DialogResult.Yes: // handle...
case DialogResult.No: // handle...
case DialogResult.Cancel: // handle...
...
}
It would be great if we could offer a similar experience our users would be familiar with:
var result = TaskDialog.Show(.....);
switch (result)
{
case TaskDialogResult.Yes: // handle...
case TaskDialogResult.No: // handle...
case TaskDialogResult.Cancel: // handle...
case TaskDialogResult.Custom1: // handle...
case TaskDialogResult.Custom2: // handle...
case TaskDialogResult.Custom3: // handle...
...
}
It doesn't look perfect, but I think it looks better than:
TaskDialogButton buttonSave = new TaskDialogButton("&Save");
buttonSave.Tag = 1;
TaskDialogButton buttonDontSave = new TaskDialogButton("D&on't save");
buttonDontSave.Tag = 2;
var result = TaskDialog.Show(..., new[] { buttonSave, buttonDontSave });
if (result.Tag == 1) { ... }
else if (result.Tag == "foo") { ... }
Another options comes in mind having a TaskDialogResult.Custom
and make a user set another property to contain custom result (like Tag
). But this feels complex and brittle.
I'm not entirely happy with either option at this stage. Open to discussing further options.
When specifying both custom buttons and command link buttons in the same collection,
TaskDialog.Show()
(or thePage
setter for navigation) throws anInvalidOperationException
.
👍
The public API of the button classes now looks like this:
👍
Regarding the default button, alternatively to having the property
TaskDialogButton.Default
, I think it might be possible to instead add an parameterTaskDialogButton? defaultButton
to theShowDialog
method, which would be similar to theMessageBox.Show()
API, e.g.:var result = TaskDialog.ShowDialog("Hello World", buttons: new[] { TaskDialogButton.Yes, TaskDialogButton.No, }, defaultButton: TaskDialogButton.Yes);
I don't quite like this. How do we default to a custom button without creating the custom button outside TaskDialog.ShowDialog(...)
first?
What if the dialog is populated dynamically?
Below are issues I come across applying the new branch to https://github.com/RussKie/TaskDialogStudy-exercises codebase (branch: taskdialogRefactorButton).
The native task dialog displays buttons from the collection in the following order:
- Custom Buttons/Command Links in their relative order from the collection
Standard Buttons in an OS-defined order:
OK
Yes
No
Abort
Retry
Cancel
Ignore
TryAgain
Continue
Close
Help
From the user perspective the following looks unexpected. The expected result - [Yes] [No]
static
buttonsAs you have already raised making standard buttons static
brings a whole lot of issues. Whilst it may be relatively edge-casey to have multiple active dialogs concurrently, it is still a possibility. Perhaps we need to reconsider this approach. 🤔
For example, it is impossible to make a standard button default without affecting all running instances of TaskDialog
across a process.
var button = TaskDialogButton.No;
button.DefaultButton = true; // [!] <-- modified globally shared instance
button.Tag = ...; // [!] <--- OH NO!
// we don't have an API, but even if we did, it would have to do something like: button.DefaultButton = true, which is wrong
taskDialog.Page.Buttons.Add(TaskDialogButton.No, defaultButton: true);
A "hacky" workaround is to write something like the following, but it brings us to the issue described in #1. And it will probably fail identity checks when attempting to evaluate the results to
taskDialog.Page.Buttons.Add(TaskDialogButton.No.Text, defaultButton: true);
This may be unrelated, so happy to move it to a separate issue
var result = TaskDialog.ShowDialog(//this,
text: "What level of difficulty do you want to play?",
caption: "Minesweeper",
buttons: new TaskDialogButton[]
{
new TaskDialogCommandLinkButton("&Beginner", "10 mines, 9 x 9 title grid"),
new TaskDialogCommandLinkButton("&Intermediate", "10 mines, 1 x 16 title grid", enabled: true),
new TaskDialogCommandLinkButton("I&nsane", "259 mines, 16 x 30 title grid", defaultButton: true),
TaskDialogButton.Cancel,
},
icon: TaskDialogIcon.ShieldBlueBar
);
I'm not sure it is a good idea to force users to use
Tag
property. It may cause unnecessary allocations. Besides how does a user setTag
s for standard buttons?
For standard buttons it would not be needed, since you could just write (if button == TaskDialogButton.Yes)
:wink: But in order to set the tag, you would need to store the button in a variable first, e.g.
var btnYes = TaskDialogButton.Yes;
btnYes.Tag = 123;
//...
Each button (standard or custom) would have its own
TaskDialogResult
, andTaskDialog.Show(...) : TaskDialogResult
. For standard buttons it's straight forward, for custom buttons we could haveCustom1...CustomN
(see #7).
I can see the benefit of keeping the TaskDialogResult
. However, I'm not sure if it's a good idea to change the return type of TaskDialog.ShowDialog
from TaskDialogButton
to TaskDialogResult
(enum value), as it makes it harder to actually identify the button instance if you only care about the actual TaskDialogButton
instance.
Maybe we can do it similar to System.Windows.Forms.Button
that has a DialogResult
property, and add TaskDialogButton.DialogResult
(getter) that returns the dialog result, which will be OK
, Yes
, No
, ... for standard buttons, and Custom1
, Custom2
etc. for custom buttons. Then, you can do
var button = TaskDialog.Show(...);
switch (button.DialogResult) {
case TaskDialogResult.Yes: // handle...
case TaskDialogResult.No: // handle...
case TaskDialogResult.Cancel: // handle...
case TaskDialogResult.Custom1: // handle...
case TaskDialogResult.Custom2: // handle...
}
Edit: Done - see commit https://github.com/kpreisser/winforms/commit/6a73a754f1321141a1f57e28af6d016e840f8918.
How do we default to a custom button without creating the custom button outside
TaskDialog.ShowDialog(...)
first?
You would need to store the button in a variable, in order to specify it as default button.
From the user perspective the following looks unexpected. The expected result - [Yes] [No]
Right, but I don't think there's anything we can do about that. The standard ("common") button order is defined by the OS.
As you have already raised making standard buttons
static
brings a whole lot of issues. Whilst it may be relatively edge-casey to have multiple active dialogs concurrently, it is still a possibility. Perhaps we need to reconsider this approach. 🤔For example, it is impossible to make a standard button default without affecting all running instances of
TaskDialog
across a process.
In the current implementation of branch taskdialogRefactorButton
this isn't a problem any more, because I implemented the static properties to return a new button instance on each call (as shown in the code sample I posted earlier).
Do you think this isn't a good option?
3. Visual glitch calculating the shield bar (minor)
This may be unrelated, so happy to move it to a separate issue
The native task dialog probably expects a instruction text to be present when using one of the colored bar icons, which is why command-link buttons are positioned a bit too hight. Maybe this can be fixed in the OS.
Thanks!
I can see the benefit of keeping the
TaskDialogResult
. However, I'm not sure if it's a good idea to change the return type ofTaskDialog.ShowDialog
fromTaskDialogButton
toTaskDialogResult
(enum value), as it makes it harder to actually identify the button instance if you only care about the actualTaskDialogButton
instance.Maybe we can do it similar to
System.Windows.Forms.Button
that has aDialogResult
property, and addTaskDialogButton.DialogResult
(getter) that returns the dialog result, which will beOK
,Yes
,No
, ... for standard buttons, andCustom1
,Custom2
etc. for custom buttons. Then, you can dovar button = TaskDialog.Show(...); switch (button.DialogResult) { case TaskDialogResult.Yes: // handle... case TaskDialogResult.No: // handle... case TaskDialogResult.Cancel: // handle... case TaskDialogResult.Custom1: // handle... case TaskDialogResult.Custom2: // handle... }
Edit: Done - see commit 6a73a75.
I like that 👍 The only issue is to decide how many "CustomX" is appropriate. Especially if we don't impose any limits on number of buttons and commandlinks shown at the same time. @terrajobst any thoughts on this?
From the user perspective the following looks unexpected. The expected result - [Yes] [No]
Right, but I don't think there's anything we can do about that. The standard ("common") button order is defined by the OS.
We just have to make sure to have it documented.
The native task dialog probably expects a instruction text to be present when using one of the colored bar icons, which is why command-link buttons are positioned a bit too hight. Maybe this can be fixed in the OS.
Haha! I doubt the Windows team will consider this. But I'm happy to label this as "won't fix, by design".
What do you think about making users to specify the result? E.g.:
- public TaskDialogButton(string? text, bool enabled = true, bool defaultButton = false, bool allowCloseDialog = true)
+ public TaskDialogButton(string? text, TaskDialogResult buttonResult, bool enabled = true, bool defaultButton = false, bool allowCloseDialog = true)
You could see the whole change in https://github.com/kpreisser/winforms/pull/9 And changes a user will see can be seen here: https://github.com/RussKie/TaskDialogStudy-exercises/commit/1c58d5064029d75104ca95d122f17ceb0a1ee275
As a side-effect of this design, the user could specify a custom value, e.g.:
new TaskDialogButton("&Save", (TaskDialogResult)500)
Though it does feel a little hacky.
I had a chat with @terrajobst discussing pros and cons of both approaches.
The main reason I pushed for a variant of DialogResult
is because it is idiomatic (cool word, thanks Immo) for Windows Forms, it is used by both a Form
and a MessageBox
.
However it also opens a pandora box of issues like how to set a result for custom buttons, and whether we need to limit a number of buttons a user can have.
An alternative is what has been suggested earlier and what you have already implemented in a991e82c683c54a8dcae3a8926aea1167bcda6c8 - it isn't as idiomatic but avoids the above problems, and perhaps making a learning curve slightly less steep. The downside of this approach - we can't use switch
statements (C# 8.0).
It is quite likely users may start with basic use-cases like this:
var result = TaskDialog.ShowDialog(this, text: ..., mainInstruction: ..., caption: ...,
buttons: new TaskDialogButton[] { TaskDialogButton.Yes, TaskDialogButton.No, },
icon: TaskDialogIcon....
);
if (result == TaskDialogButton.Yes)
{
...
}
From this the following code isn't as big of a jump, and avoids the API bloat:
var btnSave = new TaskDialogButton("&Save");
var result = TaskDialog.ShowDialog(this, text: ..., mainInstruction: ..., caption: ...,
buttons: new TaskDialogButton[] { btnSave, TaskDialogButton.Yes, TaskDialogButton.No, },
icon: TaskDialogIcon....
);
if (result == btnSave)
{
...
}
else if (result == TaskDialogButton.Yes)
{
...
}
We also touched on static
standard buttons and concluded that they have to be made immutable singletons.
That is, any writable property of TaskDialogButton
must check whether it is a standard button, and throw if a write operation is requested.
I think with this we are getting on the final stretch.
Please let me know if you have any thoughts or doubts. Thank you
We also touched on
static
standard buttons and concluded that they have to be made immutable singletons. That is, any writable property ofTaskDialogButton
must check whether it is a standard button, and throw if a write operation is requested.
OK, but then how would a user modify the button (e.g. enable/disable the button) or add event handlers for the Click
event to standard buttons?
For example, the Multi Page Dialog demo in TaskDialogDemo
needs to be able to enable and disable the Yes
button, and needs to be able to handle the Click
event to navigate to the next page, without closing the dialog.
(And it needs to add an invisible Cancel
button where AllowCloseDialog = false
, so that in the second page, the dialog cannot be closed by clicking on the red [X] button.)
If we would make the standard button instances that are available thorugh the static
getters immutable, that would no longer be possible, and I think that would be a pretty huge loss of functionality.
OK, but then how would a user modify the button (e.g. enable/disable the button) or add event handlers for the
Click
event to standard buttons?
Opps, I was concentrating on the single page scenarios, and forgot about this.
I have spent more time reflecting over the @terrajobst's suggestions, and then had few internal discussions with the team about the situation. I have also spent some time tweaking the API. Below are my thoughts.
There was a suggestion to disable/remove the multi-page capability, and turning the TaskDialog
effectively into a very powerful MessageBox
. By removing multi-page we can forgo button's Click
events, since in a single page dialog clicking a button means closing the dialog.
Doing so we could deliver an MVP quicker, and resolve any design issues pertaining to the multi-page dialog later.
I have tried just that by removing/hiding the public constructor and Page
property (https://github.com/dotnet/winforms/commit/a317bb44f7e6186d9871bd326247fc43d744661c). The concept of the page was found confusing, and it can't be accessed via ShowDialog
method anyway.
This removal prompted to add many arguments to ShowDialog
methods to expose all of TaskDialogPage
's functionality, e.g.:
TaskDialogFooter
,TaskDialogCheckBox
,TaskDialogExpander
,TaskDialogRadioButtonCollection
,TaskDialogProgressBar
,RightToLeftLayout
, and more...There's just too many of them to pass into the method (https://github.com/dotnet/winforms/commit/fc88d7d60be72517e91dd338fa060571dfa995a4):
var result = TaskDialog.ShowDialog(this,
text: ...,
mainInstruction: ...,
caption: ...,
buttons: new TaskDialogButton[] { button1, button2, button3 },
icon: TaskDialogIcon.ShieldWarningYellowBar,
footer: footer,
checkBox: verificationCheckBox,
expander: expander,
....
);
One way out of this situation is to consider a creation of an TaskDialogOptions
class that would allow to configure a page without exposing an instance of a TaskDialogPage
to the user. It is more idiomatic to ASP.NET Core than Windows Forms though, but didn't look too bad API-wise:
public static TaskDialogButton ShowDialog(
string? text,
string? mainInstruction = null,
string? caption = null,
IEnumerable<TaskDialogButton>? buttons = null,
TaskDialogIcon? icon = null,
TaskDialogOptions? footer = null)
@kpreisser @terrajobst @OliaG @merriemcgaw @KlausLoeffelmann what do you think? I know it is pretty radical, but I'm trying to find way for us to merge the bulk of it. We are fully committed to offering full capabilities of TaskDialog
controls to our users.
By removing multi-page we can forgo button's
Click
events, since in a single page dialog clicking a button means closing the dialog.
I don't think the ability to handle a button's Click
event is only needed to handle multi-page dialogs. Even in single page dialogs, you might want to handle the button click event, e.g. for doing an action while keeping the dialog open. For example, the Elevation Required demo of TaskDialogDemo
handles the TaskDialogButton.Click
event in order to start an elevated process while keeping the dialog open, and closing the dialog once the process is successfully started.
(While it handles the click event from a custom button, it could as well handle the click event of a standard button.)
Or, imagine a progress bar dialog in which you want to display the current progress and don't want the user to close the dialog (as you will programmatically close it afterwards). To prevent the user from closing the dialog, you will have do add at least one button where you set Enabled = false
(as otherwise the dialog would show an OK
button which will close the dialog on click).
While it would be possible to use a custom button for this, it is probably better to use a standard button (like the Close
button), as the text for it is provided by the OS.
Also, as a user, I would be asking "why are there instances of TaskDialogButton
where I can handle events and change properties like Enabled
(=custom buttons), and other instances where this doesn't work (=standard buttons)? That doesn't make sense, they are all just TaskDialogButton
s so they should work the same way."
For the buttons API, I personally would still prefer the approach to have the static TaskDialogButton
getters return new instances on each call (as implemented with https://github.com/kpreisser/winforms/commit/a991e82c683c54a8dcae3a8926aea1167bcda6c8), so while that may not exactly be the convention, there will be no problem to handle events or modify the buttons.
Another suggestion from a colleague of mine was to have instance properties (OK
, Cancel
, ...) on TaskDialogButtonCollection
, which are pre-polulated with corresponding TaskDialogButton
instances, so e.g. you could add them with collection.Add(collection.Yes);
or collection.Add(collection.YesButton);
or similar code; or, they are automatically added to the collection by passing an TaskDialogStandardButtons
flags enum to the TaskDialogButtonCollection
constructor.
Regarding hiding the Page
functionality for now, note that this would also remove the ability to update the dialog's text that is (currently) exposed in TaskDialogPage
properties (Instruction
, Text
, Icon
).
Honestly, while removing the page functionality for now might allow to delay some design decisions to a later time, it would mean a core functionality would be missing from the task dialog (similar to not allowing to modify/handle events of standard buttons), and in that case I'm not sure if it's even worth to implement that in winforms, as it would just look like a "more powerful message box" (as you already said), and there are a number of existing libraries that users could just use for that.
(E.g. in applications that we (the company I work for) develop at work, a task dialog without navigation would be pretty useless for us.)
For example, as I mentioned in the initial post of dotnet#146, the Windows API Code Pack 1.1 provided an implementation of Task Dialogs, but apart from a number of bugs/issues, it was also missing a number of features that are actually possible with the native API (and are implemented in dotnet#1133):
TaskDialog.Closing
event)TaskDialog.Close()
)IntPtr
)ProgressBar
between marquee and non-marquee state while the dialog is shownRadioButton
selection programmatically while the dialog is shown, or retrieve their current value (only indirectly possible by tracking the last instance where the TaskDialogRadioButton.Click
event was raised)When implementing the task dialog, it was my goal to include all of these features (most importantly, page navigation, and modifying/handling clicks of standard buttons). I also think that the current API has a good shape for implementing these features, including controls like Button
, CheckBox
etc. (whose properties and events are similar to WinForms controls) and navigation to a different page that is implemented with TaskDialogPage
and the TaskDialog.Page
property.
I think it is not avoidable that users may not immediately know how these scenarios work, but when you want to use more complex features (like navigation) you might have to read the documentation first. For simple (messagebox-like) scenarios, there are the static ShowDialog
methods which don't support navigation or updating text, but allow the user to get started quickly.
For comparison, here are examples of how other .NET Task Dialog libraries implement navigation:
TaskDialog
instance where you can populate the properties and controls that should be shown for the new page, and then call dialog1.NavigateTo(dialog2)
which will start navigation.
However, it is actually dialog1
that will now show the contents of dialog2
, so if you added any event handlers to dialog2
, they will not work as the events are still raised on dialog1
. Also, this means that the current UI state (native dialog shows contents of dialog2
) doesn't match the code model (dialog1
is the instance that shows the current dialog).Thank you!
I don't think the ability to handle a button's
Click
event is only needed to handle multi-page dialogs. Even in single page dialogs, you might want to handle the button click event, e.g. for doing an action while keeping the dialog open. For example, the Elevation Required demo ofTaskDialogDemo
handles theTaskDialogButton.Click
event in order to start an elevated process while keeping the dialog open, and closing the dialog once the process is successfully started. (While it handles the click event from a custom button, it could as well handle the click event of a standard button.)Or, imagine a progress bar dialog in which you want to display the current progress and don't want the user to close the dialog (as you will programmatically close it afterwards). To prevent the user from closing the dialog, you will have do add at least one button where you set
Enabled = false
(as otherwise the dialog would show anOK
button which will close the dialog on click). While it would be possible to use a custom button for this, it is probably better to use a standard button (like theClose
button), as the text for it is provided by the OS.Also, as a user, I would be asking "why are there instances of
TaskDialogButton
where I can handle events and change properties likeEnabled
(=custom buttons), and other instances where this doesn't work (=standard buttons)? That doesn't make sense, they are all justTaskDialogButton
s so they should work the same way."For the buttons API, I personally would still prefer the approach to have the static
TaskDialogButton
getters return new instances on each call (as implemented with , so while that may not exactly be the convention, there will be no problem to handle events or modify the buttons.
I've been mulling over this for the past few days.
This looks like the best option right now, so let's keep "the static TaskDialogButton
getters return new instances on each call" implementation.
We just need to ensure that the following is true
:
var button = TaskDialogButton.Yes;
button.<change state>
Assert.True(button == TaskDialogButton.Yes);
Which I believe a991e82 facilitates.
Hi @RussKie,
I've been mulling over this for the past few days. This looks like the best option right now, so let's keep "the static
TaskDialogButton
getters return new instances on each call" implementation. We just need to ensure that the following istrue
:var button = TaskDialogButton.Yes; button.<change state> Assert.True(button == TaskDialogButton.Yes);
Which I believe a991e82 facilitates.
Thank you!
Yes, with a991e82, the comparison button == TaskDialogButton.Yes
will work, as TaskDialogButton
overrides Equals
and GetHashCode
and overloads ==
and !=
operators.
Closing as the core issue of improving the "buttons" API is resolved.
Let's explore how we can streamline the API and make it easier to grasp.
During an internal usability study for the TaskDialog API, all participant were confused by various “buttons” exposed in the API.
Few sentiments:
Some users expressed ideas along the lines of:
There were few other suggestions, e.g.
TaskDialogResult
toTaskDialogButtons
“For customer facing APIs the naming should be dictated by the users scenarios rather than internal architecture of the implementation.”CustomButtons
andStandardButtons
haveButtons
which will accept different types. This will also remove the confusion related to the display order.