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

[macOS] NSCollectionView.ValidateDrop should use 'ref', not 'out'. Fixes #60416 #2947

Merged
merged 2 commits into from Nov 10, 2017
Merged

[macOS] NSCollectionView.ValidateDrop should use 'ref', not 'out'. Fixes #60416 #2947

merged 2 commits into from Nov 10, 2017

Conversation

timrisi
Copy link
Contributor

@timrisi timrisi commented Nov 1, 2017

The two pointer parameters pass in the proposed index and operation to the method, then the developer has the option to change those values if desired inside the method. The current binding using 'out' parameters is never called when it is supposed to be in an application.

Because it is an existing binding, we cannot simply change the parameters to 'refs' and instead have to bind a new method using 'ref' and use code-behind to keep versions of the old binding around.

The new method also has to have a new name, because you can not have 2 methods with matching signatures where the only difference is 'ref' versus 'out.

@monojenkins
Copy link
Collaborator

Build success

src/appkit.cs Outdated
#else
[Mac (10,11)]
[Export ("collectionView:validateDrop:proposedIndexPath:dropOperation:")]
NSDragOperation ValidateDrop (NSCollectionView collectionView, [Protocolize (4)] NSDraggingInfo draggingInfo, ref NSIndexPath proposedDropIndexPath, ref NSCollectionViewDropOperation proposedDropOperation);
Copy link
Member

Choose a reason for hiding this comment

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

Minor: you can use INSDraggingInfo instead of [Protocolize (4)] NSDraggingInfo since this is a XAMCORE_4_0-only code path.

[Mac (10, 11)]
public virtual NSDragOperation ValidateDrop (NSCollectionView collectionView, NSDraggingInfo draggingInfo, out NSIndexPath proposedDropIndexPath, out NSCollectionViewDropOperation proposedDropOperation)
{
throw new InvalidOperationException ("Use 'ValidateDropOperation (NSCollectionView collectionView, NSDraggingInfo draggingInfo, ref NSIndexPath proposedDropIndexPath, ref NSCollectionViewDropOperation proposedDropOperation)' instead.");
Copy link
Member

Choose a reason for hiding this comment

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

Could these methods work in any scenario with the out parameter? If so, we should continue making it work by calling the ref overload instead of throwing an exception.

Same goes for the other methods below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I can tell they do not. The ValidateDrop method is called when you drag an object in the collection view. When you drag over a location, it calls Validatedrop to determine if it can be dropped there. With the 'out' parameter, the method is never called in the first place from what I've seen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose there's no real reason I couldn't make it call the ref version anyway

Copy link
Contributor

@spouliot spouliot left a comment

Choose a reason for hiding this comment

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

Please implement @rolfbjarne suggestion - it can only avoid compatibility issues :)

@monojenkins
Copy link
Collaborator

Build failure

@timrisi
Copy link
Contributor Author

timrisi commented Nov 9, 2017

Failure appears unrelated: https://github.com/xamarin/maccore/issues/573

@timrisi
Copy link
Contributor Author

timrisi commented Nov 9, 2017

build

@monojenkins
Copy link
Collaborator

Build success

@spouliot spouliot merged commit 09b994a into xamarin:master Nov 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants