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

Use List<T> instead of ICollection in BehaviorDataObject #10278

Merged
merged 2 commits into from
Nov 13, 2023

Conversation

halgab
Copy link
Contributor

@halgab halgab commented Nov 11, 2023

Finish work started in #8673
Related: #8140

Now that DropSourceBehavior is passed a List<IComponent> in its constructor, we can update BehaviorDataObject to do the same instead of using ICollection

Proposed changes

  • Refactor BehaviorDataObject.DragComponents to be a List<IComponent> and update code accordingly
  • Another small BehaviorDataObject refacto while we're touching this code, but in a separate commit
Microsoft Reviewers: Open in CodeFlow

@ghost ghost assigned halgab Nov 11, 2023
@ghost ghost added the draft draft PR label Nov 11, 2023
@halgab halgab marked this pull request as ready for review November 11, 2023 16:03
@halgab halgab requested a review from a team as a code owner November 11, 2023 16:03
@ghost ghost removed the draft draft PR label Nov 11, 2023
@@ -1478,8 +1478,7 @@ protected override void OnDragEnter(DragEventArgs de)
object[] dragComps;
Copy link
Contributor

@elachlan elachlan Nov 12, 2023

Choose a reason for hiding this comment

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

dragComps can now be IComponent[]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't look that simple... OleDragDropHandler.GetDraggingObjects return an object[]. I think it can be refactored to return an IComponent[], but that's more involved. I'm not sure I should add it to this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at it again, it might be possible, but I'm not comfortable with making such a change. That would lead to assumptions that I'm not sure would hold all the time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it depends if the team ever decided to implement another Component interface.

@elachlan
Copy link
Contributor

As apart of this change maybe refactor this?

private Control ExtractControlFromDragEvent(DragEventArgs de)
{
DropSourceBehavior.BehaviorDataObject data = de.Data as DropSourceBehavior.BehaviorDataObject;
if (data is not null)
{
dragComps = new ArrayList(data.DragComponents);
return dragComps[0] as Control;
}
return null;
}

Something like:

private Control ExtractControlFromDragEvent(DragEventArgs de) =>
    (de.Data as DropSourceBehavior.BehaviorDataObject)?.DragComponents.FirstOrDefault() as Control;

@halgab
Copy link
Contributor Author

halgab commented Nov 12, 2023

As apart of this change maybe refactor this?

private Control ExtractControlFromDragEvent(DragEventArgs de)
{
DropSourceBehavior.BehaviorDataObject data = de.Data as DropSourceBehavior.BehaviorDataObject;
if (data is not null)
{
dragComps = new ArrayList(data.DragComponents);
return dragComps[0] as Control;
}
return null;
}

Something like:

private Control ExtractControlFromDragEvent(DragEventArgs de) =>
    (de.Data as DropSourceBehavior.BehaviorDataObject)?.DragComponents.FirstOrDefault() as Control;

Nice catch, I hadn't noticed this one. dragComps is a field though, so we have to keep it around. We could definitely change it to be a List<IComponent> after DragComponents itself becomes generic. As it's a change to a field in another type, perhaps is it more suited to a follow-up PR?

@elachlan
Copy link
Contributor

I agree, follow up PR to make this change quicker to review.

@JeremyKuhne JeremyKuhne self-requested a review November 13, 2023 21:44
Copy link
Member

@JeremyKuhne JeremyKuhne left a comment

Choose a reason for hiding this comment

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

Thanks!

@JeremyKuhne JeremyKuhne merged commit 4319ec9 into dotnet:main Nov 13, 2023
9 checks passed
@ghost ghost added this to the 9.0 Preview1 milestone Nov 13, 2023
@halgab halgab deleted the BehaviorDataObject-refacto branch November 13, 2023 21:50
@halgab
Copy link
Contributor Author

halgab commented Nov 13, 2023

I agree, follow up PR to make this change quicker to review.

#10287

@github-actions github-actions bot locked and limited conversation to collaborators Dec 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants