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

Adds support for Seconds to TimePicker #16079

Merged
merged 5 commits into from
Jul 4, 2024

Conversation

begleysm
Copy link
Contributor

@begleysm begleysm commented Jun 20, 2024

What does the pull request do?

This PR adds support for Seconds to the TimePicker Control (which currently only supports Hours and Minutes). It adds a UseSeconds Property which defaults to false. It addresses a need being tracked in Issue #12045.

What is the updated/expected behavior with this PR?

Usage of existing TimePicker Controls should see no change since the new UseSeconds Property is optional and defaults to false which results in the TimePicker behaving just as it did before.

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

I first went through and added the Seconds column by finding all of the references to Minutes and duplicating their behavior. I expanded the PickerHosts, TextBlocks, Dividers, etc and updated the ClockIdentifier column to be to the right of the new Seconds column. I then went through and found all of the references to ClockIdentifier and use it as a template on how to show/hide the new Seconds column based on my new UseSeconds property. Everywhere possible I tried to use naming consistent with the existing parameters. I also updated the DateTimePickerPage of the ControlCatalog to demonstrate the new UseSeconds feature. I added a updated one of the Unit Tests and added a Unit Test.

Checklist

Breaking changes

None

Obsoletions / Deprecations

None

Fixed issues

Fixes #12045

Example Screenshot

image

@avaloniaui-bot
Copy link

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

@cla-avalonia
Copy link
Collaborator

cla-avalonia commented Jun 20, 2024

  • All contributors have signed the CLA.

@begleysm
Copy link
Contributor Author

@cla-avalonia agree

1 similar comment
@begleysm
Copy link
Contributor Author

@cla-avalonia agree

Copy link
Member

@jmacato jmacato left a comment

Choose a reason for hiding this comment

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

Overall looks good to me! Thank you for your contribution!

cc @maxkatz6 @grokys

@grokys grokys added this pull request to the merge queue Jul 4, 2024
Merged via the queue into AvaloniaUI:master with commit ae01776 Jul 4, 2024
11 checks passed
begleysm added a commit to begleysm/avalonia-docs that referenced this pull request Jul 9, 2024
begleysm added a commit to begleysm/avalonia-docs that referenced this pull request Jul 9, 2024
@@ -230,11 +308,12 @@ private void SetSelectedTimeText()
{
var hr = newTime.Hours;
hr = hr > 12 ? hr - 12 : hr == 0 ? 12 : hr;
newTime = new TimeSpan(hr, newTime.Minutes, 0);
newTime = new TimeSpan(hr, newTime.Minutes, newTime.Seconds);
Copy link
Contributor

@VisualMelon VisualMelon Aug 21, 2024

Choose a reason for hiding this comment

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

The current behaviour with a time that includes seconds when UseSeconds = false is to keep the seconds, which may be misleading, particular as the time span defaults to the current time, so a user can select a time, but be off by 0-59 seconds without knowing it. (This is a breaking change)

I think it would be better to exclude the seconds when UseSeconds = false; I'm not sure what would be the right behaviour when setting UseSeconds = true having already been false, but my instinct is that it should be cleared to zero.

(I'm happy to work on this feature if useful; may suffice to update OnConfirmed in TimePickerPresenter; this comment is not attached to a relevant piece of code)

Copy link
Contributor Author

@begleysm begleysm Aug 28, 2024

Choose a reason for hiding this comment

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

Good catch.

Interestingly (in the absence of it being set manually with UseSeconds == true), newTime.Seconds appears to be set to be equal to the current time when the Control is first clicked on (not when it is first loaded or when the check button is clicked to set the time) and then stays that way forever (doesn't update even when the Control is clicked again or another control is clicked and then this control is clicked again). I'm not, currently, entirely sure why this is and it also isn't super relevant to this particular issue.

Changing the modified line to

newTime = (UseSeconds) ? 
                        new TimeSpan(hr, newTime.Minutes, newTime.Seconds) :
                        new TimeSpan(hr, newTime.Minutes, 0);

causes 0 to be used for seconds when UseSeconds == false & the selected seconds to be used for seconds when UseSeconds == true. But it doesn't, alone, address the case where UseSeconds switches from false to true.

Regarding when UseSeconds == false changes to UseSeconds == true. That is also a good edge case you've identified. We can detect this case in the UseSeconds portion of the OnPropertyChanged override method.
We can detect that UseSecond went from false to true and pass an argument to SetSelectedTimeText() to indicate that 0 should be used for seconds.

It could look something like this which should result in

  • using a 0 for seconds if UseSeconds == false
  • using a 0 for seconds if UseSeconds == true but we came this method because UseSeconds went from false to true
  • otherwise (if UseSeconds == true but was not just changed from false) using the SelectedTime.Seconds for seconds
    image

Copy link
Contributor

Choose a reason for hiding this comment

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

Having messed around a bit, I'm still not sure what the correct behaviour should be. I'm happy enough with the behaviour of us setting the seconds to zero when completing an interaction; trivial change is in VisualMelon@7e4df0f along with trivial example. It doesn't exercise the scenario where you have another control/piece of code updating the time while UseSeconds is false, and then setting it to true, but again, not sure what the behaviour should be, so may well warrant something more interesting

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I've not been more active on this; we should probably create an issue for it, as it is a serious breaking change and don't want it to get lost: I'll try to remember to do so on the weekend if no-one beats me to it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TimePicker No Select Second
7 participants