Repository navigation
Make Revision in Score properties and About box a clickable URL to the corresponding commit - #33712
Jojo-Schmitz wants to merge 8 commits into
Conversation
|
Important Review skippedWe couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
Walkthrough
Pre-merge checks:
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check |  |
The description summarizes the intended changes, but it does not follow the required template. It omits the required Resolves: #NNNNN reference and all checklist items, including CLA, title, coding … |
Update the PR description to use the repository template. Add the resolved issue number or link, complete every applicable checklist item with [x] or [ ], and list AI tools if AI assistance was used. |
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Title check The title names the main change: the Revision shown in Score properties becomes a clickable URL to the matching commit. The changeset does this in the project properties model. It also covers the Abou… Full details: Title check
-
Explanation
The title names the main change: the Revision shown in Score properties becomes a clickable URL to the matching commit. The changeset does this in the project properties model. It also covers the About box, which the title does not mention. The title is 91 characters and 15 words, so it is longer than the short, single-sentence form the guidelines prefer, but it is still specific and accurate.
-
Full details: Description check
-
Explanation
The description summarizes the intended changes, but it does not follow the required template. It omits the required
Resolves: #NNNNNreference and all checklist items, including CLA, title, coding rules, testing, commit, prior-attempt, and AI-assistance declarations.Resolution
Update the PR description to use the repository template. Add the resolved issue number or link, complete every applicable checklist item with
[x]or[ ], and list AI tools if AI assistance was used.
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create PR with unit tests
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Warning
Review ran into problems
🔥 Problems
-
Linked repositories: Your configuration references 1 linked repositories, but your current plan allows 0. Analyzed ``, skipped
musescore/muse_framework.git.
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/engraving/rendering/score/headerfooterlayout.cpp`:
- Around line 369-370: The else block with the comment around
newFragments.back().text += String(); has inconsistent indentation and fails
codestyle; run the codestyle formatter (tools/codestyle/uncrustify_run_file.sh)
on this source and fix the brace/indentation so the else, comment and statement
align with surrounding blocks, ensuring the comment stays on its own line or is
wrapped consistently and the statement newFragments.back().text += String(); is
properly indented to match surrounding code in headerfooterlayout.cpp.
- Around line 365-371: The numeric bases are swapped when converting rev to text
in headerfooterlayout.cpp: when rev > 0 && rev <= 5709 (SourceForge/decimal) use
String::number(rev, 10) instead of String::number(rev, 16), and when rev >
0xffffff (GitHub/hex) use String::number(rev, 16) instead of String::number(rev,
10); update the two String::number calls in the block that assigns to
newFragments.back().text so they match the convention used in
projectpropertiesmodel.cpp (decimal for SourceForge branch, hex for GitHub
branch), leaving the fallback else branch unchanged.
🪄 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: CHILL
Plan: Pro
Run ID: 7e0810fd-71fc-47c2-b671-4d29311a6d91
📥 Commits
Reviewing files that changed from the base of the PR and between 5189660 and 3c42419ddce7a3b72cd429f47ac5c147c2d60152.
📒 Files selected for processing (3)
src/engraving/dom/score.hsrc/engraving/rendering/score/headerfooterlayout.cppsrc/project/qml/MuseScore/Project/internal/Properties/projectpropertiesmodel.cpp
c8d6fa5 to
9d32837
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/appshell/qml/MuseScore/AppShell/aboutmodel.cpp (1)
86-98:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd validation for empty or unknown revision in clipboard text.
If
application()->revision()returns an empty string or "unknown", the Markdown link will be malformed:[](https://github.com/musescore/MuseScore/commit/). This produces a broken link when pasted into bug reports or forums. Consider adding validation to either omit the link or use plain text when the revision is unavailable.🛡️ Proposed fix with fallback handling
void AboutModel::copyRevisionToClipboard() const { + QString revision = application()->revision(); + QString revisionText = revision.isEmpty() || revision == "unknown" + ? muse::qtrc("appshell/about", "Unknown") + : QString("[%1](https://github.com/musescore/MuseScore/commit/%1)").arg(revision); + QApplication::clipboard()->setText( - QString("OS: %1, Arch.: %2, MuseScore Studio version (%3-bit): %4-%5, revision: [%6](https://github.com/musescore/MuseScore/commit/%6)") + QString("OS: %1, Arch.: %2, MuseScore Studio version (%3-bit): %4-%5, revision: %6") .arg(QSysInfo::prettyProductName() + ((QSysInfo::productType() == "windows" && (QSysInfo::productVersion() == "10" || QSysInfo::productVersion() == "11")) ? " or later" : "")) .arg(QSysInfo::currentCpuArchitecture()) .arg(QSysInfo::WordSize) .arg(application()->version().toString()) .arg(application()->build()) - .arg(application()->revision())); + .arg(revisionText)); }🤖 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/appshell/qml/MuseScore/AppShell/aboutmodel.cpp` around lines 86 - 98, AboutModel::copyRevisionToClipboard currently always injects application()->revision() into a Markdown link which yields a broken URL when the revision is empty or "unknown"; update this method to validate the revision string first and, if revision is empty or equals "unknown", format the "revision" part as plain text (e.g. "revision: unknown" or "revision: <rev>") instead of the Markdown link; keep using QApplication::clipboard()->setText and the existing QString assembly but branch on application()->revision() to choose between the linked form and the plain-text fallback so pasted text never contains an empty commit URL.
🤖 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 `@src/appshell/qml/MuseScore/AppShell/aboutmodel.cpp`:
- Around line 45-48: AboutModel::museScoreRevision currently always builds an
<a> anchor from configuration()->museScoreRevision(), which produces a
malformed/empty link when the revision string is empty or "unknown"; change the
function to first read QString rev =
QString::fromStdString(configuration()->museScoreRevision()), then if
rev.isEmpty() or rev.compare("unknown", Qt::CaseInsensitive) == 0 return a
localized fallback like tr("Unknown") (matching the project properties model),
otherwise return the formatted anchor using rev; ensure you use the same
localization mechanism used elsewhere (tr()) for the fallback text.
---
Outside diff comments:
In `@src/appshell/qml/MuseScore/AppShell/aboutmodel.cpp`:
- Around line 86-98: AboutModel::copyRevisionToClipboard currently always
injects application()->revision() into a Markdown link which yields a broken URL
when the revision is empty or "unknown"; update this method to validate the
revision string first and, if revision is empty or equals "unknown", format the
"revision" part as plain text (e.g. "revision: unknown" or "revision: <rev>")
instead of the Markdown link; keep using QApplication::clipboard()->setText and
the existing QString assembly but branch on application()->revision() to choose
between the linked form and the plain-text fallback so pasted text never
contains an empty commit URL.
🪄 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: CHILL
Plan: Pro
Run ID: 0d6f465f-4c3a-4ec4-b767-08e1b6c4934b
📥 Commits
Reviewing files that changed from the base of the PR and between 4e365f8a7f851ba2c060e4ddbecb585339af15a4 and c8d6fa5b950de0b01aed4525b3d6f847bb81cbc6.
📒 Files selected for processing (1)
src/appshell/qml/MuseScore/AppShell/aboutmodel.cpp
9d32837 to
5faf962
Compare
|
@coderabbitai, any idea what the codestylecheck is really trying to tell me, i.e. how it really wants the code formatted? Seems a plain white space issue, not visible in the check's output |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get the check runs for the PR
gh pr checks 33712 --repo musescore/MuseScore 2>/dev/null | head -50
echo "---"
# Get the PR files changed
gh pr diff 33712 --repo musescore/MuseScore 2>/dev/null | head -300Length of output: 7047 🏁 Script executed: #!/bin/bash
# Try to get the annotations/details from the failing codestyle check run
gh api repos/musescore/MuseScore/actions/runs/27067497966/jobs --jq '.jobs[] | select(.name | contains("codestyle") or contains("style")) | {name, conclusion, steps: [.steps[] | select(.conclusion == "failure" or .name | contains("style"))]}' 2>/dev/null
echo "=== Check annotations ==="
gh api repos/musescore/MuseScore/check-runs/79890796943/annotations 2>/dev/null | jq '.[] | {path, start_line, end_line, annotation_level, message, title}' 2>/dev/null
echo "=== Job log (last portion) ==="
gh api repos/musescore/MuseScore/actions/jobs/79890796943/logs 2>/dev/null | tail -100Length of output: 10507
Fix 1 – // Current (10 spaces before .arg):
return QString("<a href=\"https://github.com/musescore/MuseScore/commit/%1\">%1</a>")
.arg(QString::fromStdString(configuration()->museScoreRevision()));
// Wanted (8 spaces before .arg):
return QString("<a href=\"https://github.com/musescore/MuseScore/commit/%1\">%1</a>")
.arg(QString::fromStdString(configuration()->museScoreRevision()));Fix 2 – adjacent string literal continuation: 14 → 16 spaces // Current (14 spaces):
QString("OS: %1, Arch.: %2, MuseScore Studio version (%3-bit): %4-%5, revision: "
"[%6](https://github.com/musescore/MuseScore/commit/%6)")
// Wanted (16 spaces, aligning the opening " with the one on the line above):
QString("OS: %1, Arch.: %2, MuseScore Studio version (%3-bit): %4-%5, revision: "
"[%6](https://github.com/musescore/MuseScore/commit/%6)")Both are purely indentation — the style checker aligns method-chain continuations at 2× the base indent (8 spaces inside a function body), and expects adjacent string literals to have their |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get check runs for this PR
gh pr checks 33712 --repo musescore/MuseScore 2>/dev/null | head -50
echo "---"
# Get the check run details
gh api "repos/musescore/MuseScore/pulls/33712/commits" --jq '.[].sha' 2>/dev/null | head -5Length of output: 1925 🏁 Script executed: #!/bin/bash
# Get the diff of the PR to look for whitespace issues
gh pr diff 33712 --repo musescore/MuseScore 2>/dev/null | catLength of output: 5523
Issue 1 —
return QString("<a href=\"https://github.com/musescore/MuseScore/commit/%1\">%1</a>")
.arg(QString::fromStdString(configuration()->museScoreRevision()));
// ^ 11 spaces (was 10)Issue 2 —
QString("OS: %1, Arch.: %2, MuseScore Studio version (%3-bit): %4-%5, revision: "
"[%6](https://github.com/musescore/MuseScore/commit/%6)")
// ^ 16 spaces (was 14)Both are classic clang-format alignment requirements: chained method calls align under the start of the expression, and adjacent string literals align their opening quotes. Running 🐇 ( ´･_･`) |
93a1e53 to
145379b
Compare
|
@coderabbitai: thanks, fixed. That codestylecheck's output really needs to get fixed, dropping whitespace from the output is not helpful at all |
|
Tip For best results, initiate chat on the files or code changes.
🐇 (ﾉ◕ヮ◕)ﾉ*:･ﾟ✧ 🧠 Learnings usedIf you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
Jojo-Schmitz
changed the title
| if (rev > 0 && rev <= 5709) { // MuseScore 1.x and earlier used decimal numbers, referring to a commit on SourceForge.net, the largest being 5709 | ||
| revision = QString("<a href=\"https://sourceforge.net/p/mscore/code/%1/\">%1</a>").arg(QString::number(rev, 10)); | ||
| } else if (rev > 0xffffff) { // MuseScore 2.0 and later use a >= 7-digit hex SHA, referring to a commit on GitHub.com | ||
| revision = QString("<a href=\"https://github.com/musescore/MuseScore/commit/%1\">%1</a>").arg(QString::number(rev, 16)); |
There was a problem hiding this comment.
May want to cater for my 3.7 Evolution here, something like
revision = QString("<a href=\"https://github.com/%1/MuseScore/commit/%2\">%2</a>").arg(m_projectMetaInfo.musescoreVersion = "3.6.3" ? "Jojo-Schmitz" : "musescore", QString::number(rev, 16));but I don't know whether that'd pass muster?
f7ab221 to
353e332
Compare
|
@coderabbitai: what's the stylecheck issue now? |
| } | ||
|
|
||
| return revision; | ||
| // unknown, like in a self-built development version, or in a very old version of MuseScore before the revision number was tracked | ||
| return muse::qtrc("global", "Unknown"); |
There was a problem hiding this comment.
Not sure we'd want a translated text getting placed in the score's metadata?
And is inconsistent vs. how it is dealt with for header/footer
There was a problem hiding this comment.
Hmm - if I understand you correctly this doesn't actually modify the score's metadata though? It's purely used for UI purposes in the project properties dialog when we encounter a revision we don't recognise.
There was a problem hiding this comment.
Isn't the revision also written into the score?
There was a problem hiding this comment.
It is, but not using this method (it uses application()->revision() see here).
There was a problem hiding this comment.
Ah, I see, you're right.
Remaining question is whether to convert an unknown revision number for header/footer, it might be better to not even try (and print "0" in almost all cases, or some arbitrary and meaningless number between 5710 and 0xfffffe) and just print nothing instead?
f306c7d to
038d204
Compare
…e corresponding commit Backport of musescore#33712
|
There's still an issue: both revision URLs (in Score Properties and About) are not clickable, i.e. don't open the URL in a browser, which is pretty much the main point of this PR. In my 3.x version I had to add this (in AboutBox.ui and metaedit.ui) <property name="openExternalLinks">
<bool>true</bool>
</property>and additionally add that No idea what the corresponding mechanics in a .qml file (AboutDialog.qml and ProjectPropertiesFileInfoPanel.qml ?) would be |
038d204 to
ef61fd5
Compare
|
Rebased and including the latest muse_framworks, which is a prerequisite for having URLs work |
…e corresponding commit Backport of musescore#33712
|
Needs a rebase, bit-rot, for not having been merged regardles of 2 approvals 2 months ago... |
No URL in the score though, as that isn't supported yet, see musescore#18055
ef61fd5 to
2c94331
Compare
|
Possible (IMHO usefull) addition: turn the URLs in Project properties |
|
Seems the rebase didn't go well, all builds are failing, @coderabbitai? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
Linux x64, macOS, Windows, unit tests, and the visual-test build all report: The framework pointers explain the error:
The August commit added the URL-opening support you needed. The base framework commit already includes that commit, so you do not need to retain the older pointer. Restore The PR also changes This is different from the earlier Actions incident: jobs start, but CMake configuration fails before compilation. Codestyle passes. After correcting the submodule pointers, fresh CI runs must confirm whether any further errors remain. You are interacting with an AI system. |
|
None of these changes to the 2 submodules were intended... |
Update muse_Framwork, as a prerequisite for having URLs work
the old one makes sense on musescore.org, as it automagically turns into a URL there, but it doesn't work on GitHub. This way it works on both.
209b181 to
26b4ecd
Compare
$rMacro replacement to use a similar logicNo URL in the score though, as that isn't supported yet, see Be able to link clickable URLs in text (especially in exported PDFs) #18055
The old one makes sense on musescore.org, as it automagically turns into a URL there, but it doesn't work on GitHub. The plain number would work on GitHub, but not on musescore.org. This way (using MarkDown syntax) it works on both.
Cater for MuseScore 3.7 Evolution scores (and link to the corresponding commit)