feat(ps1): Phase 1 EmuCore plugin, viewport, and session UI#577
Merged
Conversation
Introduce EmuCore/IEmuCorePlugin with QPluginLoader discovery, a stub core plugin for CI, worker-thread frame loop, EmuViewport session window, and first-run legality dialog. Tools > Experimental opens the ripper when ENABLE_PS1_RIP is on. Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughThis PR introduces a complete PS1 Runtime Ripper system with pluggable emulator cores. It defines abstract EmuCore and plugin interfaces, implements a stub core that renders test patterns, creates a worker thread to manage frame execution, builds a manager for session lifecycle control, and delivers a Qt session window with viewport rendering, BIOS/ISO file picking, and playback controls. ChangesPS1 Runtime Ripper Plugin Architecture & Session UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Poem
🚥 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 docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5333e3bca2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Native QFileDialog can hang when opened from the Ogre-hosted main window event loop. Match the rest of the app with DontUseNativeDialog and defer restoring the saved BIOS path until after the window is shown. Co-authored-by: Cursor <cursoragent@cursor.com>
Cancel pending startup via an atomic flag checked after core load so Stop before the worker finishes no longer leaves a session running. Reset waits for sessionStopped before restarting. Link PS1 runtime sources into MaterialEditorQML test targets when ENABLE_PS1_RIP is on. Co-authored-by: Cursor <cursoragent@cursor.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 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 `@plugins/ps1core_stub/StubEmuCore.cpp`:
- Around line 53-55: The reset leaves stale per-buffer phase because
fillTestPattern reads buf.frameIndex while reset() only resets m_frameIndex; fix
by initializing the selected write buffer's frame state before regenerating
pixels: set the write buffer's frameIndex (or equivalent phase/state in
m_buffers[m_writeIndex]) to m_frameIndex (or 0) prior to calling
fillTestPattern, and then set m_readIndex = m_writeIndex so the displayed frame
uses the new phase; update StubEmuCore::reset() to adjust
m_buffers[m_writeIndex]'s phase field (or call a helper to sync buffer state) so
no old phase remains.
In `@src/PS1/runtime/EmuCoreLoader_test.cpp`:
- Around line 49-51: Replace the runtime skip with a hard test failure so
missing plugin fails CI: in EmuCoreLoader_test.cpp where the code currently does
"if (!found) { GTEST_SKIP() << ...; }", change it to assert the prerequisite
(e.g., use ASSERT_TRUE(found) << "PS1 stub core plugin not built beside test
binary"; or alternatively FAIL() << "PS1 stub core plugin not built beside test
binary";) so the test fails loudly when the plugin is missing.
In `@src/PS1/runtime/EmuCoreLoader.cpp`:
- Around line 11-14: bundleOrAppDir() currently returns
QCoreApplication::applicationDirPath(), which points to Contents/MacOS inside
.app bundles; change it to return the bundle root on macOS by using
QCoreApplication::applicationDirPath() fallback to
QCoreApplication::applicationDirPath() only on non-macOS and use
QCoreApplication::applicationDirPath() replacement
QCoreApplication::macBundlePath() under a Q_OS_MAC (or `#ifdef` Q_OS_MAC) guard so
PS1Cores and any code referencing bundleOrAppDir() (also update the similar
logic around the other occurrence at the 31-37 region) resolve resources
relative to the .app bundle root as required by resources.cfg.
- Around line 16-26: The plugin discovery currently hardcodes only the
"qtmesh_ps1core_stub" filenames in the pluginFileNames() implementation,
preventing loading of real core plugins; change pluginFileNames() (and the
duplicate implementation further down) to either return broader
platform-specific name patterns like "qtmesh_ps1core*" or replace the hardcoded
list with a small discovery routine that uses QDir/QDir::entryList name filters
(e.g., "qtmesh_ps1core*.dll"/"qtmesh_ps1core*.dylib"/"qtmesh_ps1core*.so") to
enumerate all matching files in the plugin directory so real core plugins are
found in addition to the stub.
- Line 69: The current loader returns immediately whatever plugin->createCore()
yields, so if createCore() returns nullptr the loader exits without trying other
candidates; change the logic in EmuCoreLoader.cpp where plugin->createCore() is
invoked to call and store the result in a variable (e.g., auto core =
plugin->createCore()), check if core != nullptr and return it only then,
otherwise log/ignore that plugin and continue iterating over remaining plugins;
after the loop, return nullptr (or produce the existing error path) to indicate
no plugin produced a valid core.
In `@src/PS1/runtime/EmuFramebuffer.h`:
- Line 15: The isValid() check can overflow because width*height*3 is computed
in signed int; change it to compute the expected byte count in an unsigned size
type and compare to rgb24.size(). Specifically, in isValid() compute expected as
size_t(width) * size_t(height) * 3u (and keep the existing width > 0 && height >
0 checks) and then compare rgb24.size() == expected; update the function
signature/implementation of isValid() to use that computed size to avoid signed
overflow when validating rgb24.
In `@src/PS1/runtime/EmuViewport.cpp`:
- Around line 16-20: The code returns early on a null input, leaving m_frame
unchanged and showing stale content; change the flow so m_frame is cleared when
frame.isNull() and the view is updated: assign m_frame = frame (or explicitly
clear m_frame) even when frame.isNull(), then call update() so the "No frame"
state is rendered instead of keeping the previous frame; adjust the block around
frame.isNull(), m_frame and update() (referencing m_frame, frame.isNull(), and
update()) to implement this.
In `@src/PS1/runtime/PS1RipLegalityDialog.cpp`:
- Around line 61-62: Replace the custom breadcrumb category used in the
user-acknowledgement call with the mandated UI action category: update the
SentryReporter::addBreadcrumb call in PS1RipLegalityDialog (the invocation that
currently uses "ps1.rip.dialog.acknowledged") to use "ui.action" as the category
while preserving the existing message ("user acknowledged legality notice") so
the breadcrumb conforms to the required taxonomy for user-facing actions.
- Around line 57-65: The accept path should be blocked unless the acknowledgment
checkbox is checked: in PS1RipLegalityDialog::accept(), only call
setAcknowledged(...) and QDialog::accept() when m_ackCheck->isChecked(); refrain
from calling QDialog::accept() (and optionally setAcknowledged(false) or show a
warning) when the checkbox is unchecked. Update the method so
SentryReporter::addBreadcrumb and QDialog::accept() are executed inside the
m_ackCheck->isChecked() branch and do not close the dialog otherwise.
In `@src/PS1/runtime/PS1RipManager_test.cpp`:
- Line 69: The test currently uses a tautological assertion
EXPECT_GE(errorSpy.count(), 0) which always passes; update the assertion to
verify an actual error was emitted by replacing it with a meaningful check such
as EXPECT_GT(errorSpy.count(), 0) (or EXPECT_EQ(errorSpy.count(),
<expected_count>) if you know the exact number) so the test fails when no errors
are produced; target the assertion that calls errorSpy.count() in
PS1RipManager_test.cpp.
- Around line 105-108: The skip branch after ASSERT_TRUE(startedSpy.wait(3000))
is unreachable; change the flow to first capture the wait result (call
startedSpy.wait(3000) into a bool), then if the wait failed and
startedSpy.empty() && !errorSpy.empty() call GTEST_SKIP(), and finally assert
the wait succeeded (ASSERT_TRUE on the captured bool). This uses
startedSpy.wait, startedSpy.empty, errorSpy.empty and preserves the ASSERT_TRUE
semantics while allowing the conditional GTEST_SKIP path to execute.
In `@src/PS1/runtime/PS1RipManager.cpp`:
- Line 107: The breadcrumb at SentryReporter::addBreadcrumb currently sends the
full absolute path (m_biosPath), which leaks local file paths; change it to send
only the filename by extracting QFileInfo(m_biosPath).fileName() (or equivalent)
and set the category to the mandated I/O category "file.import"; do the same fix
for the other call referenced (line 124) so both SentryReporter::addBreadcrumb
invocations use the sanitized filename and category "file.import" instead of the
absolute path.
- Around line 45-57: The pause state is being cleared in the emulationStarted
and emulationStopped handlers (m_paused = false) but no signal is emitted to
notify listeners; update the lambda connected to PS1RipWorker::emulationStarted
and the one connected to PS1RipWorker::emulationStopped to emit
pausedChanged(false) immediately after clearing m_paused so UI listeners can
update; reference the connect calls for PS1RipWorker::emulationStarted and
PS1RipWorker::emulationStopped and the m_paused member and pausedChanged signal
when making the change.
In `@src/PS1/runtime/PS1RipSessionWindow.cpp`:
- Around line 38-52: Add Sentry breadcrumbs in each toolbar/action handler: in
pickBios and pickIso emit a breadcrumb with category "file.import" describing
the chosen file/action, and in onStart, onStop, onPause, onStep, and onReset
emit breadcrumbs with category "ui.action" describing the control invoked;
locate these methods in PS1RipSessionWindow (pickBios, pickIso, onStart, onStop,
onPause, onStep, onReset) and call the project's Sentry breadcrumb API (or
Sentry::addBreadcrumb equivalent) at the start of each handler with a short
message and appropriate category/level to match the coding guideline.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4f75a981-01ab-4a2e-87de-24459a22eb70
📒 Files selected for processing (31)
CLAUDE.mdCMakeLists.txtplugins/ps1core_stub/CMakeLists.txtplugins/ps1core_stub/StubEmuCore.cppplugins/ps1core_stub/StubEmuCore.hplugins/ps1core_stub/StubEmuCorePlugin.cppplugins/ps1core_stub/StubEmuCorePlugin.hplugins/ps1core_stub/ps1core_stub.jsonsrc/CMakeLists.txtsrc/PS1/CMakeLists.txtsrc/PS1/PS1_RIP_DESIGN.mdsrc/PS1/runtime/EmuCore.hsrc/PS1/runtime/EmuCoreLoader.cppsrc/PS1/runtime/EmuCoreLoader.hsrc/PS1/runtime/EmuCoreLoader_test.cppsrc/PS1/runtime/EmuFramebuffer.hsrc/PS1/runtime/EmuHooks.hsrc/PS1/runtime/EmuViewport.cppsrc/PS1/runtime/EmuViewport.hsrc/PS1/runtime/IEmuCorePlugin.hsrc/PS1/runtime/PS1RipLegalityDialog.cppsrc/PS1/runtime/PS1RipLegalityDialog.hsrc/PS1/runtime/PS1RipManager.cppsrc/PS1/runtime/PS1RipManager.hsrc/PS1/runtime/PS1RipManager_test.cppsrc/PS1/runtime/PS1RipSessionWindow.cppsrc/PS1/runtime/PS1RipSessionWindow.hsrc/PS1/runtime/PS1RipWorker.cppsrc/PS1/runtime/PS1RipWorker.hsrc/mainwindow.cpptests/CMakeLists.txt
Comment on lines
+53
to
+55
| m_frameIndex = 0; | ||
| fillTestPattern(m_buffers[m_writeIndex]); | ||
| m_readIndex = m_writeIndex; |
There was a problem hiding this comment.
Reset leaves stale phase state in the first post-reset frame.
Line [54] regenerates pixels from buf.frameIndex, but reset() only clears m_frameIndex. If the selected write buffer was previously used, the displayed reset frame can start from an old phase.
Suggested fix
void StubEmuCore::reset()
{
m_frameIndex = 0;
- fillTestPattern(m_buffers[m_writeIndex]);
+ EmuFramebuffer &buf = m_buffers[static_cast<size_t>(m_writeIndex)];
+ buf.frameIndex = m_frameIndex;
+ fillTestPattern(buf);
m_readIndex = m_writeIndex;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| m_frameIndex = 0; | |
| fillTestPattern(m_buffers[m_writeIndex]); | |
| m_readIndex = m_writeIndex; | |
| m_frameIndex = 0; | |
| EmuFramebuffer &buf = m_buffers[static_cast<size_t>(m_writeIndex)]; | |
| buf.frameIndex = m_frameIndex; | |
| fillTestPattern(buf); | |
| m_readIndex = m_writeIndex; |
🤖 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 `@plugins/ps1core_stub/StubEmuCore.cpp` around lines 53 - 55, The reset leaves
stale per-buffer phase because fillTestPattern reads buf.frameIndex while
reset() only resets m_frameIndex; fix by initializing the selected write
buffer's frame state before regenerating pixels: set the write buffer's
frameIndex (or equivalent phase/state in m_buffers[m_writeIndex]) to
m_frameIndex (or 0) prior to calling fillTestPattern, and then set m_readIndex =
m_writeIndex so the displayed frame uses the new phase; update
StubEmuCore::reset() to adjust m_buffers[m_writeIndex]'s phase field (or call a
helper to sync buffer state) so no old phase remains.
Comment on lines
+49
to
+51
| if (!found) { | ||
| GTEST_SKIP() << "PS1 stub core plugin not built beside test binary"; | ||
| } |
There was a problem hiding this comment.
Don’t skip this prerequisite; fail loudly when the plugin is missing.
Skipping here can mask CI/build-output regressions in plugin installation. This should be an assertion failure.
Proposed fix
- if (!found) {
- GTEST_SKIP() << "PS1 stub core plugin not built beside test binary";
- }
+ ASSERT_TRUE(found) << "PS1 stub core plugin not built beside test binary";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (!found) { | |
| GTEST_SKIP() << "PS1 stub core plugin not built beside test binary"; | |
| } | |
| ASSERT_TRUE(found) << "PS1 stub core plugin not built beside test binary"; |
🤖 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/PS1/runtime/EmuCoreLoader_test.cpp` around lines 49 - 51, Replace the
runtime skip with a hard test failure so missing plugin fails CI: in
EmuCoreLoader_test.cpp where the code currently does "if (!found) { GTEST_SKIP()
<< ...; }", change it to assert the prerequisite (e.g., use ASSERT_TRUE(found)
<< "PS1 stub core plugin not built beside test binary"; or alternatively FAIL()
<< "PS1 stub core plugin not built beside test binary";) so the test fails
loudly when the plugin is missing.
Comment on lines
+11
to
+14
| QString bundleOrAppDir() | ||
| { | ||
| return QCoreApplication::applicationDirPath(); | ||
| } |
There was a problem hiding this comment.
Use bundle-root path resolution on macOS for PS1Cores.
applicationDirPath() points to .../Contents/MacOS in app bundles, not the bundle root expected by the PR contract.
As per coding guidelines "Use macBundlePath() to get .app bundle root on macOS; Ogre resource paths in resources.cfg resolve relative to bundle root, not Contents/MacOS/".
Also applies to: 31-37
🤖 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/PS1/runtime/EmuCoreLoader.cpp` around lines 11 - 14, bundleOrAppDir()
currently returns QCoreApplication::applicationDirPath(), which points to
Contents/MacOS inside .app bundles; change it to return the bundle root on macOS
by using QCoreApplication::applicationDirPath() fallback to
QCoreApplication::applicationDirPath() only on non-macOS and use
QCoreApplication::applicationDirPath() replacement
QCoreApplication::macBundlePath() under a Q_OS_MAC (or `#ifdef` Q_OS_MAC) guard so
PS1Cores and any code referencing bundleOrAppDir() (also update the similar
logic around the other occurrence at the 31-37 region) resolve resources
relative to the .app bundle root as required by resources.cfg.
Comment on lines
+16
to
+26
| QStringList pluginFileNames() | ||
| { | ||
| #if defined(Q_OS_WIN) | ||
| return {QStringLiteral("qtmesh_ps1core_stub.dll")}; | ||
| #elif defined(Q_OS_MACOS) | ||
| return {QStringLiteral("libqtmesh_ps1core_stub.dylib"), | ||
| QStringLiteral("qtmesh_ps1core_stub.dylib")}; | ||
| #else | ||
| return {QStringLiteral("libqtmesh_ps1core_stub.so"), | ||
| QStringLiteral("qtmesh_ps1core_stub.so")}; | ||
| #endif |
There was a problem hiding this comment.
Loader discovery is hardcoded to the stub plugin name.
This only loads qtmesh_ps1core_stub* and prevents discovering any real core plugin with a different filename.
Proposed fix
-QStringList pluginFileNames()
-{
-#if defined(Q_OS_WIN)
- return {QStringLiteral("qtmesh_ps1core_stub.dll")};
-#elif defined(Q_OS_MACOS)
- return {QStringLiteral("libqtmesh_ps1core_stub.dylib"),
- QStringLiteral("qtmesh_ps1core_stub.dylib")};
-#else
- return {QStringLiteral("libqtmesh_ps1core_stub.so"),
- QStringLiteral("qtmesh_ps1core_stub.so")};
-#endif
-}
+QStringList pluginFileNames(const QDir &dir)
+{
+ QStringList out;
+ const QFileInfoList entries = dir.entryInfoList(QDir::Files);
+ for (const QFileInfo &fi : entries) {
+ if (QLibrary::isLibrary(fi.fileName()))
+ out << fi.fileName();
+ }
+ return out;
+}
...
- const QStringList names = pluginFileNames();
for (const QString &dirPath : coreSearchPaths()) {
const QDir dir(dirPath);
if (!dir.exists())
continue;
- for (const QString &fileName : names) {
+ for (const QString &fileName : pluginFileNames(dir)) {
const QString pluginPath = dir.filePath(fileName);Also applies to: 43-51
🧰 Tools
🪛 Cppcheck (2.20.0)
[error] 22-22: There is an unknown macro here somewhere. Configuration is required. If Q_DECLARE_INTERFACE is a macro then please configure it.
(unknownMacro)
🤖 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/PS1/runtime/EmuCoreLoader.cpp` around lines 16 - 26, The plugin discovery
currently hardcodes only the "qtmesh_ps1core_stub" filenames in the
pluginFileNames() implementation, preventing loading of real core plugins;
change pluginFileNames() (and the duplicate implementation further down) to
either return broader platform-specific name patterns like "qtmesh_ps1core*" or
replace the hardcoded list with a small discovery routine that uses
QDir/QDir::entryList name filters (e.g.,
"qtmesh_ps1core*.dll"/"qtmesh_ps1core*.dylib"/"qtmesh_ps1core*.so") to enumerate
all matching files in the plugin directory so real core plugins are found in
addition to the stub.
| continue; | ||
| } | ||
|
|
||
| return plugin->createCore(); |
There was a problem hiding this comment.
Handle null createCore() and keep searching candidates.
If a plugin loads but returns nullptr, the loader exits early instead of trying other plugins and returning a useful error.
Proposed fix
- return plugin->createCore();
+ if (std::unique_ptr<EmuCore> core = plugin->createCore())
+ return core;
+ if (errorOut)
+ *errorOut = QObject::tr("Plugin loaded but failed to create core: %1").arg(pluginPath);
+ continue;🤖 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/PS1/runtime/EmuCoreLoader.cpp` at line 69, The current loader returns
immediately whatever plugin->createCore() yields, so if createCore() returns
nullptr the loader exits without trying other candidates; change the logic in
EmuCoreLoader.cpp where plugin->createCore() is invoked to call and store the
result in a variable (e.g., auto core = plugin->createCore()), check if core !=
nullptr and return it only then, otherwise log/ignore that plugin and continue
iterating over remaining plugins; after the loop, return nullptr (or produce the
existing error path) to indicate no plugin produced a valid core.
| EXPECT_FALSE(manager->start()); | ||
| EXPECT_TRUE(manager->isoPath().isEmpty()); | ||
| EXPECT_GE(errorSpy.count(), 1); | ||
| EXPECT_GE(errorSpy.count(), 0); |
There was a problem hiding this comment.
Replace the tautological signal assertion.
Line 69 always passes, so it doesn’t validate error emission. Assert a real expectation instead.
Proposed fix
- EXPECT_GE(errorSpy.count(), 0);
+ EXPECT_GT(errorSpy.count(), 0);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| EXPECT_GE(errorSpy.count(), 0); | |
| EXPECT_GT(errorSpy.count(), 0); |
🤖 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/PS1/runtime/PS1RipManager_test.cpp` at line 69, The test currently uses a
tautological assertion EXPECT_GE(errorSpy.count(), 0) which always passes;
update the assertion to verify an actual error was emitted by replacing it with
a meaningful check such as EXPECT_GT(errorSpy.count(), 0) (or
EXPECT_EQ(errorSpy.count(), <expected_count>) if you know the exact number) so
the test fails when no errors are produced; target the assertion that calls
errorSpy.count() in PS1RipManager_test.cpp.
Comment on lines
+105
to
+108
| ASSERT_TRUE(startedSpy.wait(3000)); | ||
| if (startedSpy.empty() && !errorSpy.empty()) | ||
| GTEST_SKIP() << "Emulator failed to start in test environment"; | ||
|
|
There was a problem hiding this comment.
Fix unreachable skip logic in startup wait path.
After ASSERT_TRUE(startedSpy.wait(3000)), the skip branch can’t execute. This currently hard-fails in environments where startup times out.
Proposed fix
- ASSERT_TRUE(startedSpy.wait(3000));
- if (startedSpy.empty() && !errorSpy.empty())
- GTEST_SKIP() << "Emulator failed to start in test environment";
+ if (!startedSpy.wait(3000)) {
+ if (!errorSpy.empty())
+ GTEST_SKIP() << "Emulator failed to start in test environment";
+ FAIL() << "Session did not start within timeout";
+ }🤖 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/PS1/runtime/PS1RipManager_test.cpp` around lines 105 - 108, The skip
branch after ASSERT_TRUE(startedSpy.wait(3000)) is unreachable; change the flow
to first capture the wait result (call startedSpy.wait(3000) into a bool), then
if the wait failed and startedSpy.empty() && !errorSpy.empty() call
GTEST_SKIP(), and finally assert the wait succeeded (ASSERT_TRUE on the captured
bool). This uses startedSpy.wait, startedSpy.empty, errorSpy.empty and preserves
the ASSERT_TRUE semantics while allowing the conditional GTEST_SKIP path to
execute.
Comment on lines
+45
to
+57
| connect(m_worker, &PS1RipWorker::emulationStarted, this, [this]() { | ||
| m_startPending = false; | ||
| m_sessionActive = true; | ||
| m_paused = false; | ||
| emit sessionStarted(); | ||
| }); | ||
| connect(m_worker, &PS1RipWorker::emulationStopped, this, [this]() { | ||
| m_startPending = false; | ||
| m_sessionActive = false; | ||
| m_paused = false; | ||
| m_captureArmed = false; | ||
| emit sessionStopped(); | ||
| }); |
There was a problem hiding this comment.
Emit pausedChanged(false) when start/stop clears pause state.
m_paused is reset on start/stop, but listeners are never notified. That can leave pause UI state stale.
Proposed fix
connect(m_worker, &PS1RipWorker::emulationStarted, this, [this]() {
+ const bool wasPaused = m_paused;
m_startPending = false;
m_sessionActive = true;
m_paused = false;
+ if (wasPaused)
+ emit pausedChanged(false);
emit sessionStarted();
});
connect(m_worker, &PS1RipWorker::emulationStopped, this, [this]() {
+ const bool wasPaused = m_paused;
m_startPending = false;
m_sessionActive = false;
m_paused = false;
m_captureArmed = false;
+ if (wasPaused)
+ emit pausedChanged(false);
emit sessionStopped();
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| connect(m_worker, &PS1RipWorker::emulationStarted, this, [this]() { | |
| m_startPending = false; | |
| m_sessionActive = true; | |
| m_paused = false; | |
| emit sessionStarted(); | |
| }); | |
| connect(m_worker, &PS1RipWorker::emulationStopped, this, [this]() { | |
| m_startPending = false; | |
| m_sessionActive = false; | |
| m_paused = false; | |
| m_captureArmed = false; | |
| emit sessionStopped(); | |
| }); | |
| connect(m_worker, &PS1RipWorker::emulationStarted, this, [this]() { | |
| const bool wasPaused = m_paused; | |
| m_startPending = false; | |
| m_sessionActive = true; | |
| m_paused = false; | |
| if (wasPaused) | |
| emit pausedChanged(false); | |
| emit sessionStarted(); | |
| }); | |
| connect(m_worker, &PS1RipWorker::emulationStopped, this, [this]() { | |
| const bool wasPaused = m_paused; | |
| m_startPending = false; | |
| m_sessionActive = false; | |
| m_paused = false; | |
| m_captureArmed = false; | |
| if (wasPaused) | |
| emit pausedChanged(false); | |
| emit sessionStopped(); | |
| }); |
🤖 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/PS1/runtime/PS1RipManager.cpp` around lines 45 - 57, The pause state is
being cleared in the emulationStarted and emulationStopped handlers (m_paused =
false) but no signal is emitted to notify listeners; update the lambda connected
to PS1RipWorker::emulationStarted and the one connected to
PS1RipWorker::emulationStopped to emit pausedChanged(false) immediately after
clearing m_paused so UI listeners can update; reference the connect calls for
PS1RipWorker::emulationStarted and PS1RipWorker::emulationStopped and the
m_paused member and pausedChanged signal when making the change.
| stop(); | ||
|
|
||
| m_biosPath = info.absoluteFilePath(); | ||
| SentryReporter::addBreadcrumb(QStringLiteral("ps1.rip.bios.load"), m_biosPath); |
There was a problem hiding this comment.
Avoid sending absolute local file paths in breadcrumbs.
Line 107 and Line 124 currently send full paths (often including usernames) to telemetry, which is a privacy/compliance risk. Emit sanitized values (e.g., filename) and use the mandated I/O category.
Proposed fix
- SentryReporter::addBreadcrumb(QStringLiteral("ps1.rip.bios.load"), m_biosPath);
+ SentryReporter::addBreadcrumb(QStringLiteral("file.import"),
+ QFileInfo(m_biosPath).fileName());
- SentryReporter::addBreadcrumb(QStringLiteral("ps1.rip.iso.load"), m_isoPath);
+ SentryReporter::addBreadcrumb(QStringLiteral("file.import"),
+ QFileInfo(m_isoPath).fileName());Also applies to: 124-124
🤖 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/PS1/runtime/PS1RipManager.cpp` at line 107, The breadcrumb at
SentryReporter::addBreadcrumb currently sends the full absolute path
(m_biosPath), which leaks local file paths; change it to send only the filename
by extracting QFileInfo(m_biosPath).fileName() (or equivalent) and set the
category to the mandated I/O category "file.import"; do the same fix for the
other call referenced (line 124) so both SentryReporter::addBreadcrumb
invocations use the sanitized filename and category "file.import" instead of the
absolute path.
Comment on lines
+38
to
+52
| auto *biosAct = toolbar->addAction(tr("Load BIOS…")); | ||
| connect(biosAct, &QAction::triggered, this, &PS1RipSessionWindow::pickBios); | ||
| auto *isoAct = toolbar->addAction(tr("Load ISO…")); | ||
| connect(isoAct, &QAction::triggered, this, &PS1RipSessionWindow::pickIso); | ||
| toolbar->addSeparator(); | ||
| auto *startAct = toolbar->addAction(tr("Start")); | ||
| connect(startAct, &QAction::triggered, this, &PS1RipSessionWindow::onStart); | ||
| auto *stopAct = toolbar->addAction(tr("Stop")); | ||
| connect(stopAct, &QAction::triggered, this, &PS1RipSessionWindow::onStop); | ||
| auto *pauseAct = toolbar->addAction(tr("Pause")); | ||
| connect(pauseAct, &QAction::triggered, this, &PS1RipSessionWindow::onPause); | ||
| auto *stepAct = toolbar->addAction(tr("Step")); | ||
| connect(stepAct, &QAction::triggered, this, &PS1RipSessionWindow::onStep); | ||
| auto *resetAct = toolbar->addAction(tr("Reset")); | ||
| connect(resetAct, &QAction::triggered, this, &PS1RipSessionWindow::onReset); |
There was a problem hiding this comment.
Add Sentry breadcrumbs for every toolbar/file action using required categories.
Session controls and BIOS/ISO picks are user-facing actions, but only window open/close currently emit breadcrumbs, and not with the required category convention. Please emit breadcrumbs in each handler (pickBios, pickIso, onStart, onStop, onPause, onStep, onReset) with ui.action / file.import categories.
Suggested patch
@@
- SentryReporter::addBreadcrumb(QStringLiteral("ps1.rip.viewport.open"),
- QStringLiteral("PS1 rip session window opened"));
+ SentryReporter::addBreadcrumb(QStringLiteral("ui.action"),
+ QStringLiteral("PS1 rip session window opened"));
@@
- SentryReporter::addBreadcrumb(QStringLiteral("ps1.rip.viewport.close"),
- QStringLiteral("PS1 rip session window closed"));
+ SentryReporter::addBreadcrumb(QStringLiteral("ui.action"),
+ QStringLiteral("PS1 rip session window closed"));
@@
void PS1RipSessionWindow::pickBios()
{
+ SentryReporter::addBreadcrumb(QStringLiteral("file.import"),
+ QStringLiteral("PS1 BIOS picker opened"));
@@
void PS1RipSessionWindow::pickIso()
{
+ SentryReporter::addBreadcrumb(QStringLiteral("file.import"),
+ QStringLiteral("PS1 ISO picker opened"));
@@
void PS1RipSessionWindow::onStart()
{
+ SentryReporter::addBreadcrumb(QStringLiteral("ui.action"),
+ QStringLiteral("PS1 session start clicked"));
m_manager->start();
}
@@
void PS1RipSessionWindow::onStop()
{
+ SentryReporter::addBreadcrumb(QStringLiteral("ui.action"),
+ QStringLiteral("PS1 session stop clicked"));
m_manager->stop();
}
@@
void PS1RipSessionWindow::onPause()
{
+ SentryReporter::addBreadcrumb(QStringLiteral("ui.action"),
+ QStringLiteral("PS1 session pause clicked"));
m_manager->pause();
}
@@
void PS1RipSessionWindow::onStep()
{
+ SentryReporter::addBreadcrumb(QStringLiteral("ui.action"),
+ QStringLiteral("PS1 session step clicked"));
m_manager->step();
}
@@
void PS1RipSessionWindow::onReset()
{
+ SentryReporter::addBreadcrumb(QStringLiteral("ui.action"),
+ QStringLiteral("PS1 session reset clicked"));
if (!m_manager->isSessionActive() && !m_manager->isStartPending()) {As per coding guidelines: “All user-facing actions and significant operations must track Sentry breadcrumbs with category ui.action … or file.import/file.export for I/O.”
Also applies to: 75-76, 83-84, 107-170
🤖 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/PS1/runtime/PS1RipSessionWindow.cpp` around lines 38 - 52, Add Sentry
breadcrumbs in each toolbar/action handler: in pickBios and pickIso emit a
breadcrumb with category "file.import" describing the chosen file/action, and in
onStart, onStop, onPause, onStep, and onReset emit breadcrumbs with category
"ui.action" describing the control invoked; locate these methods in
PS1RipSessionWindow (pickBios, pickIso, onStart, onStop, onPause, onStep,
onReset) and call the project's Sentry breadcrumb API (or Sentry::addBreadcrumb
equivalent) at the start of each handler with a short message and appropriate
category/level to match the coding guideline.
|
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
1 participant
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.
Summary
EmuCoreabstract API,IEmuCorePlugin+EmuCoreLoader(QPluginLoader from<app>/PS1Cores/), stub pluginqtmesh_ps1core_stub(320×240 test pattern, GPL-free).EmuViewport(software blit, integer scale, FPS overlay) hosted inPS1RipSessionWindow.ps1Rip/acknowledged), BIOS/ISO pickers, transport toolbar; persisted BIOS path and recent ISOs.PS1RipWorkerruns the core on its QThread at ~60 Hz; main thread receivesframePresentedvia queued signals.Test plan
cmake -DENABLE_PS1_RIP=ON -DBUILD_TESTS=ONbuilds app +qtmesh_ps1core_stub+UnitTestsUnitTests --gtest_filter="EmuCoreLoader*:PS1RipManager*"— 7 passedCloses #415, #416, #417. Epic #412.
Made with Cursor
Summary by CodeRabbit
Release Notes
New Features
Documentation