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.

Sign up for GitHub

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

Jump to bottom

Set TimePicker.Time seconds to zero when UseSeconds is False #17251

Merged
merged 3 commits into from
Oct 15, 2024
Merged

Set TimePicker.Time seconds to zero when UseSeconds is False #17251

merged 3 commits into from
Oct 15, 2024

Conversation

VisualMelon
Copy link
Contributor

What does the pull request do?

Addresses the most significant issue (breaking change) discussed in #17024

What is the current behavior?

The seconds component of the Time property is always set to an uncontrolled value when UseSeconds is false.

What is the updated/expected behavior with this PR?

When UseSeconds is false, the Time property has its seconds component set to zero

How was the solution implemented (if it's not obvious)?

N/A

Checklist

Breaking changes

N/A

Obsoletions / Deprecations

N/A

Fixed issues

Fixes part of #17024

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.3.999-cibuild0052604-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@maxkatz6 maxkatz6 added bug backport-candidate-11.2.x Consider this PR for backporting to 11.2 branch labels Oct 11, 2024
@MrJul
Copy link
Member

MrJul commented Oct 12, 2024

Thank you, the change looks reasonable to me.
Can you please add a short unit test for this case?

@VisualMelon
Copy link
Contributor Author

I had a go at that, and my AV got upset, but I'll try again. May take a bit of time for me to work out how to test it sensibly.

@VisualMelon
Copy link
Contributor Author

Pushed a couple of simple tests: not sure if they are up to muster; obviously I'm happy to change anything with a bit of direction. Should the TimePickerPresenter have its own tests file?

I had a lot of trouble with the templating the TimePickerPresenter:

  • Added a couple of TemplatePart declarations to it that seemed to be missing (not sure if they're inherited from somewhere else?)
  • Changed the order that the ItemFormat and SelectedValue are assigned in InitPicker, as SelectedValue was crashing beforehand
  • I seemed to have to call pickerPresenter.ApplyTemplate(); inside the TimePicker template, but it still seemed to crash inside ApplyTemplate when I didn't (before the above fix was put in), so I'm just confused what's going on there

Note that the test explicitly invoke the AcceptBtn: the seconds property will be (59/60 times) non-zero before this is pressed. See discussion in #17024; I think this behaviour is the same as 11.1 anyway, so not a breaking change.

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.3.999-cibuild0052617-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@VisualMelon
Copy link
Contributor Author

Looks like I've broken something, but no idea why (Appium makes my AV unhappy to didn't run those locally). Will take another look in a few hours/tomorrow.

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.3.999-cibuild0052619-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@VisualMelon
Copy link
Contributor Author

Rebased against master and tests stopped failing; not sure those are related.

maxkatz6
maxkatz6 reviewed Oct 12, 2024
View reviewed changes
src/Avalonia.Controls/DateTimePickers/TimePickerPresenter.cs Outdated
Comment on lines 31 to 33
[TemplatePart("PART_FirstSpacer", typeof(Rectangle), IsRequired = true)]
[TemplatePart("PART_SecondSpacer", typeof(Rectangle), IsRequired = true)]
[TemplatePart("PART_ThirdSpacer", typeof(Rectangle), IsRequired = true)]
Copy link
Member

@maxkatz6 maxkatz6 Oct 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Just a sanity check, are these really required? I.e. will control work fine, if these spacers were missing, without any C# code change. For example, a third-party theme.

Copy link
Contributor Author

@VisualMelon VisualMelon Oct 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

ApplyTemplate crashed when I didn't include them in the test template, so I added them here as they seemed to be missing.

It did seem odd that they are required: I considered making them not required, but decided the test code was messy enough without my trying to break anything else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like FirstSpacer doesn't need to exist (isn't used at all, which is a bit odd), but SecondSpacer is assumed in the code. I'll remove FirstSpacer or make any other suggested changes tomorrow.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, they're unfortunately needed currently. We should refactor these templates someday: I'm sure a lot of parts could be made optional and/or simple styles added to the themes instead. Anyways, such a refactoring isn't the point of this PR :)

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.3.999-cibuild0052627-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@MrJul
Copy link
Member

MrJul commented Oct 14, 2024

Rebased against master and tests stopped failing; not sure those are related.

They aren't related, these specific integration tests are unfortunately flaky, you can ignore them for now. I'm hoping #17259 will help them pass...

MrJul
MrJul approved these changes Oct 14, 2024
View reviewed changes
Copy link
Member

@MrJul MrJul left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.3.999-cibuild0052649-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@MrJul MrJul merged commit 3e3d434 into AvaloniaUI:master Oct 15, 2024
8 of 10 checks passed
@VisualMelon VisualMelon deleted the useseconds branch October 15, 2024 08:28
maxkatz6 pushed a commit that referenced this pull request Oct 27, 2024
@VisualMelon @MrJul @maxkatz6
Set TimePicker.Time seconds to zero when UseSeconds is False (#17251)
7da26e2 
* Set TimePickerPresenter.Time seconds component to zero when UseSeconds is false

* Remove unneeded PART_FirstSpace annotation from TimePickerPresenter

---------

Co-authored-by: Julien Lebosquain <julien@lebosquain.net>
@maxkatz6 maxkatz6 added backported-11.2.x and removed backport-candidate-11.2.x Consider this PR for backporting to 11.2 branch labels Oct 27, 2024
@renovate renovate bot mentioned this pull request Dec 21, 2024
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxkatz6 maxkatz6 maxkatz6 left review comments

@MrJul MrJul MrJul approved these changes

Assignees
No one assigned
Labels
backported-11.2.x bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@VisualMelon @avaloniaui-bot @MrJul @maxkatz6