Skip to content

MudDatePicker: Fix FixedDay, FixMonth, FixYear for non-Gregorian calendar#11992

Merged
ScarletKuro merged 4 commits intoMudBlazor:devfrom
oxayotl:dev
Nov 1, 2025
Merged

MudDatePicker: Fix FixedDay, FixMonth, FixYear for non-Gregorian calendar#11992
ScarletKuro merged 4 commits intoMudBlazor:devfrom
oxayotl:dev

Conversation

@oxayotl
Copy link
Contributor

@oxayotl oxayotl commented Oct 23, 2025

Checklist:

  • The PR is submitted to the correct branch (dev).
    Yes
  • My code follows the style of this project.
    Unsure, please help.
  • I've added relevant tests or confirmed existing ones.
    Not yet, also not sure how to do it, help would be welcome!

This PR fixes #6112

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

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),
Copy link
Contributor

Choose a reason for hiding this comment

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

critical

There seems to be a copy-paste error here. You are using Culture.Calendar.GetYear(_selectedDate.Value) as the fallback for FixDay. It should be Culture.Calendar.GetDayOfMonth(_selectedDate.Value).

                    FixDay ?? Culture.Calendar.GetDayOfMonth(_selectedDate.Value),

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

high

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 })
Copy link
Contributor

Choose a reason for hiding this comment

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

high

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))
Copy link
Contributor

Choose a reason for hiding this comment

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

high

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))

Comment on lines 171 to 179
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)
Copy link
Contributor

Choose a reason for hiding this comment

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

high

In OnYearClickedAsync, when creating the new DateTime, there are two issues:

  1. It's using Culture.Calendar.GetYear(_selectedDate.Value) instead of the year parameter passed to the method. The user's selected year should be used.
  2. The DateTime constructor is missing the Calendar object, 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)

@ScarletKuro
Copy link
Member

Hello, thanks for the PR.

  1. Please, mention in description what issue it fixes (aka MudDatePicker returns incorrect date with Persian culture #6112)
  2. Please, add a bUnit test that verifies the problem from MudDatePicker returns incorrect date with Persian culture #6112 is gone
  3. Please, do dotnet format in the root of the project to get rid of build fail.

@mudbot mudbot bot added needs: changes A maintainer has asked for further modifications to be made to this pull request needs: tests A maintainer has explicitly asked for associated test cases to be added to this pull request labels Oct 23, 2025
@mudbot mudbot bot changed the title Fix MudDatePicker with FixedDay, FixMonth, FixYear for non-Gregorian calendar MudDatePicker: Fix FixedDay, FixMonth, FixYear for non-Gregorian calendar Oct 24, 2025
@oxayotl
Copy link
Contributor Author

oxayotl commented Oct 24, 2025

Hi @ScarletKuro ! Thanks, I did edit the description and used dotnet format, also added the corrections by gemini-code-assist, but I need help to write the bUnit test! When I tried to copy the Open_FixYear_Click2ndMonth_Click3_CheckDate test, I had an error because SimpleMudDatePicker doesn't have a Culture parameter, and when I tried to copy PersianCalendarTest_GoToDate I also had an error, this time because PersianDatePickerTest doesn't have a FixedYear parameter.

@mudbot
Copy link

mudbot bot commented Oct 24, 2025

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!

@ScarletKuro
Copy link
Member

Can't you just copy paste the reproduction from #6112 aka https://try.mudblazor.com/snippet/GYmnEbOVnSPfGgUO then set the culture via

[SetCulture("fa-IR")]
[SetUICulture("fa-IR")]

?

@oxayotl
Copy link
Contributor Author

oxayotl commented Oct 24, 2025

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.

@mudbot mudbot bot added awaiting triage Needs maintainer review or assistance and removed needs: tests A maintainer has explicitly asked for associated test cases to be added to this pull request needs: changes A maintainer has asked for further modifications to be made to this pull request labels Oct 24, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

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 })
Copy link

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

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

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)

Suggested change
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)

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What does this change do, and it is required to be in the Culture's calendar?

Copy link
Contributor

Choose a reason for hiding this comment

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

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"
Copy link

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

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

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.

Suggested change
FixDay="FixDay"
FixDay="FixDay"

Copilot uses AI. Check for mistakes.
Comment on lines 16 to 17
[Parameter]
public int? FixDay { get; set; } = null;
Copy link

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

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

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.

Suggested change
[Parameter]
public int? FixDay { get; set; } = null;
[Parameter]
public int? FixDay { get; set; } = null;

Copilot uses AI. Check for mistakes.
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;
Copy link

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

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

This assignment to datePicker is useless, since its value is never read.

Suggested change
var datePicker = comp.FindComponent<MudDatePicker>().Instance;

Copilot uses AI. Check for mistakes.
@mudbot mudbot bot removed the awaiting triage Needs maintainer review or assistance label Oct 30, 2025
@oxayotl
Copy link
Contributor Author

oxayotl commented Oct 31, 2025

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.

@mudbot mudbot bot added the awaiting triage Needs maintainer review or assistance label Oct 31, 2025
@versile2 versile2 removed their request for review October 31, 2025 12:53
@versile2
Copy link
Contributor

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.

@mudbot mudbot bot removed the awaiting triage Needs maintainer review or assistance label Nov 1, 2025
@ScarletKuro ScarletKuro merged commit 76ebc6f into MudBlazor:dev Nov 1, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Unexpected behavior or functionality not working as intended localization Translations, locale formats, RTL layout, calendars

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MudDatePicker returns incorrect date with Persian culture

6 participants