moved app base logic to base app#32950
Merged
igorkorsukov merged 1 commit intomusescore:masterfrom Apr 9, 2026
Merged
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:
📝 WalkthroughWalkthroughCmdOptions was split: a new framework-level polymorphic 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/internal/guiapp.cpp (1)
78-96:⚠️ Potential issue | 🟠 MajorAvoid returning shared
m_appOptionsfor empty-args contexts.Returning the app-level options here means contexts created with empty args (including the initial context in
main.cpp:177) share the same mutableCmdOptionsinstance with the original app. This couples all empty-args contexts to the original startup state and risks inconsistency if options are modified after context creation.The method should return a fresh
CmdOptionsinstance (or a properly isolated copy) for empty-args contexts instead of reusing the shared object.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/internal/guiapp.cpp` around lines 78 - 96, The method MuseScoreGuiApp::makeContextOptions currently returns the shared m_appOptions when args is empty, causing contexts to share a mutable CmdOptions instance; change it to return a fresh shared_ptr<Cm dOptions> by constructing a new CmdOptions that copies the current m_appOptions state (e.g., make_shared<muse::CmdOptions>(*m_appOptions) or equivalent deep-copy) so empty-args contexts get an isolated instance; ensure you handle the case where m_appOptions may be null by creating a default-constructed CmdOptions when necessary and return that new shared_ptr instead of m_appOptions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/internal/commandlineparser.cpp`:
- Around line 351-354: Multiple branches (those checking m_parser.isSet for
score-media, score-meta, score-parts, score-parts-pdf, score-transpose,
score-elements, score-video and the source-update branch) access positional
arrays scorefiles or args2 at index 0 without bounds checks; add a guard that
verifies scorefiles.size() >= 1 (or args2.size() >= 1 for source-update) before
assigning m_options->converterTask.inputFile = scorefiles[0] or using args2[0],
and if the check fails set an error/usage state (e.g., set m_options->runMode to
show help or return an error) and log a clear message so malformed CLI input
cannot cause out-of-bounds access in the functions handling these ConvertType
branches.
In `@src/app/internal/consoleapp.cpp`:
- Around line 147-151: In MuseScoreConsoleApp::doStartupScenario, the result of
std::dynamic_pointer_cast to MuseScoreCmdOptions is not checked for null before
use; add a null check after obtaining options (from context(ctxId).options) and
handle the failure path (e.g., log an error and return/abort startup) similar to
the fix in applyCommandLineOptions so that options->runMode is only accessed
when options != nullptr.
- Around line 88-90: The code assumes
std::dynamic_pointer_cast<MuseScoreCmdOptions>(opt) returned a valid pointer but
doesn't check for nullptr; update the logic that obtains options (the
std::dynamic_pointer_cast result) to verify options != nullptr before
dereferencing and calling
uiConfiguration()->setCustomPhysicalDotsPerInch(options->ui.physicalDotsPerInch),
and handle the null case (e.g., log an error/warning and skip the call or use
safe defaults) so dereferencing options is never done when opt isn't a
MuseScoreCmdOptions.
In `@src/framework/global/globalmodule.cpp`:
- Around line 177-182: In GlobalModule::onPreInit() the code calls
IApplication::fullVersion() during early boot which can access uninitialized
services; replace that call with the static BaseApplication::appFullVersion()
for the log message (keep application->title() and application->build() as-is)
so the version string is obtained via BaseApplication::appFullVersion() instead
of application->fullVersion().
In `@src/framework/global/internal/baseapplication.cpp`:
- Around line 158-163: BaseApplication::doSetup currently asserts runMode ==
IApplication::RunMode::GuiApp which breaks ConsoleApplication startup; remove or
relax that assertion in BaseApplication::doSetup so doSetup runs for all valid
RunMode values (GuiApp, ConsoleApp, AudioPluginRegistration) or, if only
GUI-specific setup should run, move GUI-only logic into an override in
ConsoleApplication (i.e., override doSetup in ConsoleApplication and keep GUI
check there). Update the IF_ASSERT_FAILED block around BaseApplication::doSetup
to either be removed entirely or replaced with a validation that allows the
three enum values, and ensure ConsoleApplication::doSetup overrides when
console-specific behavior is required.
In `@src/framework/global/internal/guiapplication.cpp`:
- Around line 122-129: The code drops the false branch of loadMainWindow(ctxId)
leaving Context.initializing set and the splash open on QML load failure; add an
explicit failure path inside the outer QMetaObject::invokeMethod: when
loadMainWindow returns false, schedule a queued call (via
QMetaObject::invokeMethod(qApp, ... , Qt::QueuedConnection)) to run the same
cleanup steps used in MuseScoreGuiApp::doStartupScenario (or a new helper like
finishInitializationFailure) to close the splash and clear Context.initializing
for ctxId so the context is not left stuck in initializing state. Ensure the new
cleanup is invoked on the GUI thread and references loadMainWindow,
doStartupScenario (or the new failure helper) and Context.initializing.
- Around line 146-154: The sceneGraphError handler attached in the
QQmlApplicationEngine::objectCreated lambda is never reached because
QQmlComponent::create() is used; move the scene graph error connection to
immediately after the created window is obtained (after the QQuickWindow* window
returned by component.create()), use qobject_cast<QQuickWindow*>(...) with a
null-check and log on failure, and connect window->sceneGraphError to qApp
there; also ensure doDestroyContext() calls window->deleteLater() (instead of
only hiding/erasing) to release ownership and avoid the leak.
---
Outside diff comments:
In `@src/app/internal/guiapp.cpp`:
- Around line 78-96: The method MuseScoreGuiApp::makeContextOptions currently
returns the shared m_appOptions when args is empty, causing contexts to share a
mutable CmdOptions instance; change it to return a fresh shared_ptr<Cm dOptions>
by constructing a new CmdOptions that copies the current m_appOptions state
(e.g., make_shared<muse::CmdOptions>(*m_appOptions) or equivalent deep-copy) so
empty-args contexts get an isolated instance; ensure you handle the case where
m_appOptions may be null by creating a default-constructed CmdOptions when
necessary and return that new shared_ptr instead of m_appOptions.
🪄 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: c6491c08-fef4-463c-a436-a54ed7cb123e
📒 Files selected for processing (20)
src/app/appfactory.cppsrc/app/appfactory.hsrc/app/cmdoptions.hsrc/app/internal/commandlineparser.cppsrc/app/internal/commandlineparser.hsrc/app/internal/consoleapp.cppsrc/app/internal/consoleapp.hsrc/app/internal/guiapp.cppsrc/app/internal/guiapp.hsrc/app/main.cppsrc/framework/global/CMakeLists.txtsrc/framework/global/globalmodule.cppsrc/framework/global/internal/baseapplication.cppsrc/framework/global/internal/baseapplication.hsrc/framework/global/internal/cmdoptions.hsrc/framework/global/internal/consoleapplication.cppsrc/framework/global/internal/consoleapplication.hsrc/framework/global/internal/guiapplication.cppsrc/framework/global/internal/guiapplication.hsrc/framework/global/modularity/imodulesetup.h
💤 Files with no reviewable changes (1)
- src/framework/global/modularity/imodulesetup.h
igorkorsukov
force-pushed
the
w/ioc/step_41
branch
from
5fee536 to
cb1bce2
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
src/framework/global/globalmodule.cpp (1)
177-181:⚠️ Potential issue | 🟠 MajorUse
BaseApplication::appFullVersion()in pre-init.
GlobalModule::onPreInit()still runs before services are ready, soapplication->fullVersion()can hit the same early-boot hazard again. Keeptitle()andbuild()on the instance, but switch just the version to the static accessor.🛠️ Proposed fix
if (application) { LOGI() << "=== Started " << application->title() - << " " << application->fullVersion().toString() + << " " << BaseApplication::appFullVersion().toString() << ", build: " << application->build() << " ==="; }Based on learnings:
IApplication::fullVersion()must only be called after application/services initialization; for early boot code, usemuse::BaseApplication::appFullVersion()instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/global/globalmodule.cpp` around lines 177 - 181, GlobalModule::onPreInit logs application info too early: replace the instance call application->fullVersion() with the static early-boot accessor muse::BaseApplication::appFullVersion() to avoid calling IApplication::fullVersion() before services are ready; keep using application->title() and application->build(), but use muse::BaseApplication::appFullVersion() for the version string in the LOGI() call inside GlobalModule::onPreInit.src/framework/global/internal/guiapplication.cpp (2)
119-126:⚠️ Potential issue | 🟠 MajorHandle
loadMainWindow()failure explicitly.If Line 120 returns
false, this branch just falls through.MuseScoreGuiApp::doStartupScenario()(src/app/internal/guiapp.cpp, Lines 127-140) is where the splash is closed andContext.initializingis cleared, so a QML load failure leaves the startup context stuck in the initializing state with no cleanup path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/global/internal/guiapplication.cpp` around lines 119 - 126, The current invokeMethod lambda ignores a false return from loadMainWindow(ctxId) which leaves the startup Context.initializing stuck; modify the lambda in guiapplication.cpp to explicitly handle the failure branch from loadMainWindow(ctxId): when ok is false, perform the same cleanup that doStartupScenario(ctxId) would do (close splash, clear Context.initializing, log/report the QML load error, and ensure no further startup steps run) or else call a dedicated cleanup method (or call MuseScoreGuiApp::doStartupScenario in a failure-safe mode) so the splash and initializing state are always cleared on both success and failure.
143-151:⚠️ Potential issue | 🟠 MajorAttach
sceneGraphErrorto the created window and release it on teardown.
QQmlApplicationEngine::objectCreatedonly observesengine->load*()paths, but this code instantiates the root object viaQQmlComponent::create(). The handler on Lines 143-151 therefore never runs, Line 180 can dereference a null cast if the root object is not aQQuickWindow, anddoDestroyContext()never deletes the created window.🧩 Suggested fix
- QObject::connect(engine, &QQmlApplicationEngine::objectCreated, qApp, [](QObject* obj, const QUrl&) { - QQuickWindow* w = dynamic_cast<QQuickWindow*>(obj); - //! NOTE It is important that there is a connection to this signal with an error, - //! otherwise the default action will be performed - displaying a message and terminating. - //! We will not be able to switch to another backend. - QObject::connect(w, &QQuickWindow::sceneGraphError, qApp, [](QQuickWindow::SceneGraphError, const QString& msg) { - LOGE() << "scene graph error: " << msg; - }); - }, Qt::DirectConnection); - QString path = mainWindowQmlPath(platform); QQmlComponent component = QQmlComponent(engine, path); if (!component.isReady()) { LOGE() << "Failed to load main qml file, err: " << component.errorString(); return false; @@ - QQuickWindow* window = dynamic_cast<QQuickWindow*>(obj); + QQuickWindow* window = qobject_cast<QQuickWindow*>(obj); + if (!window) { + LOGE() << "Main QML root is not a QQuickWindow"; + obj->deleteLater(); + return false; + } + QObject::connect(window, &QQuickWindow::sceneGraphError, qApp, [](QQuickWindow::SceneGraphError, const QString& msg) { + LOGE() << "scene graph error: " << msg; + }); window->setOpacity(0.01); window->setVisible(true); @@ if (wit != m_windows.end()) { QQuickWindow* window = wit->second; window->setVisible(false); + window->deleteLater(); m_windows.erase(wit); }Also applies to: 166-184, 189-196
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/framework/global/internal/baseapplication.cpp`:
- Around line 370-378: The finish path fails to clean up contexts still marked
Context.initializing because destroyContext()/doDestroyContext() returns early
when initializing is true; update the teardown logic so finish()/doFinish()
forces full cleanup: either clear the initializing flag before calling
doDestroyContext() or add a force boolean to destroyContext()/doDestroyContext()
(e.g., destroyContext(ctx, force=true)) and when force is true bypass the
IF_ASSERT_FAILED(!initializing) check and run onDeinit(), qDeleteAll(ctx.setups)
and removeIoC(); apply the same change to the other finish/destroy call site so
both paths perform full teardown for initializing contexts.
In `@src/framework/global/internal/consoleapplication.h`:
- Around line 22-25: Remove the stray leading spaces before preprocessor
directives in this header to match project formatting: align the `#pragma once`
and `#include "baseapplication.h"` and `#include "cmdoptions.h"` directives to
the start of the line (no leading whitespace) in
src/framework/global/internal/consoleapplication.h so all three directives are
consistently left-aligned.
In `@src/framework/ui/internal/guiapplication.cpp`:
- Around line 183-189: The code dereferences ctxId->id when inserting into
m_windows without checking ctxId for null; update the logic around the
QQuickWindow* window assignment and the m_windows insertion (symbols: ctxId,
m_windows, QQuickWindow, window) to first verify ctxId is non-null (and
return/abort or skip insertion if it is null) and also guard the dynamic_cast
result (window) before calling methods or storing it; if ctxId is null, log or
handle the error and avoid m_windows[ctxId->id] access.
---
Duplicate comments:
In `@src/framework/global/globalmodule.cpp`:
- Around line 177-181: GlobalModule::onPreInit logs application info too early:
replace the instance call application->fullVersion() with the static early-boot
accessor muse::BaseApplication::appFullVersion() to avoid calling
IApplication::fullVersion() before services are ready; keep using
application->title() and application->build(), but use
muse::BaseApplication::appFullVersion() for the version string in the LOGI()
call inside GlobalModule::onPreInit.
In `@src/framework/global/internal/guiapplication.cpp`:
- Around line 119-126: The current invokeMethod lambda ignores a false return
from loadMainWindow(ctxId) which leaves the startup Context.initializing stuck;
modify the lambda in guiapplication.cpp to explicitly handle the failure branch
from loadMainWindow(ctxId): when ok is false, perform the same cleanup that
doStartupScenario(ctxId) would do (close splash, clear Context.initializing,
log/report the QML load error, and ensure no further startup steps run) or else
call a dedicated cleanup method (or call MuseScoreGuiApp::doStartupScenario in a
failure-safe mode) so the splash and initializing state are always cleared on
both success and 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: 42bd28af-50c3-46d1-ae2f-717630f37849
📒 Files selected for processing (22)
src/app/appfactory.cppsrc/app/appfactory.hsrc/app/cmdoptions.hsrc/app/internal/commandlineparser.cppsrc/app/internal/commandlineparser.hsrc/app/internal/consoleapp.cppsrc/app/internal/consoleapp.hsrc/app/internal/guiapp.cppsrc/app/internal/guiapp.hsrc/app/main.cppsrc/framework/global/CMakeLists.txtsrc/framework/global/globalmodule.cppsrc/framework/global/internal/baseapplication.cppsrc/framework/global/internal/baseapplication.hsrc/framework/global/internal/cmdoptions.hsrc/framework/global/internal/consoleapplication.cppsrc/framework/global/internal/consoleapplication.hsrc/framework/global/internal/guiapplication.cppsrc/framework/global/modularity/imodulesetup.hsrc/framework/ui/CMakeLists.txtsrc/framework/ui/internal/guiapplication.cppsrc/framework/ui/internal/guiapplication.h
💤 Files with no reviewable changes (1)
- src/framework/global/modularity/imodulesetup.h
igorkorsukov
force-pushed
the
w/ioc/step_41
branch
from
cb1bce2 to
169b645
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/app/internal/guiapp.cpp (1)
77-96: 🧹 Nitpick | 🔵 TrivialPointer lifetime risk in
makeContextOptions.The
argvvector holds pointers to the internal data ofargs_strings (line 85:argv[i] = args_[i].data()). This is safe here becausecommandLineParser.parse()completes beforeargs_goes out of scope, but this is fragile—ifCommandLineParserstores these pointers for later use, undefined behavior would occur.Consider documenting this assumption or using a safer approach.
📝 Documentation suggestion
if (args.size() > 0) { std::vector<std::string> args_ = args.toStdStringList(); args_.insert(args_.begin(), "dummy/path/to/app.exe"); // for compatibility const int argc = static_cast<int>(args_.size()); + // NOTE: argv pointers are valid only while args_ is in scope. + // CommandLineParser::parse() must not store these pointers. std::vector<char*> argv(argc + 1, nullptr); for (int i = 0; i < argc; ++i) { argv[i] = args_[i].data(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/internal/guiapp.cpp` around lines 77 - 96, The function MuseScoreGuiApp::makeContextOptions has two issues: it uses the wrong argc (it uses args.size() instead of the post-insert args_.size()) and it creates argv pointers into args_ which is fragile if CommandLineParser::parse stores them; fix by setting const int argc = static_cast<int>(args_.size()) -> use args_.size() after inserting the dummy element, and either modify CommandLineParser::parse to accept a std::vector<std::string> or to immediately copy the argv strings internally (so it does not retain pointers), or ensure args_ outlives any stored pointers (e.g., move args_ to a scope that lives as long as parsed options or return a stable structure). Update code paths around CommandLineParser::parse and CommandLineParser::options() accordingly and add a short comment documenting the lifetime assumption if you keep the current approach.
♻️ Duplicate comments (3)
src/app/internal/commandlineparser.cpp (2)
351-354:⚠️ Potential issue | 🟠 MajorGuard the first positional argument before accessing
scorefiles[0].This branch accesses
scorefiles[0]without first verifying thatscorefilesis non-empty. If--score-mediais passed without a positional score file, this will cause an out-of-bounds access.The same issue exists in:
--score-meta(line 364),--score-parts(line 370),--score-parts-pdf(line 376),--score-transpose(line 382),--score-elements(line 389), and--score-video(line 420).🐛 Suggested fix pattern
if (m_parser.isSet("score-media")) { + if (scorefiles.isEmpty()) { + LOGE() << "Option: --score-media requires an input file"; + } else { m_options->runMode = IApplication::RunMode::ConsoleApp; m_options->converterTask.type = ConvertType::ExportScoreMedia; m_options->converterTask.inputFile = scorefiles[0]; if (m_parser.isSet("highlight-config")) { m_options->converterTask.params[MuseScoreCmdOptions::ParamKey::HighlightConfigPath] = fromUserInputPath(m_parser.value("highlight-config")); } + } }Apply the same guard to all
score-*branches that accessscorefiles[0].🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/internal/commandlineparser.cpp` around lines 351 - 354, The branches that handle score-related flags (checked via m_parser.isSet("score-media"), "score-meta", "score-parts", "score-parts-pdf", "score-transpose", "score-elements", and "score-video") directly read scorefiles[0] without validating the container; add a guard that verifies scorefiles is non-empty before assigning m_options->converterTask.inputFile = scorefiles[0], and handle the error path (e.g., set an error state or print a message and return/exit) in each of those branches so you never perform an out-of-bounds access when no positional score file was provided.
392-404:⚠️ Potential issue | 🟠 MajorGuard
args2[0]access in--source-updatebranch.Line 397 accesses
args2[0]without first checking thatargs2contains at least one element. This will crash if--source-updateis invoked without any positional arguments.🐛 Suggested fix
if (m_parser.isSet("source-update")) { QStringList args2 = m_parser.positionalArguments(); + if (args2.isEmpty()) { + LOGE() << "Option: --source-update requires an input file"; + } else { m_options->runMode = IApplication::RunMode::ConsoleApp; m_options->converterTask.type = ConvertType::SourceUpdate; m_options->converterTask.inputFile = fromUserInputPath(args2[0]); if (args2.size() >= 2) { m_options->converterTask.params[MuseScoreCmdOptions::ParamKey::ScoreSource] = args2[1]; } else { LOGW() << "Option: --source-update no source specified"; } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/internal/commandlineparser.cpp` around lines 392 - 404, The code accesses args2[0] inside the m_parser.isSet("source-update") branch without ensuring positional arguments exist; update the branch to first check args2.size() >= 1 before calling fromUserInputPath(args2[0]) and assigning m_options->converterTask.inputFile, and if no positional args are present log a warning via LOGW() (or set an appropriate error/early-return) so the code does not dereference args2 out-of-bounds; reference m_parser.isSet("source-update"), args2, fromUserInputPath, and m_options->converterTask.inputFile when making the guard.src/framework/ui/internal/guiapplication.cpp (1)
187-191:⚠️ Potential issue | 🟡 MinorMissing null check on
windowafterdynamic_cast.While
ctxIdis now validated at line 136, thedynamic_cast<QQuickWindow*>(obj)at line 187 could return null ifobjis not aQQuickWindowsubtype. CallingsetOpacityandsetVisibleon a null pointer would crash.The past review flagged the
ctxIdnull check which was addressed. However, thewindownull check concern remains valid. The component creates the root object, but if it's unexpectedly not aQQuickWindow, the cast fails silently.🛡️ Proposed fix
QQuickWindow* window = dynamic_cast<QQuickWindow*>(obj); + IF_ASSERT_FAILED(window) { + return false; + } window->setOpacity(0.01); window->setVisible(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/ui/internal/guiapplication.cpp` around lines 187 - 191, After dynamic_casting obj to QQuickWindow* (the variable window) in the function where you call window->setOpacity and window->setVisible and then store it into m_windows[ctxId->id], add a null check for window; if the cast returns nullptr, log an error or warning with context (including ctxId->id if available), avoid calling setOpacity/setVisible, and do not insert a null pointer into m_windows (return or skip further initialization for that ctxId). This change should be applied around the dynamic_cast<QQuickWindow*>(obj) usage to prevent dereferencing a null window pointer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/framework/global/internal/baseapplication.cpp`:
- Around line 291-324: In BaseApplication::setupNewContext replace the
unsynchronized static int lastId with a static std::atomic<int> (initialized to
0) and perform an atomic increment when assigning ctxId->id (e.g., use fetch_add
or pre-increment semantics so each new context gets a unique id). Add the
<atomic> include and update the increment/assignment around ctxId->id so the
counter is thread-safe even if the function is later called from multiple
threads.
---
Outside diff comments:
In `@src/app/internal/guiapp.cpp`:
- Around line 77-96: The function MuseScoreGuiApp::makeContextOptions has two
issues: it uses the wrong argc (it uses args.size() instead of the post-insert
args_.size()) and it creates argv pointers into args_ which is fragile if
CommandLineParser::parse stores them; fix by setting const int argc =
static_cast<int>(args_.size()) -> use args_.size() after inserting the dummy
element, and either modify CommandLineParser::parse to accept a
std::vector<std::string> or to immediately copy the argv strings internally (so
it does not retain pointers), or ensure args_ outlives any stored pointers
(e.g., move args_ to a scope that lives as long as parsed options or return a
stable structure). Update code paths around CommandLineParser::parse and
CommandLineParser::options() accordingly and add a short comment documenting the
lifetime assumption if you keep the current approach.
---
Duplicate comments:
In `@src/app/internal/commandlineparser.cpp`:
- Around line 351-354: The branches that handle score-related flags (checked via
m_parser.isSet("score-media"), "score-meta", "score-parts", "score-parts-pdf",
"score-transpose", "score-elements", and "score-video") directly read
scorefiles[0] without validating the container; add a guard that verifies
scorefiles is non-empty before assigning m_options->converterTask.inputFile =
scorefiles[0], and handle the error path (e.g., set an error state or print a
message and return/exit) in each of those branches so you never perform an
out-of-bounds access when no positional score file was provided.
- Around line 392-404: The code accesses args2[0] inside the
m_parser.isSet("source-update") branch without ensuring positional arguments
exist; update the branch to first check args2.size() >= 1 before calling
fromUserInputPath(args2[0]) and assigning m_options->converterTask.inputFile,
and if no positional args are present log a warning via LOGW() (or set an
appropriate error/early-return) so the code does not dereference args2
out-of-bounds; reference m_parser.isSet("source-update"), args2,
fromUserInputPath, and m_options->converterTask.inputFile when making the guard.
In `@src/framework/ui/internal/guiapplication.cpp`:
- Around line 187-191: After dynamic_casting obj to QQuickWindow* (the variable
window) in the function where you call window->setOpacity and window->setVisible
and then store it into m_windows[ctxId->id], add a null check for window; if the
cast returns nullptr, log an error or warning with context (including ctxId->id
if available), avoid calling setOpacity/setVisible, and do not insert a null
pointer into m_windows (return or skip further initialization for that ctxId).
This change should be applied around the dynamic_cast<QQuickWindow*>(obj) usage
to prevent dereferencing a null window pointer.
🪄 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: 8852f1c2-7039-4052-b38f-fc51c38e4b4a
📒 Files selected for processing (22)
src/app/appfactory.cppsrc/app/appfactory.hsrc/app/cmdoptions.hsrc/app/internal/commandlineparser.cppsrc/app/internal/commandlineparser.hsrc/app/internal/consoleapp.cppsrc/app/internal/consoleapp.hsrc/app/internal/guiapp.cppsrc/app/internal/guiapp.hsrc/app/main.cppsrc/framework/global/CMakeLists.txtsrc/framework/global/globalmodule.cppsrc/framework/global/internal/baseapplication.cppsrc/framework/global/internal/baseapplication.hsrc/framework/global/internal/cmdoptions.hsrc/framework/global/internal/consoleapplication.cppsrc/framework/global/internal/consoleapplication.hsrc/framework/global/modularity/imodulesetup.hsrc/framework/testing/environment.cppsrc/framework/ui/CMakeLists.txtsrc/framework/ui/internal/guiapplication.cppsrc/framework/ui/internal/guiapplication.h
💤 Files with no reviewable changes (2)
- src/framework/testing/environment.cpp
- src/framework/global/modularity/imodulesetup.h
igorkorsukov
force-pushed
the
w/ioc/step_41
branch
from
169b645 to
a022f11
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
src/app/internal/commandlineparser.cpp (1)
351-354:⚠️ Potential issue | 🟠 MajorGuard the first positional argument in these console modes.
These branches read
scorefiles[0]without first checking that a positional score file was provided. This can cause an out-of-bounds access on malformed CLI input. The same issue applies to lines 364, 370, 376, 382, 389, 397, and 420.Line 319 shows the correct pattern already used for
-o:if (scorefiles.size() < 1) { LOGE() << "Option: ... no input file specified"; } else { m_options->converterTask.inputFile = scorefiles[0]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/internal/commandlineparser.cpp` around lines 351 - 354, Several console-mode branches assign m_options->converterTask.inputFile = scorefiles[0] without checking that a positional score file was provided, risking out-of-bounds access; for each branch (e.g., the m_parser.isSet("score-media") branch that sets RunMode to IApplication::RunMode::ConsoleApp and converterTask.type to ConvertType::ExportScoreMedia, and any other branches that similarly assign from scorefiles[0]) add a guard: if scorefiles.size() < 1 then log an error via LOGE() (matching the existing pattern used for the -o option) and do not assign inputFile, else assign inputFile = scorefiles[0]; apply this same check to every location that assigns from scorefiles[0].src/framework/ui/internal/guiapplication.cpp (1)
150-158:⚠️ Potential issue | 🔴 CriticalGuard the
QQuickWindowcast before dereferencing it.
QQmlApplicationEngine::objectCreatedcan delivernullptron load failure, and this startup path later dereferences thedynamic_cast<QQuickWindow*>result unconditionally. A broken or non-window root QML will crash here instead of failing cleanly. (doc.qt.io)🛡️ Proposed fix
QObject::connect(engine, &QQmlApplicationEngine::objectCreated, qApp, [](QObject* obj, const QUrl&) { QQuickWindow* w = dynamic_cast<QQuickWindow*>(obj); + if (!w) { + return; + } //! NOTE It is important that there is a connection to this signal with an error, //! otherwise the default action will be performed - displaying a message and terminating. //! We will not be able to switch to another backend. QObject::connect(w, &QQuickWindow::sceneGraphError, qApp, [](QQuickWindow::SceneGraphError, const QString& msg) { LOGE() << "scene graph error: " << msg; @@ QQuickWindow* window = dynamic_cast<QQuickWindow*>(obj); + IF_ASSERT_FAILED(window) { + LOGE() << "Main QML root object is not a QQuickWindow"; + delete obj; + return false; + } window->setOpacity(0.01); window->setVisible(true);Also applies to: 187-189
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/ui/internal/guiapplication.cpp` around lines 150 - 158, The dynamic_cast<QQuickWindow*> result in the QQmlApplicationEngine::objectCreated handler must be null-checked before using it; update the lambda attached to QQmlApplicationEngine::objectCreated (and the similar handler handling the later occurrence of QQuickWindow connection) to first verify that the incoming QObject* obj is non-null and that dynamic_cast<QQuickWindow*>(obj) returns a valid pointer, and only then call QObject::connect on w to bind QQuickWindow::sceneGraphError to qApp (and log via LOGE()); if the cast fails, simply return from the lambda so startup can fail cleanly without dereferencing a null pointer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/internal/commandlineparser.h`:
- Around line 45-51: The const accessor options() currently returns
std::shared_ptr<MuseScoreCmdOptions> allowing mutation through a const
CommandLineParser; change the API to return std::shared_ptr<const
MuseScoreCmdOptions> from options() const and make the stored member m_options a
std::shared_ptr<const MuseScoreCmdOptions> so the const contract is enforced;
update any places that assign or use m_options to respect the const-qualified
shared_ptr (e.g., construction/initialization sites and callers that expect
mutable access) and add a separate non-const accessor only if mutation is
required.
In `@src/app/internal/guiapp.cpp`:
- Around line 156-163: The code currently checks options->guitarPro.experimental
and options->guitarPro.linkedTabStaffCreated for presence but always calls
guitarProConfiguration()->setExperimental(true) and
setLinkedTabStaffCreated(true); instead, forward the actual optional value (or
its contained bool) to the setters so the explicit false is honored: locate uses
of guitarProConfiguration(), options->guitarPro.experimental, and
options->guitarPro.linkedTabStaffCreated and change the calls to pass the
std::optional<bool> (or
options->guitarPro.experimental.value()/linkedTabStaffCreated.value() if the
setter expects bool) rather than the hard-coded true; mirror the approach used
in consoleapp.cpp where the optional is forwarded to the configuration setters.
In `@src/framework/testing/environment.cpp`:
- Line 65: The test registers the mock IApplication under the "testing" key
which doesn't match production wiring; change the registration in
environment.cpp so
muse::modularity::globalIoc()->registerExport<IApplication>(...) uses the
"global" key instead of "testing" so it matches BaseApplication::setup() and
GlobalModule::onPreInit() resolution of IApplication and ensures the mock is
resolved in tests the same way as in production.
In `@src/framework/ui/internal/guiapplication.cpp`:
- Around line 167-191: The root QQuickWindow created via QQmlComponent::create()
in loadMainWindow() is unparented and never freed; set the QQuickWindow's
QObject parent (e.g., window->setParent(this) or another long-lived owner)
immediately after dynamic_cast in loadMainWindow() so it becomes owned, and
update doDestroyContext() to explicitly hide and schedule deletion (call
deleteLater() or otherwise delete the QQuickWindow stored in
m_windows[ctxId->id]) and remove the entry from m_windows; reference
loadMainWindow(), doDestroyContext(), m_windows, QQmlComponent::create(), and
QQuickWindow to locate the changes.
---
Duplicate comments:
In `@src/app/internal/commandlineparser.cpp`:
- Around line 351-354: Several console-mode branches assign
m_options->converterTask.inputFile = scorefiles[0] without checking that a
positional score file was provided, risking out-of-bounds access; for each
branch (e.g., the m_parser.isSet("score-media") branch that sets RunMode to
IApplication::RunMode::ConsoleApp and converterTask.type to
ConvertType::ExportScoreMedia, and any other branches that similarly assign from
scorefiles[0]) add a guard: if scorefiles.size() < 1 then log an error via
LOGE() (matching the existing pattern used for the -o option) and do not assign
inputFile, else assign inputFile = scorefiles[0]; apply this same check to every
location that assigns from scorefiles[0].
In `@src/framework/ui/internal/guiapplication.cpp`:
- Around line 150-158: The dynamic_cast<QQuickWindow*> result in the
QQmlApplicationEngine::objectCreated handler must be null-checked before using
it; update the lambda attached to QQmlApplicationEngine::objectCreated (and the
similar handler handling the later occurrence of QQuickWindow connection) to
first verify that the incoming QObject* obj is non-null and that
dynamic_cast<QQuickWindow*>(obj) returns a valid pointer, and only then call
QObject::connect on w to bind QQuickWindow::sceneGraphError to qApp (and log via
LOGE()); if the cast fails, simply return from the lambda so startup can fail
cleanly without dereferencing a null pointer.
🪄 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: 94eb180b-db10-4cdd-abde-eaca7ba0732c
📒 Files selected for processing (26)
FileTests_Exists_Append.txtFileTests_Exists_ReadWrite.txtFileTests_Exists_WriteOnly.txtFileTests_RW.txtsrc/app/appfactory.cppsrc/app/appfactory.hsrc/app/cmdoptions.hsrc/app/internal/commandlineparser.cppsrc/app/internal/commandlineparser.hsrc/app/internal/consoleapp.cppsrc/app/internal/consoleapp.hsrc/app/internal/guiapp.cppsrc/app/internal/guiapp.hsrc/app/main.cppsrc/framework/global/CMakeLists.txtsrc/framework/global/globalmodule.cppsrc/framework/global/internal/baseapplication.cppsrc/framework/global/internal/baseapplication.hsrc/framework/global/internal/cmdoptions.hsrc/framework/global/internal/consoleapplication.cppsrc/framework/global/internal/consoleapplication.hsrc/framework/global/modularity/imodulesetup.hsrc/framework/testing/environment.cppsrc/framework/ui/CMakeLists.txtsrc/framework/ui/internal/guiapplication.cppsrc/framework/ui/internal/guiapplication.h
💤 Files with no reviewable changes (1)
- src/framework/global/modularity/imodulesetup.h
Comment on lines
156
to
163
| if (guitarProConfiguration()) { | ||
| if (options.guitarPro.experimental) { | ||
| if (options->guitarPro.experimental) { | ||
| guitarProConfiguration()->setExperimental(true); | ||
| } | ||
|
|
||
| if (options.guitarPro.linkedTabStaffCreated) { | ||
| if (options->guitarPro.linkedTabStaffCreated) { | ||
| guitarProConfiguration()->setLinkedTabStaffCreated(true); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the file and check its content at the specified lines
cat -n src/app/internal/guiapp.cpp | sed -n '150,170p'Repository: musescore/MuseScore
Length of output: 639
🏁 Script executed:
# Look for cmdoptions.h to verify the type definitions
find . -name "cmdoptions.h" -type fRepository: musescore/MuseScore
Length of output: 130
🏁 Script executed:
# Search for the MuseScoreCmdOptions structure
rg "struct MuseScoreCmdOptions|class MuseScoreCmdOptions" -A 30Repository: musescore/MuseScore
Length of output: 1420
🏁 Script executed:
# Search for guitarProConfiguration definition to understand its type
rg "guitarProConfiguration\(\)" -B 2 -A 5 | head -100Repository: musescore/MuseScore
Length of output: 2946
🏁 Script executed:
# Get the full guitarPro struct definition
rg "struct.*guitarPro|\.guitarPro" -B 2 -A 15 src/app/cmdoptions.h | head -60Repository: musescore/MuseScore
Length of output: 45
🏁 Script executed:
# Find the setter function signatures
rg "setExperimental|setLinkedTabStaffCreated" -B 2 -A 3Repository: musescore/MuseScore
Length of output: 3816
🏁 Script executed:
# Check if std::optional<bool> can be implicitly converted to bool
rg "guitarProConfiguration\(\)" -B 5 | head -50Repository: musescore/MuseScore
Length of output: 2749
🏁 Script executed:
# Get the full guitarPro struct definition from cmdoptions.h
cat -n src/app/cmdoptions.h | grep -A 20 "struct {" | grep -A 10 "guitarPro"Repository: musescore/MuseScore
Length of output: 399
🏁 Script executed:
# Alternative search for guitarPro in cmdoptions.h
rg "guitarPro" src/app/cmdoptions.h -B 2 -A 10Repository: musescore/MuseScore
Length of output: 415
Pass the actual optional value, not a hard-coded true.
In guiapp.cpp lines 157-158 and 161-162, the code tests whether the optional exists (via operator bool()), then always passes true to the setters. This ignores the actual boolean value, so --experimental false would still enable the feature. The setters accept std::optional<bool>, so pass the optional directly as consoleapp.cpp does, or explicitly extract and forward the value.
Proposed fix
if (guitarProConfiguration()) {
- if (options->guitarPro.experimental) {
- guitarProConfiguration()->setExperimental(true);
+ if (options->guitarPro.experimental.has_value()) {
+ guitarProConfiguration()->setExperimental(options->guitarPro.experimental.value());
}
- if (options->guitarPro.linkedTabStaffCreated) {
- guitarProConfiguration()->setLinkedTabStaffCreated(true);
+ if (options->guitarPro.linkedTabStaffCreated.has_value()) {
+ guitarProConfiguration()->setLinkedTabStaffCreated(options->guitarPro.linkedTabStaffCreated.value());
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/internal/guiapp.cpp` around lines 156 - 163, The code currently
checks options->guitarPro.experimental and
options->guitarPro.linkedTabStaffCreated for presence but always calls
guitarProConfiguration()->setExperimental(true) and
setLinkedTabStaffCreated(true); instead, forward the actual optional value (or
its contained bool) to the setters so the explicit false is honored: locate uses
of guitarProConfiguration(), options->guitarPro.experimental, and
options->guitarPro.linkedTabStaffCreated and change the calls to pass the
std::optional<bool> (or
options->guitarPro.experimental.value()/linkedTabStaffCreated.value() if the
setter expects bool) rather than the hard-coded true; mirror the approach used
in consoleapp.cpp where the optional is forwarded to the configuration setters.
a022f11 to
8a88aaf
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
src/framework/testing/environment.cpp (1)
65-65:⚠️ Potential issue | 🟠 MajorRegister the mock
IApplicationunder"global".
GlobalModule::onPreInit()now resolvesIApplicationwith the"global"key, so keeping"testing"here makes the test container diverge from production wiring and leaves that resolve path empty in tests.♻️ Minimal fix
- muse::modularity::globalIoc()->registerExport<IApplication>("testing", std::make_shared<NiceMock<muse::ApplicationMock> >()); + muse::modularity::globalIoc()->registerExport<IApplication>("global", std::make_shared<NiceMock<muse::ApplicationMock>>());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/testing/environment.cpp` at line 65, The test IoC registers a mock IApplication under the "testing" key, but GlobalModule::onPreInit() resolves IApplication with the "global" key; change the registration call to registerExport<IApplication> to use the "global" key so tests wire the same as production (locate the call muse::modularity::globalIoc()->registerExport<IApplication>("testing", std::make_shared<NiceMock<muse::ApplicationMock> >()) and replace the "testing" key with "global").src/app/internal/commandlineparser.h (1)
45-45: 🛠️ Refactor suggestion | 🟠 MajorReturn a read-only options handle from the
constaccessor.Line 45 still lets callers mutate parser state through a
const CommandLineParser&. Preferstd::shared_ptr<const MuseScoreCmdOptions>here, and add a non-const accessor only if post-parse mutation is intentional.♻️ API shape
- std::shared_ptr<MuseScoreCmdOptions> options() const; + std::shared_ptr<const MuseScoreCmdOptions> options() const;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/internal/commandlineparser.h` at line 45, The const accessor CommandLineParser::options() currently returns std::shared_ptr<MuseScoreCmdOptions> allowing mutation through a const parser; change the const declaration to return std::shared_ptr<const MuseScoreCmdOptions> to enforce read-only access, and update the corresponding const method signature/implementation accordingly; only add or keep a non-const std::shared_ptr<MuseScoreCmdOptions> options() overload if deliberate post-parse mutation is required (adjust callers to use the non-const accessor when mutation is intended).src/app/internal/consoleapp.cpp (1)
149-154:⚠️ Potential issue | 🟠 MajorFail fast when the context options cast is invalid.
Line 153 just returns after the failed cast. In this console startup path, that leaves no
qApp->exit(...)call, so a bad options object can turn into a headless process that never terminates cleanly.🛠️ Suggested fix
void MuseScoreConsoleApp::doStartupScenario(const muse::modularity::ContextPtr& ctxId) { const std::shared_ptr<MuseScoreCmdOptions> options = std::dynamic_pointer_cast<MuseScoreCmdOptions>(context(ctxId).options); IF_ASSERT_FAILED(options) { + qApp->exit(1); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/internal/consoleapp.cpp` around lines 149 - 154, The NULL/invalid cast of context(ctxId).options to MuseScoreCmdOptions in MuseScoreConsoleApp::doStartupScenario currently just returns; change the failure path to fail fast by calling qApp->exit(...) with a non-zero status (and ensure QApplication instance exists) instead of silently returning. Locate the dynamic_pointer_cast to MuseScoreCmdOptions (variable options) and replace the early return inside the IF_ASSERT_FAILED block with a call to qApp->exit(1) (or equivalent error code) so the headless process terminates cleanly when options is invalid.src/app/internal/commandlineparser.cpp (1)
351-355:⚠️ Potential issue | 🔴 CriticalGuard positional file access in these console branches.
These paths still read
scorefiles[0]/args2[0]without verifying that a positional score was provided, so malformed CLI input can still crash before emitting a useful error.🛠️ Suggested pattern
+ auto firstScoreFile = [&](const char* option) -> std::optional<QString> { + if (scorefiles.isEmpty()) { + LOGE() << "Option: " << option << " no input file specified"; + return std::nullopt; + } + return scorefiles.front(); + }; + if (m_parser.isSet("score-media")) { m_options->runMode = IApplication::RunMode::ConsoleApp; m_options->converterTask.type = ConvertType::ExportScoreMedia; - m_options->converterTask.inputFile = scorefiles[0]; + if (auto input = firstScoreFile("--score-media")) { + m_options->converterTask.inputFile = *input; + } if (m_parser.isSet("highlight-config")) { m_options->converterTask.params[MuseScoreCmdOptions::ParamKey::HighlightConfigPath] = fromUserInputPath(m_parser.value("highlight-config")); } }Apply the same guard to the other
score-*branches, and requireargs2.size() >= 1before usingargs2[0]in--source-update.Also applies to: 361-364, 367-370, 373-376, 379-383, 386-389, 392-397, 417-421
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/internal/commandlineparser.cpp` around lines 351 - 355, Several console branches (e.g., the block guarded by m_parser.isSet("score-media") that assigns m_options->converterTask.inputFile = scorefiles[0], and other score-* branches that use scorefiles[0] or args2[0]) assume a positional file exists and can crash; add explicit guards checking scorefiles.size() >= 1 (and args2.size() >= 1 for --source-update) before accessing [0], and if the check fails set a user-friendly error via m_parser.error()/m_options error handling and return/exit the parse routine; update the same pattern in all other affected branches (the blocks indicated in the comment: the subsequent score-* branches around lines mentioned) so no branch dereferences scorefiles[0] or args2[0] without size validation.src/app/internal/guiapp.cpp (1)
153-160:⚠️ Potential issue | 🟠 MajorHonor explicit
falsefor the Guitar Pro CLI flags.These branches test whether the
std::optional<bool>is present, but then always writetrue. An explicitfalsefrom the command line still enables the feature.Proposed fix
if (guitarProConfiguration()) { - if (options->guitarPro.experimental) { - guitarProConfiguration()->setExperimental(true); + if (options->guitarPro.experimental.has_value()) { + guitarProConfiguration()->setExperimental(options->guitarPro.experimental.value()); } - if (options->guitarPro.linkedTabStaffCreated) { - guitarProConfiguration()->setLinkedTabStaffCreated(true); + if (options->guitarPro.linkedTabStaffCreated.has_value()) { + guitarProConfiguration()->setLinkedTabStaffCreated(options->guitarPro.linkedTabStaffCreated.value()); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/internal/guiapp.cpp` around lines 153 - 160, The current branches check presence of the std::optional<bool> flags but always call guitarProConfiguration()->setExperimental(true) and setLinkedTabStaffCreated(true), ignoring explicit false; change the logic in the block handling options->guitarPro.experimental and options->guitarPro.linkedTabStaffCreated so that when the optional has a value you pass that actual boolean to guitarProConfiguration()->setExperimental(...) and guitarProConfiguration()->setLinkedTabStaffCreated(...) (e.g., use .has_value() / .value() or operator* on the std::optional) so explicit false from the CLI is honored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/appfactory.cpp`:
- Around line 282-288: AppFactory::newApp currently dereferences the passed
shared_ptr options without checking for null, causing a crash when callers pass
nullptr; add a non-null guard at the start of AppFactory::newApp that validates
options (the shared_ptr<MuseScoreCmdOptions>&) and returns a clear diagnostic or
throws if it's null, then proceed to call newGuiApp(options) or
newConsoleApp(options); update any callers/tests if they rely on null behavior
and ensure the guard uses the same error reporting convention as the project
(e.g., throw std::invalid_argument or log and return nullptr) so newGuiApp and
newConsoleApp remain unchanged.
In `@src/app/internal/commandlineparser.h`:
- Around line 45-51: The header declares and uses std::shared_ptr in options()
and the m_options member but doesn’t include its definition, making the header
non–self-contained. Add `#include` <memory> at the top of this header so
std::shared_ptr used by options() and m_options is defined locally, avoiding
reliance on transitive includes.
In `@src/framework/ui/internal/guiapplication.cpp`:
- Around line 150-189: The sceneGraphError handler is being attached inside the
QQmlApplicationEngine::objectCreated lambda which won't run when the root is
created with QQmlComponent::create(); move the QObject::connect that installs
the QQuickWindow::sceneGraphError handler to after the root object is created
and casted (i.e., after QQmlComponent::create() and when creating QQuickWindow*
window = dynamic_cast<QQuickWindow*>(obj)), validate that window is non-null
before connecting, keep the connection target (qApp) and logging (LOGE()) as-is,
and remove the earlier connect-to-objectCreated block so the scene-graph error
handler is reliably installed for the window created by component.create().
---
Duplicate comments:
In `@src/app/internal/commandlineparser.cpp`:
- Around line 351-355: Several console branches (e.g., the block guarded by
m_parser.isSet("score-media") that assigns m_options->converterTask.inputFile =
scorefiles[0], and other score-* branches that use scorefiles[0] or args2[0])
assume a positional file exists and can crash; add explicit guards checking
scorefiles.size() >= 1 (and args2.size() >= 1 for --source-update) before
accessing [0], and if the check fails set a user-friendly error via
m_parser.error()/m_options error handling and return/exit the parse routine;
update the same pattern in all other affected branches (the blocks indicated in
the comment: the subsequent score-* branches around lines mentioned) so no
branch dereferences scorefiles[0] or args2[0] without size validation.
In `@src/app/internal/commandlineparser.h`:
- Line 45: The const accessor CommandLineParser::options() currently returns
std::shared_ptr<MuseScoreCmdOptions> allowing mutation through a const parser;
change the const declaration to return std::shared_ptr<const
MuseScoreCmdOptions> to enforce read-only access, and update the corresponding
const method signature/implementation accordingly; only add or keep a non-const
std::shared_ptr<MuseScoreCmdOptions> options() overload if deliberate post-parse
mutation is required (adjust callers to use the non-const accessor when mutation
is intended).
In `@src/app/internal/consoleapp.cpp`:
- Around line 149-154: The NULL/invalid cast of context(ctxId).options to
MuseScoreCmdOptions in MuseScoreConsoleApp::doStartupScenario currently just
returns; change the failure path to fail fast by calling qApp->exit(...) with a
non-zero status (and ensure QApplication instance exists) instead of silently
returning. Locate the dynamic_pointer_cast to MuseScoreCmdOptions (variable
options) and replace the early return inside the IF_ASSERT_FAILED block with a
call to qApp->exit(1) (or equivalent error code) so the headless process
terminates cleanly when options is invalid.
In `@src/app/internal/guiapp.cpp`:
- Around line 153-160: The current branches check presence of the
std::optional<bool> flags but always call
guitarProConfiguration()->setExperimental(true) and
setLinkedTabStaffCreated(true), ignoring explicit false; change the logic in the
block handling options->guitarPro.experimental and
options->guitarPro.linkedTabStaffCreated so that when the optional has a value
you pass that actual boolean to guitarProConfiguration()->setExperimental(...)
and guitarProConfiguration()->setLinkedTabStaffCreated(...) (e.g., use
.has_value() / .value() or operator* on the std::optional) so explicit false
from the CLI is honored.
In `@src/framework/testing/environment.cpp`:
- Line 65: The test IoC registers a mock IApplication under the "testing" key,
but GlobalModule::onPreInit() resolves IApplication with the "global" key;
change the registration call to registerExport<IApplication> to use the "global"
key so tests wire the same as production (locate the call
muse::modularity::globalIoc()->registerExport<IApplication>("testing",
std::make_shared<NiceMock<muse::ApplicationMock> >()) and replace the "testing"
key with "global").
🪄 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: 36d6d89d-7183-4605-9bf1-7e64c3b0b0d1
📒 Files selected for processing (26)
FileTests_Exists_Append.txtFileTests_Exists_ReadWrite.txtFileTests_Exists_WriteOnly.txtFileTests_RW.txtsrc/app/appfactory.cppsrc/app/appfactory.hsrc/app/cmdoptions.hsrc/app/internal/commandlineparser.cppsrc/app/internal/commandlineparser.hsrc/app/internal/consoleapp.cppsrc/app/internal/consoleapp.hsrc/app/internal/guiapp.cppsrc/app/internal/guiapp.hsrc/app/main.cppsrc/framework/global/CMakeLists.txtsrc/framework/global/globalmodule.cppsrc/framework/global/internal/baseapplication.cppsrc/framework/global/internal/baseapplication.hsrc/framework/global/internal/cmdoptions.hsrc/framework/global/internal/consoleapplication.cppsrc/framework/global/internal/consoleapplication.hsrc/framework/global/modularity/imodulesetup.hsrc/framework/testing/environment.cppsrc/framework/ui/CMakeLists.txtsrc/framework/ui/internal/guiapplication.cppsrc/framework/ui/internal/guiapplication.h
💤 Files with no reviewable changes (1)
- src/framework/global/modularity/imodulesetup.h
igorkorsukov
force-pushed
the
w/ioc/step_41
branch
3 times, most recently
from
c958894 to
ded29a1
Compare
Eism
approved these changes
Apr 9, 2026
igorkorsukov
force-pushed
the
w/ioc/step_41
branch
from
ded29a1 to
273368e
Compare
8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.