MudDatePicker: Fix FixedDay, FixMonth, FixYear for non-Gregorian calendar#11992
MudDatePicker: Fix FixedDay, FixMonth, FixYear for non-Gregorian calendar#11992ScarletKuro merged 4 commits intoMudBlazor:devfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request aims to add support for non-Gregorian calendars to the MudDatePicker by replacing direct DateTime property access with Culture.Calendar methods. The approach is correct, but I've found several issues in the implementation. Some DateTime constructors are still using the Gregorian calendar by default, and there are a couple of logic errors, including a critical copy-paste mistake. I've provided detailed comments and suggestions to address these problems and ensure the fix is complete and correct.
| FixMonth.HasValue || FixDay.HasValue) | ||
| _selectedDate = new DateTime(FixYear ?? Culture.Calendar.GetYear(_selectedDate.Value), | ||
| FixMonth ?? Culture.Calendar.GetMonth(_selectedDate.Value), | ||
| FixDay ?? Culture.Calendar.GetYear(_selectedDate.Value), |
There was a problem hiding this comment.
| if (a.HasValue) | ||
| { | ||
| a = new DateTime(a.Value.Year, a.Value.Month, 1); | ||
| a = new DateTime(Culture.Calendar.GetYear(a.Value), Culture.Calendar.GetMonth(a.Value), 1); |
There was a problem hiding this comment.
The DateTime constructor used here is missing the Calendar argument. Without it, the constructor will default to the Gregorian calendar, which will cause issues with non-Gregorian cultures. You should use the overload that accepts a Calendar object.
a = new DateTime(Culture.Calendar.GetYear(a.Value), Culture.Calendar.GetMonth(a.Value), 1, Culture.Calendar);| monthStartDate = new DateTime(correctYear, correctMonth, Culture.Calendar.GetDayOfMonth(monthStartDate), 0, 0, 0, 0, Culture.Calendar, DateTimeKind.Utc); | ||
|
|
||
| // Return the min supported datetime of the calendar when this is year 1 and first month! | ||
| if (_picker_month is { Year: 1, Month: 1 }) |
There was a problem hiding this comment.
This condition _picker_month is { Year: 1, Month: 1 } uses property patterns on DateTime, which relies on the Gregorian calendar. This will lead to incorrect behavior for non-Gregorian calendars. You've corrected this pattern in other places, but this one seems to have been missed. It should be updated to use Culture.Calendar to get the year and month.
if (_picker_month.HasValue && Culture.Calendar.GetYear(_picker_month.Value) == 1 && Culture.Calendar.GetMonth(_picker_month.Value) == 1)| var month = FixMonth ?? (year == Culture.Calendar.GetYear(today) ? Culture.Calendar.GetMonth(today) : 1); | ||
| var day = FixDay ?? 1; | ||
|
|
||
| if (DateTime.TryParseExact($"{year}-{month}-{day}", "yyyy-M-d", CultureInfo.InvariantCulture, DateTimeStyles.None, out var date)) |
There was a problem hiding this comment.
DateTime.TryParseExact is being called with CultureInfo.InvariantCulture, which forces the use of the Gregorian calendar for parsing. Since the year, month, and day variables are derived using Culture.Calendar, you should pass the Culture object to TryParseExact to ensure the correct calendar is used for parsing the date string.
if (DateTime.TryParseExact($"{year}-{month}-{day}", "yyyy-M-d", Culture, DateTimeStyles.None, out var date))| new DateTime( | ||
| Culture.Calendar.GetYear(_selectedDate.Value), | ||
| Culture.Calendar.GetMonth(_selectedDate.Value), | ||
| Culture.Calendar.GetDayOfMonth(_selectedDate.Value), | ||
| Culture.Calendar.GetHour(_selectedDate.Value), | ||
| Culture.Calendar.GetMinute(_selectedDate.Value), | ||
| Culture.Calendar.GetSecond(_selectedDate.Value), | ||
| (int)Culture.Calendar.GetMilliseconds(_selectedDate.Value), | ||
| _selectedDate.Value.Kind) |
There was a problem hiding this comment.
In OnYearClickedAsync, when creating the new DateTime, there are two issues:
- It's using
Culture.Calendar.GetYear(_selectedDate.Value)instead of theyearparameter passed to the method. The user's selected year should be used. - The
DateTimeconstructor is missing theCalendarobject, which will cause it to default to the Gregorian calendar.
Here's the corrected code.
new DateTime(
year,
Culture.Calendar.GetMonth(_selectedDate.Value),
Culture.Calendar.GetDayOfMonth(_selectedDate.Value),
Culture.Calendar.GetHour(_selectedDate.Value),
Culture.Calendar.GetMinute(_selectedDate.Value),
Culture.Calendar.GetSecond(_selectedDate.Value),
(int)Culture.Calendar.GetMilliseconds(_selectedDate.Value),
Culture.Calendar,
_selectedDate.Value.Kind)|
Hello, thanks for the PR.
|
|
Hi @ScarletKuro ! Thanks, I did edit the description and used |
|
That's a good question about how to set up the tests for this scenario. For specific technical questions and help with contributions, our Discord community is often the best place to get faster feedback from other developers. You're also welcome to use GitHub Discussions for broader topics. I'm an AI assistant — If I missed something or made a mistake, please let me know in a reply! |
|
Can't you just copy paste the reproduction from #6112 aka https://try.mudblazor.com/snippet/GYmnEbOVnSPfGgUO then set the culture via ? |
|
I found help on Discord. I checked and the introduced unit test does report an error in current dev version, and pass successfully in the version with my commits :) . Hoping the checks won't find any problem. |
There was a problem hiding this comment.
Pull Request Overview
This PR updates the DatePicker component to properly support non-Gregorian calendars (e.g., Persian calendar) by replacing direct property access to DateTime members (Year, Month, Day, etc.) with Calendar-specific methods. This ensures date operations respect the configured culture's calendar system.
- Replaced direct DateTime property access with Culture.Calendar.Get* methods throughout date construction
- Updated DateTime constructors to use Calendar-specific overloads
- Added a test case for Persian calendar with FixDay parameter
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/MudBlazor/Components/DatePicker/MudDatePicker.cs | Updated date construction in OnMonthSelectedAsync, OnYearClickedAsync, and SubmitAsync to use calendar-aware methods |
| src/MudBlazor/Components/DatePicker/MudBaseDatePicker.razor.cs | Applied calendar-aware date operations in GetMonthStart, IsMonthDisabled, OnPreviousMonthClick, and OnInitialized |
| src/MudBlazor/Components/DatePicker/MudBaseDatePicker.razor | Updated month/year checks in razor template to use calendar methods |
| src/MudBlazor.UnitTests/Components/DatePickerTests.cs | Added test case for Persian calendar with FixDay functionality |
| src/MudBlazor.UnitTests.Viewer/TestComponents/DatePicker/PersianDatePickerTest.razor | Added FixDay parameter support to Persian date picker test component |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| monthStartDate = new DateTime(correctYear, correctMonth, Culture.Calendar.GetDayOfMonth(monthStartDate), 0, 0, 0, 0, Culture.Calendar, DateTimeKind.Utc); | ||
|
|
||
| // Return the min supported datetime of the calendar when this is year 1 and first month! | ||
| if (_picker_month is { Year: 1, Month: 1 }) |
There was a problem hiding this comment.
This check uses direct DateTime properties (Year, Month) but should use calendar-aware methods for consistency with the rest of the PR. It should be: if (_picker_month.HasValue && Culture.Calendar.GetYear(_picker_month.Value) == 1 && Culture.Calendar.GetMonth(_picker_month.Value) == 1)
| if (_picker_month is { Year: 1, Month: 1 }) | |
| if (_picker_month.HasValue && Culture.Calendar.GetYear(_picker_month.Value) == 1 && Culture.Calendar.GetMonth(_picker_month.Value) == 1) |
There was a problem hiding this comment.
What does this change do, and it is required to be in the Culture's calendar?
There was a problem hiding this comment.
this particular change doesn't actually do anything that I can tell but there are some of the Culture.Calendar methods that do make translations based on culture. This suggestion is to make everything use the same structure.
|
|
||
| <MudDatePicker Label="Date Format" @bind-Date="Date" | ||
| HelperText="Persian" DateFormat="yyyy/MM/dd" | ||
| FixDay="FixDay" |
There was a problem hiding this comment.
The indentation uses tabs instead of spaces, which is inconsistent with the file's formatting. The line should use consistent whitespace (spaces) to match the surrounding code.
| FixDay="FixDay" | |
| FixDay="FixDay" |
| [Parameter] | ||
| public int? FixDay { get; set; } = null; |
There was a problem hiding this comment.
The indentation uses tabs instead of spaces, which is inconsistent with the file's formatting. These lines should use consistent whitespace (spaces) to match the surrounding code.
| [Parameter] | |
| public int? FixDay { get; set; } = null; | |
| [Parameter] | |
| public int? FixDay { get; set; } = null; |
| var date = new DateTime(1404, 1, 1, cal); | ||
|
|
||
| var comp = Context.RenderComponent<PersianDatePickerTest>(parameter => parameter.Add(p => p.Date, date).Add(p => p.FixDay, 1)); | ||
| var datePicker = comp.FindComponent<MudDatePicker>().Instance; |
There was a problem hiding this comment.
This assignment to datePicker is useless, since its value is never read.
| var datePicker = comp.FindComponent<MudDatePicker>().Instance; |
|
I'm a bit reluctant to do the suggested change for MudBaseDatePicker.razor.cs because I don't really understand what this test is for. Is it because MudDatePicker doesn't support dates with negative years? I'm not sure if it's a problem with certain calendars. If I get the go ahead from someone more familiar with the code I'll push it though. |
|
I don't know enough about how culture works and interacts to be a qualified reviewer. Hopefully @Anu6is will have time soon for a 2nd look. |
Checklist:
dev).Yes
Unsure, please help.
Not yet, also not sure how to do it, help would be welcome!
This PR fixes #6112