-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Removed all nullability overrides from TimePickerPresenter and DatePickerPresenter
#19241
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
Removed all nullability overrides from TimePickerPresenter and DatePickerPresenter
#19241
Conversation
|
Can you revert back to blocked namespaces? This change is something we want, but it needs to be done at least in a separated commit that can be added to |
|
There is a need to rewrite styling of these two controls. See #17515 |
|
You can test this PR using the following package version. |
db1f45d to
59525cc
Compare
|
I removed the namespace change, but do you know that you can do
I've struggled with these controls for this exact reason. Any improvements ought not to break existing themes, so these changes are still worthwhile. |
|
You can test this PR using the following package version. |
|
You are doing something new here: creating a struct for the template parts. Naming the controls exactly the same as the XAML template part name and then using I think this should be considered as part of the overall discussions below. All ideas essentially converged to code generators to solve this problem. |
MrJul
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! As mentioned by Max, this control really needs a rewrite, there are really too many template parts. This doesn't prevent fixing the nullability issues in the meantime :)
…ckerPresenter Fixed obscure cases where NullReferenceException could be thrown if a template hasn't been applied yet, or where it provides only some optional items Replaced repeated string literals with shared const values Relaxed template part requirements: RepeatButton to Button, Rectancle to Control
59525cc to
2a86a4c
Compare
|
You can test this PR using the following package version. |
MrJul
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
…ckerPresenter (#19241) Fixed obscure cases where NullReferenceException could be thrown if a template hasn't been applied yet, or where it provides only some optional items Replaced repeated string literals with shared const values Relaxed template part requirements: RepeatButton to Button, Rectancle to Control
Some mis-use of nullability checking was noted in #19210, but the problem goes much deeper. This PR moves template part fields into a struct, which is used by a nullable field on the presenter. This two-layer approach allows us to differentiate between optional and required template parts at compile time, while still allowing the template to be applied after the constructor has executed.
The only remaining uses of the
!character in the two affected files are boolean inversion operations.What is the updated/expected behavior with this PR?
There are no changes to behaviour, but:
NullReferenceExceptioncould be thrown if a template hasn't been applied yet, or where it provides only some optional items.nameof.RepeatButtontoButton,RectancletoControl.Breaking changes
None
Obsoletions / Deprecations
None
Fixed issues
Addresses one of the complaints made in #19210.