Fix LocaleShortFormat to show date/time as per current locale#79
Fix LocaleShortFormat to show date/time as per current locale#79Jojo-Schmitz wants to merge 2 commits into
LocaleShortFormat to show date/time as per current locale#79Conversation
LocaleShortFormat to show date/time as per current localleLocaleShortFormat to show date/time as per current locale
d800a8b to
0e25be9
Compare
|
Warning Review limit reached
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 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe pull request modifies 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
✅ Action performedReview finished.
|
|
At least with Windows (11) this is the case, not sure about macOS and Linux? |
|
This looks good to me as an easy fix, thanks! I would perhaps move the line to (And you may or may not need to 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 |
0e25be9 to
55b2a7c
Compare
|
Like this? |
55b2a7c to
ff3eba9
Compare
There was a problem hiding this comment.
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
📒 Files selected for processing (1)
framework/languages/internal/languagesservice.cpp
ff3eba9 to
72b23d8
Compare
reg.: 'args': unreferenced parameter (C4100)
|
See also musescore/MuseScore#33780 |
Prerequisite to resolve musescore/MuseScore#15437
Without this using
muse::DateFormat::LocaleShortFormatresults 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 themuse::DateFormat::ISODateformat YYYY-MM-DD in headerfooterlayout.cpp