Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API Review: DragStarting Event API #4900

Merged
merged 9 commits into from
Nov 13, 2024
Merged

API Review: DragStarting Event API #4900

merged 9 commits into from
Nov 13, 2024

Conversation

johna-ms
Copy link
Contributor

This is an API review for the DragStarting event API

/// This interface is implemented by the
/// Microsoft.Web.WebView2.Core.CoreWebView2CompositionController runtime class.
[uuid(7a4daef9-1701-463f-992d-2136460cf76e), object, pointer_default(unique)]
interface ICoreWebView2StagingCompositionControllerInterop : IUnknown {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Remove Staging from interface names

Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix.

# API Details
```C++
/// Flags enum that represents the effects that a given WebView2 drag drop
/// operation can have. The values of this enum align with the ole DROPEFFECT
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a link to any public doc here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix. (Also ole -> OLE)

specs/DragStarting.md Show resolved Hide resolved
specs/DragStarting.md Show resolved Hide resolved
# Examples
## DragStarting
Users can use `add_DragStarting` on the CompositionController to add an event
handler that is invoked when drag is starting. They can use the the event args
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
handler that is invoked when drag is starting. They can use the the event args
handler that is invoked when drag is starting. They can use the event args

Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix

/// constant with the exception of DROPEFFECT_SCROLL which is only relevant for
/// drop and therefore unsupported.
[v1_enum]
typedef enum COREWEBVIEW2_DRAG_EFFECTS {
Copy link
Member

Choose a reason for hiding this comment

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

The existing CompositionController APIs for DragEnter, DragOver, and Drop use DWORD* for the DROPEFFECT. Should we follow that existing pattern?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes let's use a DWORD here. It simplifies expected usage of the API and is consistent with our existing drag/drop WebView2 APIs.

# Examples
## DragStarting
Users can use `add_DragStarting` on the CompositionController to add an event
handler that is invoked when drag is starting. They can use the the event args
Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix

specs/DragStarting.md Show resolved Hide resolved
# API Details
```C++
/// Flags enum that represents the effects that a given WebView2 drag drop
/// operation can have. The values of this enum align with the ole DROPEFFECT
Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix. (Also ole -> OLE)

/// constant with the exception of DROPEFFECT_SCROLL which is only relevant for
/// drop and therefore unsupported.
[v1_enum]
typedef enum COREWEBVIEW2_DRAG_EFFECTS {
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes let's use a DWORD here. It simplifies expected usage of the API and is consistent with our existing drag/drop WebView2 APIs.

[uuid(edb6b243-334f-59d0-b3b3-de87dd401adc), object, pointer_default(unique)]
interface ICoreWebView2DragStartingEventArgs : IUnknown {
/// The operations this drag data supports.
[propget] HRESULT AllowedOperations(
Copy link
Contributor

Choose a reason for hiding this comment

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

Please change to AllowedDropEffects

specs/DragStarting.md Show resolved Hide resolved
[propget] HRESULT Data([out, retval] IDataObject** value);

/// The position at which drag was detected. This position is given in
/// screen pixel coordinates as opposed to WebView2 relative coordinates.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update docs to explain DPI story. It should be WebView2 client coord space like what DragOver does

specs/DragStarting.md Show resolved Hide resolved
/// This interface is implemented by the
/// Microsoft.Web.WebView2.Core.CoreWebView2CompositionController runtime class.
[uuid(7a4daef9-1701-463f-992d-2136460cf76e), object, pointer_default(unique)]
interface ICoreWebView2StagingCompositionControllerInterop : IUnknown {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix.

specs/DragStarting.md Show resolved Hide resolved
@johna-ms johna-ms merged commit 47b5622 into main Nov 13, 2024
@johna-ms johna-ms deleted the api-dragstarting branch November 13, 2024 00:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Proposal Review WebView2 API Proposal for review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants