Skip to content

Fixed DateTimePicker, and made names more intuitive#25

Merged
ghost1372 merged 9 commits intoghost1372:mainfrom
AlexanderBlackman:main
Jan 18, 2025
Merged

Fixed DateTimePicker, and made names more intuitive#25
ghost1372 merged 9 commits intoghost1372:mainfrom
AlexanderBlackman:main

Conversation

@AlexanderBlackman
Copy link
Copy Markdown
Contributor

Breaking

Renamed SelectedDate to SelectedDateTime and SelectedDateTime to SelectedDateTimeString. Calling a string a datetime would be very confusing.

Non-Breaking

  • Made sure the DateTimePicker's SelectedDateTime DateTime is updated if the underlying Calendar's SelectedDateTime updates.
  • Set minimum width of the Calendar to 300, as otherwise Friday and Saturday would be cut off.
  • Changed Am/Pm to am/pm as it should be the same case.
  • Added a button in the sample app to show the unmodified emitted SelectedDateTime in a textblock.

@ghost1372
Copy link
Copy Markdown
Owner

ghost1372 commented Jan 17, 2025

Hi @AlexanderBlackman Tnx for your Great PR.
I have no problem with breaking changes because we are currently in preview.
Changing names is also great.
I saw some things that needed to be fixed.
First, please revert the Git Ignore items to their previous state. i think This action was probably unintentional.

9ed50c0

Regarding the following code, it is likely to end up in an infinite loop.

private void CalendarWithClock_SelectedTimeChanged(object sender, DateTimeOffset e)
{
    if (calendarWithClock != null)
    {
        SelectedDateTime = e;
        SelectedTime = e.TimeOfDay;
    }
    SelectedTimeChanged?.Invoke(this, e);
}

Assigning a new value to SelectedTime triggers OnSelectedTimeChanged, which calls UpdateSelectedTime(). This method updates calendarWithClock.SelectedTime.
if SelectedDateTime or SelectedTime is changed within CalendarWithClock_SelectedTimeChanged, the associated property-changed callback will run. This could cause the CalendarWithClock properties (SelectedDateTime or SelectedTime) to be updated again, potentially resulting in a loop.

do we need to Update SelectedDateTime/SelectedTime in SelectedTimeChanged? I haven't seen any problems with the tests before.
If there are circumstances where this is necessary, please Ensure updates only occur when necessary to avoid unnecessary callbacks.
something like:

if (SelectedDateTime != e)
    SelectedDateTime = e;

if (SelectedTime != e.TimeOfDay)
    SelectedTime = e.TimeOfDay;

@ghost1372 ghost1372 added the enhancement New feature or request label Jan 17, 2025
@AlexanderBlackman
Copy link
Copy Markdown
Contributor Author

AlexanderBlackman commented Jan 18, 2025

Thank you for your suggestions, I only checked for loops via the debugger, as I didn't want to add a new test project.

I assumed this wouldn't a infinite loop, as the event is triggered by CalendarWithClock's SelectedDateTime changing, and not DateTimePicker. But you are right, it might cause problems, so I've copied your try-finally isUpdating = false wrapping.

When stepping through with debugger, MicaSystemBackdrop's OnDefaultSystemBackdropConfigurationChanged is called many many times, but I'm assuming that's work-in-progress, so I stayed clear.

Sorry for the gitignore changes, I think it happened automatically when I just tried to change the DevWinUI.Controls project on its own as at the time I hadn't installed VS Preview and couldn't open slnx. I'll be more careful next time, this is the first time I've contributed on Github for years.

@ghost1372
Copy link
Copy Markdown
Owner

LGTM🚀
Thank you for addressing the feedback.
I would be happy if you could work on MicaSystemBackdrop's issue.
I'm currently focused on controls and trying to bridge the gap between WPF and WinUI.
I'm currently working on a carousel control and I'm planning to create something similar to the Microsoft Store Carousel.
That's why I can't work on fixing these problems right now because it will probably take more time.

@ghost1372 ghost1372 merged commit 6875197 into ghost1372:main Jan 18, 2025
@AlexanderBlackman
Copy link
Copy Markdown
Contributor Author

Good suggestion, but I'm not too familiar with backdrops.
I'm currently experimenting with making the DateTimePicker being settable and bindable.
Originally the clock's default time overwrote it, but I seem to have fixed it.

Here's me using DateTimePicker to edit an existing appointment in a scheduler.
Editing an existing timeblock
Editing an existing timeblock with Datetimepicker
The colours are deliberately ugly, sorry.

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

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants