Fix the date macros to display dates in the locale's format#33780
Fix the date macros to display dates in the locale's format#33780Jojo-Schmitz wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR changes HeaderFooterLayout::replaceTextMacros to format timestamp macros using muse::DateFormat::LocaleShortFormat. The macros $d (current date), $D (creation date), $m (last modification time), and $M (last modification date) now use LocaleShortFormat; $m/$M "newly created" branches use muse::Time::currentTime()/muse::Date::currentDate() formatted with LocaleShortFormat. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsLinked repositories: Your configuration references 1 linked repositories, but your current plan allows 0. Analyzed ``, skipped 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/engraving/rendering/score/headerfooterlayout.cpp (2)
323-330:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPlaceholder string format inconsistency with locale format.
For newly created files, the placeholder
"HH:mm:ss"(line 326) uses an ISO-like format, but after saving, the macro displays time in locale format (line 328). This creates a visual inconsistency where the format changes after the first save.Consider using a locale-appropriate placeholder or a generic string like
"--:--:--"to avoid implying a specific format.🤖 Prompt for 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. In `@src/engraving/rendering/score/headerfooterlayout.cpp` around lines 323 - 330, The placeholder in the case 'm' branch uses a hardcoded "HH:mm:ss" for newly created files while saved files use fileInfo->lastModified().time().toString(muse::DateFormat::LocaleShortFormat), causing a format mismatch; change the newFragments.back().text assignment in the case 'm' block (where fileInfo->isNewlyCreated() is checked) to use a locale-appropriate placeholder (e.g. produce a localized empty-time representation by formatting a sentinel time with DateFormat::LocaleShortFormat or use a neutral placeholder like "--:--:--") so the displayed format for newly created files matches the locale-based format used after saving.
332-339:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPlaceholder string format inconsistency with locale format.
For newly created files, the placeholder
"YYYY-MM-DD"(line 335) uses an ISO format, but after saving, the macro displays the date in locale format (line 337). This creates a visual inconsistency where the format changes after the first save.Consider using a locale-appropriate placeholder or a generic string like
"-----..--"to avoid implying a specific format.🤖 Prompt for 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. In `@src/engraving/rendering/score/headerfooterlayout.cpp` around lines 332 - 339, In case 'M' inside headerfooterlayout.cpp where newFragments.back().text is set, avoid hardcoding the ISO placeholder "YYYY-MM-DD" when fileInfo->isNewlyCreated(); instead append a locale-consistent placeholder (or the same formatted string used for saved files) so the displayed format does not change after save — e.g., replace the literal with a locale-aware placeholder generated the same way as fileInfo->lastModified().date().toString(muse::DateFormat::LocaleShortFormat) or use a neutral placeholder like "-----..--" so the UI remains consistent; update the code around fileInfo->isNewlyCreated() and the case 'M' block accordingly.
🤖 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.
Outside diff comments:
In `@src/engraving/rendering/score/headerfooterlayout.cpp`:
- Around line 323-330: The placeholder in the case 'm' branch uses a hardcoded
"HH:mm:ss" for newly created files while saved files use
fileInfo->lastModified().time().toString(muse::DateFormat::LocaleShortFormat),
causing a format mismatch; change the newFragments.back().text assignment in the
case 'm' block (where fileInfo->isNewlyCreated() is checked) to use a
locale-appropriate placeholder (e.g. produce a localized empty-time
representation by formatting a sentinel time with DateFormat::LocaleShortFormat
or use a neutral placeholder like "--:--:--") so the displayed format for newly
created files matches the locale-based format used after saving.
- Around line 332-339: In case 'M' inside headerfooterlayout.cpp where
newFragments.back().text is set, avoid hardcoding the ISO placeholder
"YYYY-MM-DD" when fileInfo->isNewlyCreated(); instead append a locale-consistent
placeholder (or the same formatted string used for saved files) so the displayed
format does not change after save — e.g., replace the literal with a
locale-aware placeholder generated the same way as
fileInfo->lastModified().date().toString(muse::DateFormat::LocaleShortFormat) or
use a neutral placeholder like "-----..--" so the UI remains consistent; update
the code around fileInfo->isNewlyCreated() and the case 'M' block accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7382387b-a243-43b3-91fd-32d610fba3d1
📒 Files selected for processing (1)
src/engraving/rendering/score/headerfooterlayout.cpp
Jojo-Schmitz
force-pushed
the
date-macros
branch
from
157b144 to
a5e7641
Compare
|
Should the date/time format be a(n advanced) preferecnce setting (defaulting to |
Jojo-Schmitz
force-pushed
the
date-macros
branch
3 times, most recently
from
24b8618 to
5fc4da5
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/engraving/rendering/score/headerfooterlayout.cpp (1)
213-246:⚠️ Potential issue | 🟡 MinorAlign header/footer macro formatting + “keep in sync” references for date macros
HeaderFooterLayout::replaceTextMacrosformats$d/$Dand$m/$Musingmuse::DateFormat::LocaleShortFormat.EditStyle::setHeaderFooterMacroInfoTextdocuments$d/$D/$m/$Mbut doesn’t specify a particular date/time format; no otherLocaleShortFormathandling for these macros was found, so preview/render inconsistencies from formatting are unlikely.- The tooltip comment still says “keep in sync with implementation in
Page::replaceTextMacros (page.cpp)”, but the actual macro implementation is inHeaderFooterLayout::replaceTextMacros—update the comment to avoid future drift.🤖 Prompt for 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. In `@src/engraving/rendering/score/headerfooterlayout.cpp` around lines 213 - 246, The header/footer macro handling and its doc comment are out of sync: update the tooltip/comment that currently references Page::replaceTextMacros to point to HeaderFooterLayout::replaceTextMacros, and document that date/time macros $d/$D and $m/$M are formatted using muse::DateFormat::LocaleShortFormat so EditStyle::setHeaderFooterMacroInfoText (or its tooltip text) matches the actual formatting; modify the comment block above HeaderFooterLayout::replaceTextMacros to say “keep in sync with toolTipHeaderFooter in EditStyle::EditStyle()/setHeaderFooterMacroInfoText” (or similar) and add a short note that $d/$D and $m/$M use LocaleShortFormat to avoid future drift between implementation and documentation.
🤖 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.
Outside diff comments:
In `@src/engraving/rendering/score/headerfooterlayout.cpp`:
- Around line 213-246: The header/footer macro handling and its doc comment are
out of sync: update the tooltip/comment that currently references
Page::replaceTextMacros to point to HeaderFooterLayout::replaceTextMacros, and
document that date/time macros $d/$D and $m/$M are formatted using
muse::DateFormat::LocaleShortFormat so EditStyle::setHeaderFooterMacroInfoText
(or its tooltip text) matches the actual formatting; modify the comment block
above HeaderFooterLayout::replaceTextMacros to say “keep in sync with
toolTipHeaderFooter in EditStyle::EditStyle()/setHeaderFooterMacroInfoText” (or
similar) and add a short note that $d/$D and $m/$M use LocaleShortFormat to
avoid future drift between implementation and documentation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f986b142-61c6-40e3-b5bd-fa2d2dd22a3a
📒 Files selected for processing (1)
src/engraving/rendering/score/headerfooterlayout.cpp
| } | ||
|
|
||
| for (size_t i = 0, n = s.size(); i < n; ++i) { | ||
| const auto dateFormat = muse::DateFormat::LocaleShortFormat; |
There was a problem hiding this comment.
@coderabbitai: Is there a way to find out at compile time whether a build is for the backend on musescore.com?
I'd want MuseScore Studio to use the current locale`s format, but would want to keep ISO format on musescore.com rather than having it show the (IMHO sick, for sure confusing) US format (MM/DD/YYYY)
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for backend-related preprocessor defines or CMake options
rg -rn --type=cpp --type=cmake -i 'backend|musescore\.com|server.build|cloud.build|MUSECORE_COM' -C2 | head -80
# Also check top-level CMakeLists for any backend/server options
grep -n -i 'backend\|server\|cloud\|MUSE_APP' CMakeLists.txt 2>/dev/null | head -40
# Check for any build type defines
rg -rn 'MUSE_APP\|BUILD_TYPE\|APP_TYPE\|BACKEND' --include='*.h' --include='*.cmake' --include='CMakeLists.txt' -C2 | head -80Length of output: 8984
🏁 Script executed:
#!/bin/bash
# Look at MUSE_APP_BUILD_MODE definition and its possible values
sed -n '70,120p' CMakeLists.txt
# Search for all usages of MUSE_APP_BUILD_MODE
rg -rn 'MUSE_APP_BUILD_MODE' -C2 | head -100
# Also look for any "backend" or "server" string in CMakeLists
rg -rn -i 'backend\|BUILD_FOR_SERVER\|HEADLESS\|NO_GUI' CMakeLists.txt | head -40Length of output: 8279
🏁 Script executed:
#!/bin/bash
# Look at the full backend build script
cat buildscripts/ci/backend/build.sh
# Look at SetupConfigure.cmake to see how build config becomes C++ defines
cat SetupConfigure.cmake 2>/dev/null || fd -i 'SetupConfigure.cmake' --exec cat {}
# Check if BUILD_CONFIGURATION or similar ends up as a C++ preprocessor define
rg -rn 'add_compile_definitions\|add_definitions\|target_compile_definitions' --type=cmake | grep -i 'BUILD_CONFIGURATION\|BUILD_MODE\|APP_WEB\|BACKEND' | head -30Length of output: 16155
🏁 Script executed:
#!/bin/bash
# Check app_config.h.in for compile-time defines
fd -i 'app_config.h.in' --exec cat {}
# Also check if backend CI script passes any special CMake flags
grep -n 'MUSESCORE_BUILD_CONFIGURATION\|BUILD_CONFIGURATION\|BACKEND\|configure' buildscripts/ci/backend/build.shLength of output: 3065
@Jojo-Schmitz — there is no existing compile-time flag that distinguishes a backend/musescore.com build from a regular Studio build. The buildscripts/ci/backend/build.sh script does not set a special MUSESCORE_BUILD_CONFIGURATION; it falls through to the default "app" configuration, so backend builds are currently indistinguishable from desktop builds at compile time.
However, the codebase has a well-established pattern for adding exactly this kind of flag:
-
SetupConfigure.cmake— add a newMUSESCORE_BUILD_CONFIGURATIONvalue (e.g.app-backend) or a standalone CMake option (e.g.MUE_BUILD_FOR_BACKEND):if(BUILD_CONFIGURATION STREQUAL "APP-BACKEND") set(MUE_CONFIGURATION_IS_BACKEND ON) endif()
-
src/app/app_config.h.in— expose it as a C++ preprocessor define (alongside the existingMUE_CONFIGURATION_IS_APPWEBetc.):`#cmakedefine` MUE_CONFIGURATION_IS_BACKEND 1 -
buildscripts/ci/backend/build.sh— pass the flag when building for musescore.com:MUSESCORE_BUILD_CONFIGURATION=app-backend \ bash ./ninja_build.sh -t appimage
-
headerfooterlayout.cpp— then conditionally select the format:`#ifdef` MUE_CONFIGURATION_IS_BACKEND const auto dateFormat = muse::DateFormat::ISODate; `#else` const auto dateFormat = muse::DateFormat::LocaleShortFormat; `#endif`
This would need to land as a coordinated change across those files (plus the muse_framework PR #79 you already have). Since this touches the build infrastructure, it might be worth coordinating with the MuseScore infrastructure/build team to confirm the backend CI actually uses that script and what the right hook point is.
🧠 Learnings used
Learnt from: igorkorsukov
Repo: musescore/MuseScore PR: 32874
File: src/appshell/widgets/splashscreen/loadingscreenview.cpp:32-32
Timestamp: 2026-04-01T07:34:44.168Z
Learning: In the MuseScore C++ codebase, `IApplication::fullVersion()` (non-static virtual instance method in `src/framework/global/iapplication.h`) must only be called after application/services initialization. Do not use it in code paths that run before services are ready (e.g., splash/loading screens). If you need the app version via service-free static access in early UI/boot code, call `muse::BaseApplication::appFullVersion()` instead (static method declared in `src/framework/global/internal/baseapplication.h`), which reads compile-time constants.
Learnt from: igorkorsukov
Repo: musescore/MuseScore PR: 32950
File: src/framework/testing/environment.cpp:65-65
Timestamp: 2026-04-09T08:32:46.263Z
Learning: When using MuseScore’s IoC container (e.g., `muse::modularity::globalIoc()->registerExport<T>(name, impl)`), treat the `name` argument as the *registering module* identifier used for informational/tracing purposes. It does not have to match the key that consumers use to resolve the service later. In reviews, do not flag cases where the registration key (module name) differs from the resolution/lookup key as a bug, as this mismatch can be intentional (e.g., `IApplication` registered under `"testing"` in `src/framework/testing/environment.cpp`).
Learnt from: igorkorsukov
Repo: musescore/MuseScore PR: 33000
File: src/framework/audio/main/audiomodule.cpp:154-154
Timestamp: 2026-04-14T09:30:22.267Z
Learning: When registering exports via MuseScore’s IoC container (`muse::modularity`), it is acceptable to pass a raw pointer created with `new` to `ioc()->registerExport<T>(name, ...)`. The container will wrap it internally into a `std::shared_ptr`, so calls like `ioc()->registerExport<T>(name, new Impl(...))` are not a bug and should not be flagged for ownership/style issues. Prefer clarity, but do not treat raw `new` in `registerExport` arguments as an error; `ioc()->registerExport<T>(name, std::make_shared<Impl>(...))` is also valid.
Learnt from: Jojo-Schmitz
Repo: musescore/MuseScore PR: 33028
File: src/project/internal/opensaveprojectscenario.cpp:477-477
Timestamp: 2026-04-16T10:04:41.682Z
Learning: In the MuseScore C++ codebase, `configuration()->supportForumUrl()` intentionally returns a MuseScore Studio/app support URL on `musescore.org` (e.g., `/forum/...`, language-specific). When adding user-facing help text or help-button links for cloud-related errors (issues involving saving/uploading via `musescore.com`), ensure the help URL points to `musescore.com` (not `musescore.org`). Use `musescore.org` for Studio/app support problems and `musescore.com` for cloud-saving/upload issues.
Learnt from: saintmatthieu
Repo: musescore/MuseScore PR: 33036
File: src/framework/uicomponents/qml/Muse/UiComponents/tablesortfilterproxymodel.cpp:101-105
Timestamp: 2026-04-21T09:36:52.350Z
Learning: This repository guarantees Qt 6.10 as the minimum supported version. Therefore, when reviewing C++ code, do not require compatibility guards (e.g., QT_VERSION_CHECK) or fallback paths for Qt 6.10+ APIs/classes/enums. For example, do not flag uses of QSortFilterProxyModel::endFilterChange or QSortFilterProxyModel::Direction::Rows as needing guards for older Qt versions; such guards are unnecessary in this codebase.
Learnt from: Jojo-Schmitz
Repo: musescore/MuseScore PR: 33499
File: src/engraving/rendering/single/singledraw.cpp:1167-1169
Timestamp: 2026-05-20T11:29:20.381Z
Learning: When reviewing MSVC compiler warning fixes (notably warning C5054) in the engraving rendering C++ code, only flag `drawText`/painter call sites that bitwise-OR (`operator|`) `muse::draw` flags coming from *different* enum types unless the combination uses an explicit `static_cast<int>`. If the OR’d enumerators are all from the *same* enum type, they do not trigger C5054, so don’t require preemptive consistency casts—limit changes/flags to patterns that actually generate the compiler warning.
Jojo-Schmitz
force-pushed
the
date-macros
branch
from
5fc4da5 to
ef927a1
Compare
|
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. |
|
See #33797 for a draft idea / proof of concept |
4 participants
Resolves: #15437
Needs musescore/muse_framework#79