kpreisser / winforms

Windows Forms is a .NET Core UI framework for building Windows desktop applications.
MIT License
0 stars 0 forks source link

Streamline single/multipage API #13

Closed RussKie closed 4 years ago

RussKie commented 4 years ago

Let's explore how we can streamline the API and make it easier to grasp.

Right now we have the following API:

        public TaskDialogButton ShowDialog();
        public TaskDialogButton ShowDialog(IntPtr hwndOwner);
        public TaskDialogButton ShowDialog(IWin32Window owner);

        public static TaskDialogButton ShowDialog(string text, string mainInstruction = null, string caption = null, 
                                                  IEnumerable<TaskDialogButton> buttons = null, TaskDialogIcon icon = null, 
                                                  TaskDialogButton defaultButton = null);
        public static TaskDialogButton ShowDialog(IntPtr hwndOwner, string text, string mainInstruction = null, string caption = null, 
                                                  IEnumerable<TaskDialogButton> buttons = null, TaskDialogIcon icon = null, 
                                                  TaskDialogButton defaultButton = null);
        public static TaskDialogButton ShowDialog(IWin32Window owner, string text, string mainInstruction = null, string caption = null,
                                                  IEnumerable<TaskDialogButton> buttons = null, TaskDialogIcon icon = null, 
                                                  TaskDialogButton defaultButton = null);

During an internal usability study for the TaskDialog API, all participant were confused by different ways of setting up the dialog. Few issues have been identified:

RussKie commented 4 years ago

Dealing with the API discoverability issue: the static methods do not expose the full set of available APIs

There are a number of different ways to mitigate the API discoverability issue.

One can be exposing all remaining properties via the static API, these properties include (but not limited to): This removal prompted to add many arguments to ShowDialog methods to expose all of TaskDialogPage's functionality, e.g.:

However if we pass all these properties into the method, we will make the API unusable:

    var result = TaskDialog.ShowDialog(this,
        text: ...,
        mainInstruction: ...,
        caption: ...,
        buttons: new TaskDialogButton[] { button1, button2, button3 },
        icon: TaskDialogIcon.ShieldWarningYellowBar,
        footer: footer,
        checkBox: verificationCheckBox,
        expander: expander,
        progressBar: progressBar,
        rightToLeftLayout: rightToLeftLayout,
        ....
        );

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 class TaskDialogOptions
    {
        public TaskDialogFooter Footer { get; set; }
        public TaskDialogCheckBox Verification { get; set; }
        public TaskDialogExpander Expander { get; set; }
        // etc...
    }

    public static TaskDialogButton ShowDialog(
        string? text,
        string? mainInstruction = null,
        string? caption = null,
        IEnumerable<TaskDialogButton>? buttons = null,
        TaskDialogIcon? icon = null,
        TaskDialogOptions? options = null)

Internally, properties set in the options object would be assigned to the corresponding properties on a Page object.

This design addresses the first discoverability issue - the user will be able to see and explore additional properties available. However the issue of multipage API is still unaccounted for.

Dealing with the cognitive load issue

It would not totally unreasonable to expect the majority of users to use the TaskDialog as a simple replacement for MessageBox, and therefore it would be used as a single page dialog. We have initially based the public API surface to match the API of MessageBox. However, as we later discovered, following the same pattern we seriously constrain our abilities to provide a useable and discoverable API.

To reduce the cognitive load, and to make users more aware of a Page concept (and thus possible make aware of multipage functionality), perhaps we should consider simplifying the API as follows:

    public static TaskDialogButton ShowDialog(TaskDialogPage page);
    public static TaskDialogButton ShowDialog(IntPtr hwndOwner, TaskDialogPage page);
    public static TaskDialogButton ShowDialog(IWin32Window owner, TaskDialogPage page);

A user could write the following code that it is slightly more verbose than the original but marginally so:

    TaskDialogButton clickedButton = TaskDialog.ShowDialog(this,
                                       new TaskDialogPage
                                       {
                                           Text = "Are you sure you want to do this?",
                                           MainInstruction = "Stopping the operation might break things...",
                                           Caption = "Confirmation",
                                           Buttons = new TaskDialogButtonCollection { button1, button2, button3 },
                                           Icon = TaskDialogIcon.ShieldWarningYellowBar,
                                           DefaultButton = button2
                                       });

This way the user does not need to think about what method to chose, and has the full access to the properties of the dialog. We also address the first discoverability issue, and reduce the gap with the multipage API.

The added bonus is that this API would naturally lend itself to integration with the Designer. For example, the TaskDialogPage may be designable component, and a user could design one (or many) pages, that are later shown in a dialog:

// MyForm.Designer.cs

    private void InitializeComponent()
    {
        // ...

        // 
        // startDialogPage
        //
        startDialogPage.Buttons = new TaskDialogButtonCollection { buttonYes, buttonCancel };
        startDialogPage.Caption = "Confirmation";
        startDialogPage.DefaultButton = buttonCancel
        startDialogPage.Icon = TaskDialogIcon.ShieldWarningYellowBar;
        startDialogPage.MainInstruction = "Stopping the operation might break things...";
        startDialogPage.Text = "Are you sure you want to do this?";
        // 
        // selectionDialogPage
        //
        selectionDialogPage.Buttons = ...;
        selectionDialogPage.Caption = ...;
    }

    private System.Windows.Forms.TaskDialogButton buttonYes;
    private System.Windows.Forms.TaskDialogButton buttonCancel;
    private System.Windows.Forms.TaskDialogPage startDialogPage;
    private System.Windows.Forms.TaskDialogPage selectionDialogPage;
// MyForm.cs

    private void ShowDialog()
    {
        TaskDialogButton clickedButton = TaskDialog.ShowDialog(this, startDialogPage);
    }

Dealing API discoverability issue: the current API does not advertise the multipage functionality

TBD

RussKie commented 4 years ago

We had few design sessions with the team and there was a heavy focus on the Designer integration and interaction to enable users to design dialog pages and the multipage flow.

The team has endorsed the proposal to reduce the public API surface to only static entry points and look like the following:

    public static TaskDialogButton ShowDialog(TaskDialogPage page);
    public static TaskDialogButton ShowDialog(IntPtr hwndOwner, TaskDialogPage page);
    public static TaskDialogButton ShowDialog(IWin32Window owner, TaskDialogPage page);

This design should reduce the cognitive load required to learn the API and differences between them. It also should provide a straight forward path to API discoverability by exposing the notion of Page. And it will work with the Designer well too. The design will allow us to simplify and reduce the TaskDialog's public API surface removing public ctors, Page property and events from it (they can be converted to non-public, if necessary).

There is an aspect of testability that has been considered as well, i.e. it is near impossible to test a UI control that shows a MessageBox (unless introducing some kind of wrapper). We certainly wouldn't want to repeat the same mistake and inhibit testability of UI controls that chose to use TaskDialog. However I feel it should be possible to programmatically close the dialog from unit tests, if necessary, through bound pages (e.g. sut.GetTestAccessor().taskDialogPage1.BoundDialog.Close()).

To facilitate the navigation to another page we propose to add TaskDialogPage.Navigate(TaskDialogPage page), that will allow to write something like:

button1.Click += (s, e) => button1.BoundPage.Navigate(page2);

To accommodate the designer (when we get to it) we will likely add a new property TaskDialogPage TaskDialogButton.NavigateToPage { get; set; }. This way a user will be able to design pages and link them together as necessary. Having no spent much time on this, I think it would work something like:

  1. If TaskDialogButton.NavigateToPage == null, no changed to the button behaviours;
  2. If TaskDialogButton.NavigateToPage != null and TaskDialogButton.Click == null, set TaskDialogButton.AllowCloseDialog = false and (internally) wire Click to navigate to the chosen page.
  3. If TaskDialogButton.NavigateToPage != null and TaskDialogButton.Click != null, fail? 😄 j/k we'll figure when we get to it.

@kpreisser what do you think about the proposal?

RussKie commented 4 years ago

I opened a PR #14 implementing the proposal.

kpreisser commented 4 years ago

Hi @RussKie, thanks a lot for the detailed proposal and the PR! (And sorry for the delay, I was a bit busy recently.)

I think the propsal looks good. Note that the TaskDialog still has instance property TaskDialogStartupLocation that defines the position where the dialog should show. Maybe we will need to move it to the static ShowDialog methods as parameter. (It only has an effect when showing the dialog, but not when navigating it, so I think it doesn't belong to TaskDialogPage).

Similarly, TaskDialog has property SetToForeground (for flag NO_SET_FOREGROUND), but I think we do not need it initially. and I'm not 100% certain if it only has an effect when showing the dialog, or if it can also have an effect when navigating the dialog.

Regarding navigation with TaskDialogPage.Navigate() (and supplying the initial page in ShowDialog(), I think this is similar to your suggestion about calling SetNextPage() for navigation, where I noted that when specifying a page in the ShowDialog() method, the API looks like the page would be shown until the dialog closes (as ShowDialog() is blocking), whereas in fact it will only be the initial page, but it can replaced with other pages by calling TaskDialog.SetNextPage() (here: TaskDialogPage.Navigate()) while that method is still being called. However, I think I can live with this API, if it makes the dialog easier to use.

Thank you!

RussKie commented 4 years ago

thanks a lot for the detailed proposal and the PR! (And sorry for the delay, I was a bit busy recently.)

No worries, totally understandable.

think the propsal looks good. Note that the TaskDialog still has instance property TaskDialogStartupLocation that defines the position where the dialog should show. Maybe we will need to move it to the static ShowDialog methods as parameter. (It only has an effect when showing the dialog, but not when navigating it, so I think it doesn't belong to TaskDialogPage).

That's a good point, and a sensible proposal. We can certainly add it as an optional parameter.

imilarly, TaskDialog has property SetToForeground (for flag `TDF_, but I think we do not need it initially. and I'm not 100% certain if it only has an effect when showing the dialog, or if it can also have an effect when navigating the dialog.

Let's just remove it completely. If/when we found a compelling use-case we can certainly look into implementing its support.

Regarding navigation with TaskDialogPage.Navigate() (and supplying the initial page in ShowDialog(), I think this is similar to your suggestion about calling SetNextPage() for navigation, where I noted that when specifying a page in the ShowDialog() method, the API looks like the page would be shown until the dialog closes (as ShowDialog() is blocking), whereas in fact it will only be the initial page, but it can replaced with other pages by calling TaskDialog.SetNextPage() (here: TaskDialogPage.Navigate()) while that method is still being called. However, I think I can live with this API, if it makes the dialog easier to use.

API design is hard, I'm sure you know it now 😄 Personally I don't get an expectation that the page will stick until the end. To me "one page" leads to "many pages". But maybe it's just me, because I have spent too much time thinking about it. The team shared a similar notion. But this is not to say that some developers couldn't read it in the way you describe. We have to publish your work, so we can start collecting developer feedback and sentiments.