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

Refactor ListView to use Dictionary instead of Hashtable #8168

Merged
merged 3 commits into from
Nov 14, 2022

Conversation

elachlan
Copy link
Contributor

@elachlan elachlan commented Nov 11, 2022

Related: #8143

Microsoft Reviewers: Open in CodeFlow

@elachlan elachlan requested a review from a team as a code owner November 11, 2022 05:26
@ghost ghost assigned elachlan Nov 11, 2022
@@ -80,7 +61,7 @@ public bool OwnerIsDesignMode

if (_owner.IsHandleCreated && !_owner.ListViewHandleDestroyed)
{
return (ListViewItem)_owner._listItemsTable[DisplayIndexToID(displayIndex)];
return _owner._listItemsTable[DisplayIndexToID(displayIndex)];
Copy link
Contributor

Choose a reason for hiding this comment

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

This will lead to a regression. Check #7089 and related PRs. We could use TryGetValue here, but I was never able to reproduce the problem with a test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@RussKie When you investigated this originally, was TryGetValue evaluated as a solution? Or was the issue more sinister than that?

Copy link
Member

Choose a reason for hiding this comment

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

IIRC, whilst the signatures are the same, the behaviours and null value handling is different between Hashtable and Dictionary<T,V> types. TryGetValue was found to be the solution.

@RussKie
Copy link
Member

RussKie commented Nov 14, 2022

Could you please confirm we have some test coverage for the changed code?

RussKie
RussKie previously approved these changes Nov 14, 2022
@RussKie RussKie added the 📭 waiting-author-feedback The team requires more information from the author label Nov 14, 2022
@ghost ghost removed the 📭 waiting-author-feedback The team requires more information from the author label Nov 14, 2022
@elachlan
Copy link
Contributor Author

Could you please confirm we have some test coverage for the changed code?

It seems like the changes are indirectly covered by tests. But there is no direct testing of the code.

@RussKie RussKie enabled auto-merge (squash) November 14, 2022 04:10
@RussKie RussKie merged commit 7f0a54d into dotnet:main Nov 14, 2022
@ghost ghost added this to the 8.0 Preview1 milestone Nov 14, 2022
@elachlan elachlan deleted the ListView-Dictionary branch November 14, 2022 04:25
@ghost ghost locked as resolved and limited conversation to collaborators Dec 14, 2022
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