dotnet / winforms

Windows Forms is a .NET UI framework for building Windows desktop applications.
MIT License
4.41k stars 984 forks source link

support drag and drop with icon and text label #5884

Closed ghost closed 2 years ago

ghost commented 3 years ago

💥 💥 ⬇️⬇️ Scroll to the API proposal ⬇️⬇️💥 💥


About Winform drag and drop: https://docs.microsoft.com/en-us/dotnet/desktop/winforms/input-mouse/drag-and-drop

Winforms supports drag and drop, but it displays the Windows 2000/XP old school style.

003

Unlike win10, win10 can provide richer display effects. If you drag and drop a file on Win10, Win10 will display an icon and a text label. Then you can know your specific operation through the text label.

Picture 1: Drag a file:

001

Picture 2: When you hold down the Control or Alt key, the icon and text will also be changed.

002-2

Picture 3: Drag a file to application shortcut, it will display:

004

So if the drag and drop can provide an Icon and Text properties, the application can display a more detailed operating instruction to users, and the application will be more modern.

willibrandon commented 2 years ago

Just a quick update that I had to focus on some pressing matters the last couple of weeks but I'm ready to pick this back up.

I had another thought regarding the drag-image and to me it seems that whoever authors the API proposal should have good knowledge regarding bitmaps and how to work with them in GDI. For anyone that is interested, the documentation is a good place to start and another more digestible resource is Charles Petzold's book, fifth edition, chapters 14, 15, and 16 which covers Bitmaps and Bitblts, The Device-Independent Bitmap, and The Palette Manager.

willibrandon commented 2 years ago

I've started a draft PR in #6576 to show how I would propose adding support for drop targets to display drag images and set drop descriptions. So far I've only added support for DropTarget and further work needs to be done in order to add in the support for RichTextBox and ToolStripDropTargetManager.

Note that my implementation omits the ability for the application to set the drag image from a drag source. That part requires a little more work to support the drag-and-drop helper's calls to IDataObject::SetData and IDataObject::GetData to load and retrieve the drag-and-drop data formats. It's not particularly hard to do, I think it's more of a question of precisely how it should be done, the implementation details. The public API could be a Control.DoDragDrop overload Control.DoDragDrop(object data, DragDropEffects allowedEffects, Bitmap dragImage) which would allow the application to specify the drag image bitmap.

This is where I'm not entirely sure how to proceed. As far as I can tell, that is all that would be required in terms of a public API proposal. Should I proceed and show how I would add IDragSourceHelper/IDragSourceHelper2 support in the draft PR?

RussKie commented 2 years ago

I had a look at https://github.com/dotnet/winforms/pull/6576, and to proceed

I've started a draft PR in #6576 to show how I would propose adding support for drop targets to display drag images and set drop descriptions. So far I've only added support for DropTarget and further work needs to be done in order to add in the support for RichTextBox and ToolStripDropTargetManager.

👍

This is where I'm not entirely sure how to proceed. As far as I can tell, that is all that would be required in terms of a public API proposal. Should I proceed and show how I would add IDragSourceHelper/IDragSourceHelper2 support in the draft PR?

That, and we'll need a formal API proposal, which we'll take to the API Review Board. A draft PR (and any relevant screenshots/animations) makes it significantly easier to pass the ARB review. Here's our guide for submitting an API proposal. You can also look through the list of the approved API proposals under https://github.com/dotnet/winforms/issues?q=label%3Aapi-approved.

willibrandon commented 2 years ago

@RussKie - Thank you for the feedback. I'll start working on it along with the next step and I'll also start preparing the proposal.

Is there something that I should think about, or do, or test specifically in terms of accessibility? A search on accessibility with drag and drop has me wondering about keyboard and screen reader users.

RussKie commented 2 years ago

I'll start working on it along with the next step and I'll also start preparing the proposal.

🚀

Is there something that I should think about, or do, or test specifically in terms of accessibility? A search on accessibility with drag and drop has me wondering about keyboard and screen reader users.

Our go-to tool is Accessibility Insights. Generally running a Fast Pass is sufficient. More info: (https://accessibilityinsights.io/docs/en/web/getstarted/fastpass). You're right, the DnD gesture doesn't feel like something a keyboard user would perform... @merriemcgaw @Tanya-Solyanik @vladimir-krestov what do you think?

merriemcgaw commented 2 years ago

I agree with both of you that Drag and Drop wouldn't be a major keyboard experience. Application developers would need to ensure that there's no place where drag and drop replaces a keyboard functionality. We'd expect to see Drag and Drop as a complement to a keyboard accessible feature.

We could discuss things like making UIA notifications available so that vision-impaired users that can use the mouse can also hear a notification message about where you dropped the control. That would likely fall on the drop location and the dropped event.

Something that I would test is how things look in the various High Contrast modes, or give developers the ability to set different images for High Contrast. I don't know for sure how much Accessibility Insights would test drag and drop effectively. Let me ask the local Subject Matter Experts about if/how they'd expect it to work in a desktop application. I'll circle back but aside from high contrast I don't think there's much here.

willibrandon commented 2 years ago

Background and motivation

As mentioned by @roland5572 at the start of this issue, Winforms currently supports drag and drop but it displays the old school style drag cursor with no support for drag images or drop descriptions. Since Windows Vista, drag and drop has the ability for an application to set and display drag images and drop descriptions to provide the user with additional details regarding the operation to be performed. In order to deliver an improved drag and drop experience, I propose we add support for drag images and drop descriptions in Winforms.

I've opened a draft PR in #6576 with a possible implementation.

Winforms drag and drop.

https://user-images.githubusercontent.com/5017479/159851796-cf6b8176-97ed-4ec7-8c27-aa74d1d058f7.mp4

Winforms drag and drop with drag images and drop descriptions.

https://user-images.githubusercontent.com/5017479/162199945-092b21d0-3d03-49b6-a79d-b2b47e6e42c7.mp4

API proposal

namespace System.Windows.Forms;

public enum DropIconType
{
    Default = -1,
    None = 0,
    Copy = DragDropEffects.Copy,
    Move = DragDropEffects.Move,
    Link = DragDropEffects.Link,
    Label = 6,
    Warning = 7,
    NoDropIcon = 8
}

public class DragEventArgs : EventArgs
{
    // Existing:
    // public DragEventArgs(IDataObject? data, int keyState, int x, int y, DragDropEffects allowedEffect, DragDropEffects effect);

    public DragEventArgs(IDataObject? data, int keyState, int x, int y, DragDropEffects allowedEffect, DragDropEffects effect, DropIconType dropIcon, string message, string insert);

    // Existing:
    // public IDataObject? Data { get; }
    // public int KeyState { get; }
    // public int X { get; }
    // public int Y { get; }
    // public DragDropEffects AllowedEffect { get; }
    // public DragDropEffects Effect { get; set; }

    public DropIconType DropIcon { get; set; }
    public string Message { get; set; }
    public string MessageReplacementToken { get; set; }
}

public class GiveFeedbackEventArgs : EventArgs
{
    // Existing:
    // public GiveFeedbackEventArgs(DragDropEffects effect, bool useDefaultCursors);

    public GiveFeedbackEventArgs(DragDropEffects effect, bool useDefaultCursors, Bitmap? dragImage, Point cursorOffset, bool useDefaultDragImage);

    // Existing:
    // public DragDropEffects Effect { get; }
    // public bool UseDefaultCursors { get; set; }

    public Bitmap? DragImage { get; set; }
    public Point CursorOffset { get; set; }
    public bool UseDefaultDragImage { get; set; }
}

public partial class Control
{
    // Existing:
    // public DragDropEffects DoDragDrop(object data, DragDropEffects allowedEffects);

    public DragDropEffects DoDragDrop(object data, DragDropEffects allowedEffects, Bitmap dragImage, Point cursorOffset, bool useDefaultDragImage);
}

public partial class ToolStripItem
{
    // Existing:
    // public DragDropEffects DoDragDrop(object data, DragDropEffects allowedEffects);

    public DragDropEffects DoDragDrop(object data, DragDropEffects allowedEffects, Bitmap dragImage, Point cursorOffset, bool useDefaultDragImage);
}
API Diff with comments ```diff namespace System.Windows.Forms { + /// + /// Specifies the drop description icon type. + /// + public enum DropIconType + { + /// + /// No drop icon preference; use the default icon. + /// + Default = -1, + + /// + /// A red bisected circle such as that found on a "no smoking" sign. + /// + None = 0, + + /// + /// A plus sign (+) that indicates a copy operation. + /// + Copy = DragDropEffects.Copy, + + /// + /// An arrow that indicates a move operation. + /// + Move = DragDropEffects.Move, + + /// + /// An arrow that indicates a link. + /// + Link = DragDropEffects.Link, + + /// + /// A tag icon that indicates that the metadata will be changed. + /// + Label = 6, + + /// + /// A yellow exclamation mark that indicates that a problem has been encountered in the operation. + /// + Warning = 7, + + /// + /// Windows 7 and later. Use no drop icon. + /// + NoDropIcon = 8 + } } ``` ```diff namespace System.Windows.Forms { public class DragEventArgs : EventArgs { public DragEventArgs(IDataObject? data, int keyState, int x, int y, DragDropEffects allowedEffect, DragDropEffects effect) { ... } + + /// + /// Initializes a new instance of the class. + /// + public DragEventArgs(IDataObject? data, int keyState, int x, int y, DragDropEffects allowedEffect, DragDropEffects effect, DropIconType dropIcon, string message, string insert); public IDataObject? Data { get; } public int KeyState { get; } public int X { get; } public int Y { get; } public DragDropEffects AllowedEffect { get; } public DragDropEffects Effect { get; set; } + /// + /// Gets or sets the drop description icon type. + /// + public DropIconType DropIcon { get; set; } + + /// + /// Gets or sets the drop description text such as "Move to %1". + /// + /// + /// UI coloring is applied to the text in MessageReplacementToken if used by specifying %1 in Message. + /// + public string Message { get; set; } + + /// + /// Gets or sets the drop description text such as "Documents" when %1 is specified in Message. + /// + /// + /// UI coloring is applied to the text in MessageReplacementToken if used by specifying %1 in Message. + /// + public string MessageReplacementToken { get; set; } } } ``` ```diff namespace System.Windows.Forms { public class GiveFeedbackEventArgs : EventArgs { public GiveFeedbackEventArgs(DragDropEffects effect, bool useDefaultCursors) { ... } + + /// + /// Initializes a new instance of the class. + /// + public GiveFeedbackEventArgs(DragDropEffects effect, bool useDefaultCursors, Bitmap? dragImage, Point cursorOffset, bool useDefaultDragImage); public DragDropEffects Effect { get; } public bool UseDefaultCursors { get; set; } + /// + /// Gets or sets the drag image bitmap. + /// + public Bitmap? DragImage { get; set; } + + /// + /// Gets or sets the drag image cursor offset. + /// + public Point CursorOffset { get; set; } + + /// + /// Gets or sets a value indicating whether a layered window drag image is used. + /// + public bool UseDefaultDragImage { get; set; } } } ``` ```diff namespace System.Windows.Forms { public partial class Control { public DragDropEffects DoDragDrop(object data, DragDropEffects allowedEffects); + /// + /// Begins a drag operation and sets the initial drag image, cursor offset, and whether to use the + /// default drag image. If an argument of true is specified for useDefaultDragImage, the result is a + /// layered window drag image with a size of 96x96. + /// + public DragDropEffects DoDragDrop(object data, DragDropEffects allowedEffects, Bitmap dragImage, Point cursorOffset, bool useDefaultDragImage); } public abstract partial class ToolStripItem { public DragDropEffects DoDragDrop(object data, DragDropEffects allowedEffects); + /// + /// Begins a drag operation and sets the initial drag image, cursor offset, and whether to use the + /// default drag image. If an argument of true is specified for useDefaultDragImage, the result is a + /// layered window drag image with a size of 96x96. + /// + public DragDropEffects DoDragDrop(object data, DragDropEffects allowedEffects, Bitmap dragImage, Point cursorOffset, bool useDefaultDragImage); } } ```

Usage

Set the initial drag image during Control.DoDragDrop or ToolStripItem.DoDragDrop.

private void PictureBox_MouseDown(object? sender, MouseEventArgs e)
{
    pictureBox.DoDragDrop(pictureBox.Name, DragDropEffects.All, new Bitmap(@"Data\DragDrop\NyanCat1.bmp"), new Point(0, 96), false);
}

Set the drop description during the Control.DragEnter event.

private void TextBox_DragEnter(object? sender, DragEventArgs e)
{
    // Set the target drop icon to a plus sign (+).
    e.DropIcon = DropIconType.Copy;

    // Set the target drop text.
    e.Message = "Copy cat to %1";
    e.MessageReplacementToken = "~=[,,_,,]:3";
}

Set or update the drag image during the Control.GiveFeedback event.

private void PictureBox_GiveFeedback(object? sender, GiveFeedbackEventArgs e)
{
    // Note the outer edges of the drag image are blended out if the width or height exceeds 300 pixels.
    e.DragImage = new Bitmap(@"Data\DragDrop\NyanCatAscii_301.bmp");

    // Set the cursor to the bottom left-hand corner of the drag image.
    e.CursorOffset = new Point(0, 111);
}

The result.

Usage_DragImageDropDescription

Will this feature affect UI controls?

Yes. When the drag image and drop description is displayed over the UI controls, it looks incredible.

Will VS Designer need to support the feature? If yes, describe how you expect it to funсtion.

This feature does not require VS Designer support at this time.

What impact will it have on accessibility?

We need test how things look in the various High Contast modes.

Will this feature need to be localized or be localizable?

Not that I'm aware of.

API Updates

willibrandon commented 2 years ago

Thank you everyone for being so patient, this isn't something that is documented very well and I went in not knowing any of this stuff. I never would have guessed what path this would lead me down. I now know the data object intimately and am familiar with the various formats and storage mediums, and how to read and write to and from the global memory handles. It feels like somewhat uncharted territory to me which makes it very interesting. Thank you for the opportunity to take a look at this.

merriemcgaw commented 2 years ago

This is absolutely fantastic! No worries about taking time - it's your feature 😄. The next step is our internal team review of the API proposal, and then we'll take it to the .NET API proposal meeting for the official review. We're in the middle of some very high priority work at the moment, but we'll get to the WinForms review sometime in the next couple of weeks.

This is amazing work and we're excited to take this into the API! Thanks for all the hard work you've put into this so far.

willibrandon commented 2 years ago

After receiving excellent feedback from @weltkante in the draft PR, he intimated that his ideal design would also include the option to set the drag image from Control.DoDragDrop.

I liked the DoDragDrop arguments a bit more because it makes one-off calls easier where you set the image once for the whole operation and don't need to update it (the most common case). Normally the default implementation of GiveFeedback in WinForms is sufficient and I tend to not handle the event at all. Having to implement it to get to the new APIs seems a bit bothersome, but it does give the advantage of being able to update the image if needed.

In an ideal design I'd probably want both, being able to set the initial image when starting the operation, and optionally updating it in the GiveFeedback/QueryContinueDrag calls. But given the amount of work involved implementing all that I'm fine with settling with a simpler API.

If his ideal design is to set the drag image from Control.DoDragDrop and Control.GiveFeedback/Control.QueryContinueDrag, then I'm very inclined to include his request in the API proposal. I'll start looking at this now and will update the proposal asap.

weltkante commented 2 years ago

Feels a bit awkward to be emphasized that much, so just in case anyone is wondering, the phrasing "ideal design" is meant to relate to exposing the capabilities of the native API faithfully to the managed world. The design of the native API allows to set up the extensions once, when initiating the drag operation, but it also includes the option to update it at a later point. Exposing both capabilities would be "ideal" as it allows you to cover the same scenarios the native API supports.

As explained in the quoted response, the most common scenario should be setting up the image just once, having to implement an event handler just for that seems awkward. Keeping the ability to update the image during the operation would be nice to have.

RussKie commented 2 years ago

Awesome work @willibrandon!

namespace System.Windows.Forms
{
    public class DragEventArgs : EventArgs
    {
        /// <summary>
        /// Gets or sets the drop description icon type.
        /// </summary>
        public DropIconType DropIcon { get; set; }

Generally, EventArg properties should be read-only and initialised via the .ctor. Any reason to have these mutable? Ditto for other EventArg.

    /// <summary>
    /// Gets or sets the drop description text such as "Move to %1".
    /// </summary>
    /// <remarks>
    /// <para>UI coloring is applied to the text in Insert if used by specifying %1 in Message.</para>
    /// </remarks>
    public string Message { get; set; }

    /// <summary>
    /// Gets or sets the drop description text such as "Documents" when %1 is specified in Message.
    /// </summary>
    /// <remarks>
    /// <para>UI coloring is applied to the text in Insert if used by specifying %1 in Message.</para>
    /// </remarks>
    public string Insert { get; set; }

I'm struggling to provide a better name, but "Insert" on its own doesn't look very descriptive. Maybe MessageInsert or MessageReplacementToken?

        /// <summary>
        ///  Initializes a new instance of the <see cref="DragEventArgs"/> class.
        /// </summary>
        public DragEventArgs(IDataObject? data, int keyState, int x, int y, DragDropEffects allowedEffect, DragDropEffects effect, DropIconType dropIcon, string message, string insert);
    }
}

data, keyState, x, y, allowedEffect, effect don't appear to be used.

namespace System.Windows.Forms
{
    public class GiveFeedbackEventArgs : EventArgs
    {
        /// <summary>
        ///  Gets or sets the drag image bitmap.
        /// </summary>
        public Bitmap? DragImage { get; set; }

        /// <summary>
        ///  Gets or sets the drag image cursor offset.
        /// </summary>
        public Point CursorOffset { get; set; }

        /// <summary>
        ///  Gets or sets a value indicating whether a layered window drag image is used.
        /// </summary>
        public bool UseDefaultDragImage { get; set; }

         /// <summary>
        ///  Initializes a new instance of the <see cref="GiveFeedbackEventArgs"/> class.
        /// </summary>
        public GiveFeedbackEventArgs(DragDropEffects effect, bool useDefaultCursors, Bitmap? dragImage, Point cursorOffset, bool useDefaultDragImage);
    }
}

Same here, two out of five parameters don't appear to be used.

useDefaultDragImage name looks confusing. I'm glad you added visual explanations for it earlier. May be call this something like UseLayeredWindowDragImage? Also, as you indicated, the layering appears to be the default effect in Explorer, and I think we should default to true as well - i.e., add another .ctor:

public GiveFeedbackEventArgs(DragDropEffects effect, bool useDefaultCursors, Bitmap? dragImage, Point cursorOffset) : this(...., useDefaultDragImage: true);
public GiveFeedbackEventArgs(DragDropEffects effect, bool useDefaultCursors, Bitmap? dragImage, Point cursorOffset, bool useDefaultDragImage);
willibrandon commented 2 years ago

I'm struggling to provide a better name, but "Insert" on its own doesn't look very descriptive. Maybe MessageInsert or MessageReplacementToken?

I agree it needs a more descriptive name. How does MessageFormat sound? The drop description szInsert member is technically a FormatMessage which uses the subset %% and %1 as format message markers.

Some UI coloring is applied to the text in szInsert if used by specifying %1 in szMessage. The characters %% and %1 are the subset of FormatMessage markers that are processed here.

Perhaps the name should also indicate that UI coloring is applied?

RussKie commented 2 years ago

How does MessageFormat sound?

To me this sounds more what Message field should be called, as it defines the format of the message, i.e., contains the replacement sequences. Would MessageReplacementToken work?

Some UI coloring is applied to the text in szInsert if used by specifying %1 in szMessage. The characters %% and %1 are the subset of FormatMessage markers that are processed here.

Perhaps the name should also indicate that UI coloring is applied?

I think if we mention this in the intellisense/docs - it would suffice.

willibrandon commented 2 years ago

If EventArg properties should only be read-only, then we might need another way to set the drag image and drop description.

weltkante commented 2 years ago

There are two kind of events: "notifications" (read-only) and "information requests" (mutable), the latter being designed so listeners can "vote" for what the result should be. Since event handlers cannot return values it is common to store the result in properties on the event. For example CancelEventArgs allows voting for cancellation, there are also various precedents in the data binding infrastructure where you store the formatted values in the event, for example ListBox.Format uses ListControlConvertEventArgs to generate strings for data items - you can have multiple listeners each handling different aspects (e.g. different data types). Seems like a sensible design to use mutable events here too.

RussKie commented 2 years ago

@merriemcgaw @dreddy-work the API proposed in https://github.com/dotnet/winforms/issues/5884#issuecomment-1077262077 look good to me, and I recommend engaging the ARB for their formal approval.

terrajobst commented 2 years ago

Video

namespace System.Windows.Forms;

public enum DropImageType
{
    Invalid = -1,
    None = 0,
    Copy = DragDropEffects.Copy,
    Move = DragDropEffects.Move,
    Link = DragDropEffects.Link,
    Label = 6,
    Warning = 7,
    NoImage = 8
}

public class DragEventArgs : EventArgs
{
    // Existing:
    // public DragEventArgs(IDataObject? data, int keyState, int x, int y, DragDropEffects allowedEffect, DragDropEffects effect);

    public DragEventArgs(IDataObject? data, int keyState, int x, int y, DragDropEffects allowedEffect, DragDropEffects effect, DropImageType dropIcon, string message, string insert);

    // Existing:
    // public IDataObject? Data { get; }
    // public int KeyState { get; }
    // public int X { get; }
    // public int Y { get; }
    // public DragDropEffects AllowedEffect { get; }
    // public DragDropEffects Effect { get; set; }

    public DropImageType DropImageType { get; set; }
    public string? Message { get; set; }
    public string? MessageReplacementToken { get; set; }
}

public class GiveFeedbackEventArgs : EventArgs
{
    // Existing:
    // public GiveFeedbackEventArgs(DragDropEffects effect, bool useDefaultCursors);

    public GiveFeedbackEventArgs(DragDropEffects effect, bool useDefaultCursors, Bitmap? dragImage, Point cursorOffset, bool useDefaultDragImage);

    // Existing:
    // public DragDropEffects Effect { get; }
    // public bool UseDefaultCursors { get; set; }

    public Bitmap? DragImage { get; set; }
    public Point CursorOffset { get; set; }
    public bool UseDefaultDragImage { get; set; }
}

public partial class Control
{
    // Existing:
    // public DragDropEffects DoDragDrop(object data, DragDropEffects allowedEffects);

    public DragDropEffects DoDragDrop(object data, DragDropEffects allowedEffects, Bitmap dragImage, Point cursorOffset, bool useDefaultDragImage);
}

public partial class ToolStripItem
{
    // Existing:
    // public DragDropEffects DoDragDrop(object data, DragDropEffects allowedEffects);

    public DragDropEffects DoDragDrop(object data, DragDropEffects allowedEffects, Bitmap dragImage, Point cursorOffset, bool useDefaultDragImage);
}
willibrandon commented 2 years ago

That is awesome. I agree with the suggested changes and will start working on updating the proposal and draft PR.

Thank you everyone for taking a look at this. I understand that time is short and I greatly appreciate everyone's effort on this issue.

Olina-Zhang commented 2 years ago

@willibrandon Is this work completed? Since we saw the related PR is merged and tested it in the latest .Net 7.0 build.

RussKie commented 2 years ago

Yes, @Olina-Zhang, this has been resolved by #6576. Thank you for the nudge.

Olina-Zhang commented 2 years ago

Verified on .NET 7.0 test pass build: .NET 7.0.100-preview.7.22370.3, issue was fixed: it supports dragging and dropping pictures and RTF files now.

willibrandon commented 2 years ago

@roland5572 - I'm not sure how we're going to top this one. Good idea.

RussKie commented 2 years ago

Haha, we have plenty of issue in the back log 😉 I'm sure you can find something to your taste there, e.g.: https://github.com/dotnet/winforms/labels/api-approved and https://github.com/dotnet/winforms/labels/up-for-grabs

Thanks again for your contributions.