Skip to content

Fix LocaleShortFormat to show date/time as per current locale#79

Closed
Jojo-Schmitz wants to merge 2 commits into
musescore:mainfrom
Jojo-Schmitz:locale-date-time
Closed

Fix LocaleShortFormat to show date/time as per current locale#79
Jojo-Schmitz wants to merge 2 commits into
musescore:mainfrom
Jojo-Schmitz:locale-date-time

Conversation

@Jojo-Schmitz

@Jojo-Schmitz Jojo-Schmitz commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Prerequisite to resolve musescore/MuseScore#15437

Without this using muse::DateFormat::LocaleShortFormat results in the "C" locale (AKA "en_US") to be used, with the very confusing MM/DD/YYYY format, that in turn prevents MuseScore to use anything sensible but the muse::DateFormat::ISODate format YYYY-MM-DD in headerfooterlayout.cpp

@Jojo-Schmitz Jojo-Schmitz changed the title Fix LocaleShortFormat to show date/time as per current localle Fix LocaleShortFormat to show date/time as per current locale Jun 5, 2026
@coderabbitai

coderabbitai Bot commented Jun 5, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@Jojo-Schmitz, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 40 minutes and 2 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more credits in the billing tab to continue.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 683dfc68-41ef-4ec8-ad4c-d575e3c24661

📥 Commits

Reviewing files that changed from the base of the PR and between 55b2a7c and e7cb62e.

📒 Files selected for processing (2)
  • framework/languages/internal/languagesservice.cpp
  • framework/rcontrol/mcp/mcpcontroller.cpp
📝 Walkthrough

Walkthrough

The pull request modifies LanguagesService::setCurrentLanguage to call std::setlocale(LC_TIME, ...) with the selected language code. This updates the process-wide C locale for time-related string formatting operations whenever the application language is switched. The change is a single-line addition that applies the language code to the LC_TIME locale category.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description clearly explains the issue and links to the prerequisite issue, but doesn't follow the repository template structure with required checklist items. Add the required checklist items from the template (CLA signature, code guidelines compliance, testing verification, etc.) to meet repository standards.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: fixing LocaleShortFormat to respect the current locale instead of defaulting to the C locale.
Linked Issues check ✅ Passed The change successfully addresses the core requirement from issue #15437 by calling std::setlocale(LC_TIME, ...) to set the C locale to match the selected language, enabling LocaleShortFormat to use locale-appropriate formatting.
Out of Scope Changes check ✅ Passed The modification is minimal and focused: only one line changed in LanguagesService::setCurrentLanguage to set LC_TIME locale, directly addressing the linked issue without introducing unrelated changes.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai

coderabbitai Bot commented Jun 5, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@Jojo-Schmitz

Jojo-Schmitz commented Jun 5, 2026

Copy link
Copy Markdown
Contributor Author

At least with Windows (11) this is the case, not sure about macOS and Linux?

@ajuncosa

Copy link
Copy Markdown
Contributor

This looks good to me as an easy fix, thanks! I would perhaps move the line to LanguagesService::setCurrentLanguage(), though, anywhere below line 213. This is to set it just once instead of every time the configuration getter is called. Then, it would be like:

std::setlocale(LC_TIME, locale.name().toStdString().c_str());

(And you may or may not need to #include <clocale> so that the LC_TIME macro will be found.)

In the future we may want to actually wrap and use a Qt implementation as suggested in musescore/MuseScore#15437 (comment), as that would probably be the cleaner solution.

PS: once this is merged, I can update the other half of the musescore/MuseScore#15437 fix in the MuseScore repo, as it's WIP already

@Jojo-Schmitz

Copy link
Copy Markdown
Contributor Author

Like this?

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@framework/languages/internal/languagesservice.cpp`:
- Line 214: The use of std::setlocale and LC_TIME in the languages service
requires the C locale header; add the missing include for <clocale> near the
other includes so the identifiers std::setlocale and LC_TIME are defined and the
file compiles (update the includes section where other headers are declared).
- Line 214: Capture and check the return value of std::setlocale when calling
std::setlocale(LC_TIME, lang.code.toStdString().c_str()); and if it returns
nullptr, emit a warning including the requested locale string (lang.code) and
the fact that setlocale failed; use the project's logging facility (e.g.,
qWarning() or the existing logger used elsewhere in languagesservice.cpp) so
failures to set LC_TIME are visible for debugging.
- Line 214: The call to std::setlocale(LC_TIME, lang.code.toStdString().c_str())
can fail silently and uses non‑canonical Qt locale strings; change it to use the
Qt canonical name (locale.name()) and check the return value of std::setlocale.
If std::setlocale returns nullptr, log an error via the existing logging
facility (same context where std::setlocale is called) and avoid assuming
LC_TIME was changed; otherwise proceed. Update the call sites that reference
lang.code, replace with locale.name(), perform the nullptr check, and emit a
clear log message on failure.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 9c335030-8703-48de-9742-73124f922950

📥 Commits

Reviewing files that changed from the base of the PR and between 5e1045b and 55b2a7c.

📒 Files selected for processing (1)
  • framework/languages/internal/languagesservice.cpp

Comment thread framework/languages/internal/languagesservice.cpp Outdated
reg.: 'args': unreferenced parameter (C4100)
@Jojo-Schmitz

Jojo-Schmitz commented Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

See also musescore/MuseScore#33780

@ajuncosa ajuncosa self-requested a review June 12, 2026 06:00
@ajuncosa

Copy link
Copy Markdown
Contributor

Thanks, but we will be closing this for now as the issue requires more careful consideration, so we will work on a design that properly solves the problem.

@ajuncosa ajuncosa closed this Jun 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The date macros ($d, $D, $M) for header/footer show the dates in ISO format (YYYYY-MM-DD) rather than as per the current locale

2 participants