dotnet / dotnet/winforms

Refactor `PropertyStore` and review/update usage

Open
#9,508 38 comments 1 reaction 0 assignees View on GitHub
api-suggestion enhancement tenet-performance
Dominant language
C#
Stars
4.9k
Forks
1.1k
Avg merge
20h 23m
Merged PRs (30d)
103

Description

### Background and motivation

We have had a few bugs caused by incorrect usage of `PropertyStore`. It's clunky and we can use private backing fields instead. This would allow nullable annotations and compile time checks which would have prevented #8990

Also can look into using a dictionary instead to gain perf improvements. But

Issue raised based on comment: https://github.com/dotnet/winforms/pull/9503#pullrequestreview-1533416827

### API Proposal

```csharp
internal class PropertyStore
{
private sealed class ValueWrapper where T : struct
{
public T Value { get; }

public ValueWrapper(T value)
{
Value = value;
}
}

private readonly Dictionary properties = new();

private static int s_currentKey = 0;

public static int CreateKey() => s_currentKey++;

public void SetValue(int key, T value) where T : struct
{
properties[key] = new ValueWrapper(value);
}

public void SetObject(int key, T value)
{
properties[key] = value;
}

public T GetValue(int key) where T : struct
{
if (properties.TryGetValue(key, out var value) && value is ValueWrapper wrapper)
{
return wrapper.Value;
}

return default;
}

public T? GetObject(int key)
{
return properties.TryGetValue(key, out var value) && value is T tValue ? tValue : default;
}

public bool TryGetValue(int key, out T? value) where T : struct
{
if (properties.TryGetValue(key, out var objValue) && objValue is ValueWrapper wrapper)
{
value = wrapper.Value;
return true;
}

value = default;
return false;
}

public bool TryGetObject(int key, out T? value) where T : class
{
if (properties.TryGetValue(key, out var objValue) && objValue is T tValue)
{
value = tValue;
return true;
}

value = null;
return false;
}

public bool Remove(int key) => properties.Remove(key);
}
```

### API Usage

N/A

### Alternative Designs

_No response_

### Risks

It is a large refactor and could cause subtle bugs. The current design is meant for performance reasons (I don't understand why?).

### Will this feature affect UI controls?

Controls will need testing.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.