Skip to content

Improve Padding TypeConverter CreateInstance implementation #2677

@weltkante

Description

@weltkante

As noticed in PR #2676 when changing method signatures to include nullable annotations the current implementation of the Padding TypeConverter CreateInstance override is very specifically coded for being called in context of a PropertyGrid.

The TypeConverter however is registered globally and available to anyone via the TypeDescriptor registration system, so it should follow documented semantics, in particular:

  • the context argument may be null
  • expect properties of the context object being able to return null
  • do not return true from GetCreateInstanceSupported if you are always going to throw for the given context

I think the intention of the current implementation is to check the previous value of Padding.All and if it differs update it instead of the individual properties.

A correct implementation would be to gracefully fall back to the "else" branch of setting individual properties when either context is empty or no "original" Padding is available. Maybe mix in some more flexibility in what properties the caller can specify in the dictionary (so he can either specify "All" or the individual properties but doesn't have to specify both, e.g. by using TryGetValue or ContainsKey checks to probe for "All" first)

(Will create a PR with what I think a better implementation can look like, details will be up for discussion then.)

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions