Phase 2: UX Polish & Onboarding (#257) - #279
Merged
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:
📝 WalkthroughWalkthroughAdded four QML UI panels (Welcome Screen, Preferences, Shortcut Reference, Asset Browser), C++ controllers/singletons (WelcomeScreenController, AssetBrowserController), a MaterialPreviewRenderer, MainWindow integration (dock/overlay/actions), QSettings-backed persistence, QFileSystemWatcher-based asset updates, and tests for new controllers and renderer. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant QML as AssetBrowser QML
participant Controller as AssetBrowserController
participant Watcher as QFileSystemWatcher
participant FS as File System
participant Settings as QSettings
User->>QML: set root path / click Browse
QML->>Controller: setRootPath(newPath) / browseForDirectory()
Controller->>Settings: persist AssetBrowser/rootPath
Controller->>Watcher: setupWatcher(newPath)
Watcher->>FS: monitor directory
Controller->>Controller: refreshFiles()
FS->>Controller: return entries
Controller->>Controller: classify entries (mesh/texture/material/other)
Controller->>Controller: apply filter + search
Controller->>QML: emit filesChanged()
QML->>User: render updated file list
User->>QML: update search text
QML->>Controller: setSearchQuery(text)
Controller->>Controller: refreshFiles()
Controller->>QML: emit filesChanged()
QML->>User: display search results
User->>QML: double-click item
QML->>Controller: openFile(path)
alt path is directory
Controller->>Controller: navigateToDirectory(path)
Controller->>Settings: persist rootPath
else path is mesh file
Controller->>MainApp: importMeshRequested(path)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
QtMeshEditor/src/mainwindow.cpp
Line 1868 in 17ca856
Honor configured recent-files limit when trimming history
The new Preferences UI writes General/recentFilesCount, but recent-file retention is still hardcoded to 10 here, so user-configured values are ignored. This makes the new setting non-functional and causes unexpected truncation when users choose a larger history size.
ℹ️ 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".
| id: telemetryCheck | ||
| checked: readSetting("Telemetry/enabled", true) === true | ||
| || readSetting("Telemetry/enabled", true) === "true" | ||
| onToggled: writeSetting("Telemetry/enabled", checked) |
There was a problem hiding this comment.
This checkbox persists to Telemetry/enabled, but telemetry/crash-reporting is actually gated by Sentry/enabled (via SentryReporter::isEnabled()), so opting out here does not disable reporting. In practice, users can uncheck “Enable anonymous telemetry” and still have telemetry enabled on next launch, which is a privacy-impacting behavior mismatch.
Useful? React with 👍 / 👎.
| AssetBrowserController* AssetBrowserController::qmlInstance(QQmlEngine* engine, QJSEngine* /*scriptEngine*/) | ||
| { | ||
| auto* inst = instance(); | ||
| engine->setObjectOwnership(inst, QQmlEngine::CppOwnership); |
There was a problem hiding this comment.
This dereferences engine unconditionally; if qmlInstance(nullptr, ...) is called (as in the added AssetBrowser unit test), it will segfault and abort the test run. The existing singleton pattern elsewhere uses QQmlEngine::setObjectOwnership(...) statically and avoids this null-pointer path.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/CMakeLists.txt (1)
72-77:⚠️ Potential issue | 🟠 MajorAdd
AssetBrowserControllerto test source/header lists as well.
WelcomeScreenControllerwas added here, butAssetBrowserController(also wired into the main window flow) is missing fromTEST_SRC_FILES/TEST_HEADER_FILES. This can break test target linkage whenmainwindow.cppreferences it.🔧 Suggested patch
set(TEST_SRC_FILES @@ ${CMAKE_CURRENT_SOURCE_DIR}/../src/AIChatManager.cpp ${CMAKE_CURRENT_SOURCE_DIR}/../src/WelcomeScreenController.cpp + ${CMAKE_CURRENT_SOURCE_DIR}/../src/AssetBrowserController.cpp ${CMAKE_CURRENT_SOURCE_DIR}/../src/SubMeshTransform.cpp @@ set(TEST_HEADER_FILES @@ ${CMAKE_CURRENT_SOURCE_DIR}/../src/AIChatManager.h ${CMAKE_CURRENT_SOURCE_DIR}/../src/WelcomeScreenController.h + ${CMAKE_CURRENT_SOURCE_DIR}/../src/AssetBrowserController.h ${CMAKE_CURRENT_SOURCE_DIR}/../src/SubMeshTransform.hAlso applies to: 138-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/CMakeLists.txt` around lines 72 - 77, The test CMake lists are missing AssetBrowserController in the source/header variables, which will break linkage when mainwindow.cpp references it; add AssetBrowserController.cpp to TEST_SRC_FILES and AssetBrowserController.h to TEST_HEADER_FILES (also mirror the addition in the second set of lists where WelcomeScreenController was added) so the test target compiles and links against the controller referenced by mainwindow.cpp.
🧹 Nitpick comments (3)
qml/ShortcutReference.qml (1)
10-24: Use app theme source instead ofSystemPalettefor consistency.This dialog currently derives colors from
SystemPalette, while the rest of the new QML surfaces usePropertiesPanelControllertheme colors. Aligning sources will keep light/dark/custom palette behavior consistent across panels.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@qml/ShortcutReference.qml` around lines 10 - 24, Replace the SystemPalette usage in ShortcutReference.qml (the SystemPalette { id: palette } block and all properties that reference palette) with bindings to the shared app theme provided by PropertiesPanelController (or the existing theme object used by other panels); update property color bindings like backgroundColor, panelColor, textColor, borderColor, highlightColor, buttonColor, buttonTextColor, dimTextColor and keyBgColor to read from the controller's theme properties (e.g., theme.background, theme.panel, theme.text, theme.mid/border, theme.highlight, theme.button, theme.buttonText) and keep Qt.darker/Qt.lighter transformations for dimTextColor and keyBgColor so the dialog follows the same light/dark/custom palette behavior as other panels.qml/WelcomeScreen.qml (1)
143-186: Consider defensive access forrecentFileNames[index].If
recentFilesandrecentFileNamesbecome misaligned (e.g., due to a race between updates), accessingrecentFileNames[index]could returnundefined. The QML will handle this gracefully (showing empty text), but you might want to add a fallback.🛡️ Optional defensive fallback
Text { - text: WelcomeScreenController.recentFileNames[index] + text: WelcomeScreenController.recentFileNames[index] || "" color: PropertiesPanelController.textColor🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@qml/WelcomeScreen.qml` around lines 143 - 186, The Text element currently reads WelcomeScreenController.recentFileNames[index] directly which can be undefined if recentFiles and recentFileNames get out of sync; update the Text.text binding to defensively fall back (e.g. use a conditional/ternary or null-coalescing pattern) so that WelcomeScreenController.recentFileNames[index] resolves to an empty string when missing; locate the Repeater/Text pair using recentFiles, recentFileNames, and index to apply this change and ensure no runtime error or "undefined" is shown.src/WelcomeScreenController.h (1)
31-31: Remove or repurpose the unusedm_mainWindowmember variable.The
setMainWindow()method stores a pointer toMainWindow, butm_mainWindowis never accessed in the implementation. Either remove this member and method, or add a comment explaining future intended use.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WelcomeScreenController.h` at line 31, The member m_mainWindow stored by setMainWindow(MainWindow*) in WelcomeScreenController is never used; either remove the m_mainWindow field and the setMainWindow method from WelcomeScreenController, or keep them but add a clear comment above m_mainWindow and setMainWindow explaining their intended future use (e.g., to interact with MainWindow for navigation or event callbacks) so reviewers know it is intentionally reserved; update any header and corresponding implementation files (constructors/destructors) to remove unused storage if deleting, or add the comment and (optionally) a TODO explaining when it will be used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@qml/AssetBrowser.qml`:
- Around line 279-283: The onClicked handler currently calls
AssetBrowserController.openFile unconditionally, causing redundant calls for
directories because onDoubleClicked also navigates; update the onClicked handler
to check modelData.isDir and only call AssetBrowserController.openFile when
modelData.isDir is false (i.e., guard with if (!modelData.isDir) before invoking
AssetBrowserController.openFile), leaving the onDoubleClicked block that calls
AssetBrowserController.navigateToDirectory intact.
In `@src/AssetBrowserController.cpp`:
- Around line 56-61: The qmlInstance function should guard against a null
QQmlEngine* to avoid dereferencing nullptr; in
AssetBrowserController::qmlInstance, check if the engine parameter is non-null
before calling engine->setObjectOwnership(inst, QQmlEngine::CppOwnership) (e.g.,
if (engine) { ... }), otherwise skip setting ownership and still return the
singleton from instance().
---
Outside diff comments:
In `@tests/CMakeLists.txt`:
- Around line 72-77: The test CMake lists are missing AssetBrowserController in
the source/header variables, which will break linkage when mainwindow.cpp
references it; add AssetBrowserController.cpp to TEST_SRC_FILES and
AssetBrowserController.h to TEST_HEADER_FILES (also mirror the addition in the
second set of lists where WelcomeScreenController was added) so the test target
compiles and links against the controller referenced by mainwindow.cpp.
---
Nitpick comments:
In `@qml/ShortcutReference.qml`:
- Around line 10-24: Replace the SystemPalette usage in ShortcutReference.qml
(the SystemPalette { id: palette } block and all properties that reference
palette) with bindings to the shared app theme provided by
PropertiesPanelController (or the existing theme object used by other panels);
update property color bindings like backgroundColor, panelColor, textColor,
borderColor, highlightColor, buttonColor, buttonTextColor, dimTextColor and
keyBgColor to read from the controller's theme properties (e.g.,
theme.background, theme.panel, theme.text, theme.mid/border, theme.highlight,
theme.button, theme.buttonText) and keep Qt.darker/Qt.lighter transformations
for dimTextColor and keyBgColor so the dialog follows the same light/dark/custom
palette behavior as other panels.
In `@qml/WelcomeScreen.qml`:
- Around line 143-186: The Text element currently reads
WelcomeScreenController.recentFileNames[index] directly which can be undefined
if recentFiles and recentFileNames get out of sync; update the Text.text binding
to defensively fall back (e.g. use a conditional/ternary or null-coalescing
pattern) so that WelcomeScreenController.recentFileNames[index] resolves to an
empty string when missing; locate the Repeater/Text pair using recentFiles,
recentFileNames, and index to apply this change and ensure no runtime error or
"undefined" is shown.
In `@src/WelcomeScreenController.h`:
- Line 31: The member m_mainWindow stored by setMainWindow(MainWindow*) in
WelcomeScreenController is never used; either remove the m_mainWindow field and
the setMainWindow method from WelcomeScreenController, or keep them but add a
clear comment above m_mainWindow and setMainWindow explaining their intended
future use (e.g., to interact with MainWindow for navigation or event callbacks)
so reviewers know it is intentionally reserved; update any header and
corresponding implementation files (constructors/destructors) to remove unused
storage if deleting, or add the comment and (optionally) a TODO explaining when
it will be used.
🪄 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: d72893bd-02f5-4e31-b8f9-220bda98b460
📒 Files selected for processing (17)
qml/AssetBrowser.qmlqml/PreferencesDialog.qmlqml/ShortcutReference.qmlqml/WelcomeScreen.qmlsrc/AssetBrowserController.cppsrc/AssetBrowserController.hsrc/AssetBrowserController_test.cppsrc/CMakeLists.txtsrc/PropertiesPanelController.cppsrc/PropertiesPanelController.hsrc/WelcomeScreenController.cppsrc/WelcomeScreenController.hsrc/mainwindow.cppsrc/mainwindow.hsrc/qml_resources.qrctests/CMakeLists.txtui_files/mainwindow.ui
Comment on lines
+56
to
+61
| AssetBrowserController* AssetBrowserController::qmlInstance(QQmlEngine* engine, QJSEngine* /*scriptEngine*/) | ||
| { | ||
| auto* inst = instance(); | ||
| engine->setObjectOwnership(inst, QQmlEngine::CppOwnership); | ||
| return inst; | ||
| } |
There was a problem hiding this comment.
Add null check for engine parameter.
If qmlInstance is called with a nullptr engine (as seen in some test cases like line 33 of AssetBrowserController_test.cpp), calling engine->setObjectOwnership() will crash.
🐛 Proposed fix
AssetBrowserController* AssetBrowserController::qmlInstance(QQmlEngine* engine, QJSEngine* /*scriptEngine*/)
{
auto* inst = instance();
- engine->setObjectOwnership(inst, QQmlEngine::CppOwnership);
+ if (engine)
+ engine->setObjectOwnership(inst, QQmlEngine::CppOwnership);
return inst;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/AssetBrowserController.cpp` around lines 56 - 61, The qmlInstance
function should guard against a null QQmlEngine* to avoid dereferencing nullptr;
in AssetBrowserController::qmlInstance, check if the engine parameter is
non-null before calling engine->setObjectOwnership(inst,
QQmlEngine::CppOwnership) (e.g., if (engine) { ... }), otherwise skip setting
ownership and still return the singleton from instance().
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/AssetBrowserController.cpp (1)
59-63:⚠️ Potential issue | 🔴 CriticalGuard
enginebefore callingsetObjectOwnership().
qmlInstance()still dereferencesengineunconditionally, so any null call path crashes before the singleton is returned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AssetBrowserController.cpp` around lines 59 - 63, In qmlInstance(QQmlEngine* engine, QJSEngine*), guard against a null engine before calling engine->setObjectOwnership(): obtain the singleton via instance(), and if engine is non-null call engine->setObjectOwnership(inst, QQmlEngine::CppOwnership), otherwise skip setObjectOwnership and simply return inst; ensure you still set ownership only when engine is valid to avoid 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 `@qml/WelcomeScreen.qml`:
- Around line 99-105: The onClicked handlers for primary exit MouseAreas (e.g.,
the newSceneMA that calls WelcomeScreenController.newScene(), and the similar
handlers for Open File and Recent File) currently skip calling dismiss(...), so
the “Don’t show again” preference isn’t persisted; update each of those
onClicked handlers to call dismiss(...) first (or await/chain it if it returns a
Promise) and then invoke the existing action (e.g.,
WelcomeScreenController.newScene(), the open file handler, or recent-file
handler) so the preference is saved before navigating away. Ensure you modify
the MouseArea onClicked blocks that surround calls to
WelcomeScreenController.newScene() and the Open/Recent file handlers to include
dismiss(...) with the same parameters used by overlay clicks/Escape.
- Around line 22-23: Clamp the computed width and height to non-negative values
before applying Math.min to avoid negative sizes; specifically, for the width
expression using parent.width - 60 and the height expression using
cardLayout.implicitHeight + 48 and parent.height - 40, wrap the subtractions
with Math.max(..., 0) (e.g. ensure parent.width - 60 is clamped to >=0 and
parent.height - 40 is clamped to >=0) and then take the Math.min with the
existing caps (560 for width and parent-height cap for height) so the properties
width and height (and the references to cardLayout.implicitHeight and parent)
never get a negative input.
In `@src/AssetBrowserController.cpp`:
- Line 88: Replace raw "asset_browser" breadcrumbs with the repo-standard
categories and avoid emitting full absolute paths: change calls to
SentryReporter::addBreadcrumb to use "ui.action" for navigation/browse events
(and "file.import"/"file.export" where appropriate) and redact or convert
m_rootPath to a basename or token (e.g., use
std::filesystem::path(m_rootPath).filename().string() or a short redaction
function) before passing it to the message; update the occurrences around
SentryReporter::addBreadcrumb(...) at the Root directory change (m_rootPath) and
the similar calls in the block around lines 134-154 to follow this pattern.
- Around line 151-157: The classification uses QFileInfo::suffix() which only
returns the last segment (breaking names like model.mesh.xml); update the three
call sites that pass fi.suffix().toLower() into classifyExtension — specifically
in openFile, fileTypeForPath, and refreshFiles — to use
fi.completeSuffix().toLower() instead so multi-part suffixes (e.g., "mesh.xml")
are recognized; keep the toLower() and existing call to classifyExtension
unchanged.
---
Duplicate comments:
In `@src/AssetBrowserController.cpp`:
- Around line 59-63: In qmlInstance(QQmlEngine* engine, QJSEngine*), guard
against a null engine before calling engine->setObjectOwnership(): obtain the
singleton via instance(), and if engine is non-null call
engine->setObjectOwnership(inst, QQmlEngine::CppOwnership), otherwise skip
setObjectOwnership and simply return inst; ensure you still set ownership only
when engine is valid to avoid 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8143254f-c544-4bc5-abb2-58167b6f8ebc
📒 Files selected for processing (3)
qml/AssetBrowser.qmlqml/WelcomeScreen.qmlsrc/AssetBrowserController.cpp
✅ Files skipped from review due to trivial changes (1)
- qml/AssetBrowser.qml
Comment on lines
+99
to
+105
| MouseArea { | ||
| id: newSceneMA | ||
| anchors.fill: parent | ||
| hoverEnabled: true | ||
| cursorShape: Qt.PointingHandCursor | ||
| onClicked: WelcomeScreenController.newScene() | ||
| } |
There was a problem hiding this comment.
Persist “Don’t show again” for all primary exit actions
When users check “Don’t show again” and then click New Scene, Open File..., or a Recent File, that preference is not saved because these handlers skip dismiss(...) (which is where persistence happens). This creates inconsistent behavior vs. overlay click/Get Started/Escape.
Proposed fix
MouseArea {
id: newSceneMA
anchors.fill: parent
hoverEnabled: true
cursorShape: Qt.PointingHandCursor
- onClicked: WelcomeScreenController.newScene()
+ onClicked: {
+ WelcomeScreenController.dismiss(dontShowCheckbox.checked)
+ WelcomeScreenController.newScene()
+ }
}
@@
MouseArea {
id: openFileMA
anchors.fill: parent
hoverEnabled: true
cursorShape: Qt.PointingHandCursor
- onClicked: WelcomeScreenController.openFileDialog()
+ onClicked: {
+ WelcomeScreenController.dismiss(dontShowCheckbox.checked)
+ WelcomeScreenController.openFileDialog()
+ }
}
@@
MouseArea {
id: recentMA
anchors.fill: parent
hoverEnabled: true
cursorShape: Qt.PointingHandCursor
- onClicked: WelcomeScreenController.openFile(modelData)
+ onClicked: {
+ WelcomeScreenController.dismiss(dontShowCheckbox.checked)
+ WelcomeScreenController.openFile(modelData)
+ }
}Also applies to: 125-131, 184-190
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@qml/WelcomeScreen.qml` around lines 99 - 105, The onClicked handlers for
primary exit MouseAreas (e.g., the newSceneMA that calls
WelcomeScreenController.newScene(), and the similar handlers for Open File and
Recent File) currently skip calling dismiss(...), so the “Don’t show again”
preference isn’t persisted; update each of those onClicked handlers to call
dismiss(...) first (or await/chain it if it returns a Promise) and then invoke
the existing action (e.g., WelcomeScreenController.newScene(), the open file
handler, or recent-file handler) so the preference is saved before navigating
away. Ensure you modify the MouseArea onClicked blocks that surround calls to
WelcomeScreenController.newScene() and the Open/Recent file handlers to include
dismiss(...) with the same parameters used by overlay clicks/Escape.
|
|
||
| m_rootPath = dir.absolutePath(); | ||
|
|
||
| SentryReporter::addBreadcrumb("asset_browser", "Root directory changed: " + m_rootPath); |
There was a problem hiding this comment.
Use standard breadcrumb categories and avoid raw local paths.
These breadcrumbs are all emitted as "asset_browser", which makes them harder to query consistently, and Line 88 sends the full absolute path to Sentry. Prefer the repo-standard categories (ui.action for browse/navigation, file.import for mesh open) and log only a basename or redacted identifier.
As per coding guidelines, "All user-facing actions and significant operations must be tracked with SentryReporter::addBreadcrumb(category, message) using categories like 'ui.action' for toolbar/menu clicks, 'ai.tool_call' for MCP tool invocations, 'file.import'/'file.export' for I/O operations".
Also applies to: 134-154
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/AssetBrowserController.cpp` at line 88, Replace raw "asset_browser"
breadcrumbs with the repo-standard categories and avoid emitting full absolute
paths: change calls to SentryReporter::addBreadcrumb to use "ui.action" for
navigation/browse events (and "file.import"/"file.export" where appropriate) and
redact or convert m_rootPath to a basename or token (e.g., use
std::filesystem::path(m_rootPath).filename().string() or a short redaction
function) before passing it to the message; update the occurrences around
SentryReporter::addBreadcrumb(...) at the Root directory change (m_rootPath) and
the similar calls in the block around lines 134-154 to follow this pattern.
Comment on lines
+151
to
+157
| QString type = classifyExtension(fi.suffix().toLower()); | ||
|
|
||
| SentryReporter::addBreadcrumb("asset_browser", | ||
| QString("Open file: %1 (type: %2)").arg(fi.fileName(), type)); | ||
|
|
||
| if (type == "mesh") { | ||
| emit importMeshRequested(QStringList{path}); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Qt 6, what does QFileInfo::suffix() return versus QFileInfo::completeSuffix() for a filename like "model.mesh.xml"?
💡 Result:
For the filename "model.mesh.xml" in Qt 6: - QFileInfo::suffix returns "xml" (all characters after the last '.'). - QFileInfo::completeSuffix returns "mesh.xml" (all characters after the first '.'). This behavior is consistent across Qt versions, including Qt 6, as documented in the official Qt documentation. For example, with "archive.tar.gz": - suffix returns "gz" - completeSuffix returns "tar.gz"
Citations:
🏁 Script executed:
# First, find and examine the file structure
fd "AssetBrowserController" --type fRepository: fernandotonon/QtMeshEditor
Length of output: 165
🏁 Script executed:
# Check the file size first
wc -l src/AssetBrowserController.cpp
# Then examine the three specific locations mentioned in the review
echo "=== Lines 151-157 ==="
sed -n '151,157p' src/AssetBrowserController.cpp
echo "=== Lines 175-180 ==="
sed -n '175,180p' src/AssetBrowserController.cpp
echo "=== Lines 198-204 ==="
sed -n '198,204p' src/AssetBrowserController.cppRepository: fernandotonon/QtMeshEditor
Length of output: 846
🏁 Script executed:
# Find classifyExtension function and s_meshExtensions definition
echo "=== Search for classifyExtension implementation ==="
rg "classifyExtension" src/AssetBrowserController.cpp -A 10
echo "=== Search for s_meshExtensions ==="
rg "s_meshExtensions" src/AssetBrowserController.cpp -B 2 -A 5Repository: fernandotonon/QtMeshEditor
Length of output: 2283
🏁 Script executed:
# Check if there are any other uses of .suffix() in the file that might need attention
rg "\.suffix\(\)" src/AssetBrowserController.cpp -n
# Also check the header file for any relevant method signatures
cat -n src/AssetBrowserController.h | grep -A 5 "classifyExtension\|fileTypeForPath"Repository: fernandotonon/QtMeshEditor
Length of output: 710
Use completeSuffix() so .mesh.xml assets stay importable.
QFileInfo::suffix() only returns the last segment (xml), so files like model.mesh.xml are classified as other even though s_meshExtensions explicitly lists mesh.xml. This breaks mesh filtering and the import workflow.
The issue occurs in three places:
- Line 151 (openFile)
- Line 180 (fileTypeForPath)
- Line 203 (refreshFiles)
Change all three to use completeSuffix() instead:
Proposed fix
- QString type = classifyExtension(fi.suffix().toLower());
+ QString type = classifyExtension(fi.completeSuffix().toLower());
...
- return classifyExtension(fi.suffix().toLower());
+ return classifyExtension(fi.completeSuffix().toLower());
...
- type = classifyExtension(fi.suffix().toLower());
+ type = classifyExtension(fi.completeSuffix().toLower());📝 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
| QString type = classifyExtension(fi.suffix().toLower()); | |
| SentryReporter::addBreadcrumb("asset_browser", | |
| QString("Open file: %1 (type: %2)").arg(fi.fileName(), type)); | |
| if (type == "mesh") { | |
| emit importMeshRequested(QStringList{path}); | |
| QString type = classifyExtension(fi.completeSuffix().toLower()); | |
| SentryReporter::addBreadcrumb("asset_browser", | |
| QString("Open file: %1 (type: %2)").arg(fi.fileName(), type)); | |
| if (type == "mesh") { | |
| emit importMeshRequested(QStringList{path}); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/AssetBrowserController.cpp` around lines 151 - 157, The classification
uses QFileInfo::suffix() which only returns the last segment (breaking names
like model.mesh.xml); update the three call sites that pass
fi.suffix().toLower() into classifyExtension — specifically in openFile,
fileTypeForPath, and refreshFiles — to use fi.completeSuffix().toLower() instead
so multi-part suffixes (e.g., "mesh.xml") are recognized; keep the toLower() and
existing call to classifyExtension unchanged.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
src/AssetBrowserController.cpp (3)
91-91:⚠️ Potential issue | 🟠 MajorUse standard breadcrumb categories and avoid full local paths.
These breadcrumbs are emitted as
"asset_browser", and Line 91 sends the absolute directory path. That makes Sentry queries inconsistent and leaks more filesystem detail than needed. Useui.actionfor browse/navigation,file.importwhenimportMeshRequestedis emitted, and log only a basename or redacted identifier.As per coding guidelines, "All user-facing actions and significant operations must be tracked with SentryReporter::addBreadcrumb(category, message) using categories like 'ui.action' for toolbar/menu clicks, 'ai.tool_call' for MCP tool invocations, 'file.import'/'file.export' for I/O operations".
Also applies to: 137-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AssetBrowserController.cpp` at line 91, Replace direct breadcrumb uses that emit absolute paths with standard categories and redacted identifiers: change calls to SentryReporter::addBreadcrumb(...) that currently use "asset_browser" and full m_rootPath (and similar calls in the importMeshRequested handlers between lines 137-157) to use "ui.action" for navigation/root-change events and "file.import" for import events, and pass only a basename or sanitized identifier (e.g., std::filesystem::path(m_rootPath).filename() or a redacted token) in the message instead of the full absolute path; update SentryReporter::addBreadcrumb invocations in the AssetBrowserController methods where root changes and mesh-import requests are reported to follow this convention.
154-154:⚠️ Potential issue | 🟠 MajorUse
completeSuffix()so.mesh.xmlassets stay importable.
QFileInfo::suffix()only returns the last segment, somodel.mesh.xmlis classified asxmleven thoughs_meshExtensionsexplicitly includesmesh.xml. That breaks filtering and mesh import for those files.Suggested fix
- QString type = classifyExtension(fi.suffix().toLower()); + QString type = classifyExtension(fi.completeSuffix().toLower()); ... - return classifyExtension(fi.suffix().toLower()); + return classifyExtension(fi.completeSuffix().toLower()); ... - type = classifyExtension(fi.suffix().toLower()); + type = classifyExtension(fi.completeSuffix().toLower());In Qt, what does QFileInfo::suffix() return versus QFileInfo::completeSuffix() for a filename like "model.mesh.xml"?Also applies to: 183-183, 206-206
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AssetBrowserController.cpp` at line 154, The code uses QFileInfo::suffix() when classifying extensions which only returns the last suffix (so "model.mesh.xml" -> "xml"); replace calls to fi.suffix().toLower() with fi.completeSuffix().toLower() in classifyExtension call sites (e.g., the call in AssetBrowserController where QString type = classifyExtension(fi.suffix().toLower())) and the other occurrences noted (the similar usages around the other two locations) so multi-part extensions like "mesh.xml" are matched against s_meshExtensions and imported correctly.
62-66:⚠️ Potential issue | 🔴 CriticalNull-check
enginebefore setting ownership.Line 65 dereferences
engineunconditionally. If the singleton provider is invoked with a nullQQmlEngine*, this turns registration into a crash instead of a no-op.Suggested fix
AssetBrowserController* AssetBrowserController::qmlInstance(QQmlEngine* engine, QJSEngine* /*scriptEngine*/) { auto* inst = instance(); - engine->setObjectOwnership(inst, QQmlEngine::CppOwnership); + if (engine) + engine->setObjectOwnership(inst, QQmlEngine::CppOwnership); return inst; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AssetBrowserController.cpp` around lines 62 - 66, AssetBrowserController::qmlInstance currently dereferences the QQmlEngine* parameter unconditionally; add a null check for engine before calling engine->setObjectOwnership to avoid crashes when qmlInstance is invoked with a null pointer. In the qmlInstance function (which calls instance() and engine->setObjectOwnership), early-return the singleton pointer if engine is null (or skip setObjectOwnership) so instance() is still returned safely without calling setObjectOwnership on a null engine.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@qml/PropertiesPanel.qml`:
- Around line 859-864: The GridView (id: presetGrid) computes height and
cellWidth by dividing by Math.floor(width / 72), which can be zero when width <
72; compute a clamped column count first (e.g. columns = Math.max(1,
Math.floor(width / 72))) and then use that columns variable in the height and
cellWidth calculations (replace Math.floor(width / 72) occurrences with columns)
so height and cellWidth never divide by zero and columns is at least 1.
In `@src/AssetBrowserController.cpp`:
- Around line 272-299: The code currently skips reparsing .material scripts when
matMgr->resourceExists(matName) is true, causing stale previews; change the
logic to detect file changes (e.g., compare QFileInfo(filePath).lastModified()
or use file inode/mtime) and when the file has been modified or the same
material name is used from a different path, either force reload/remove the
existing Ogre resource via Ogre::MaterialManager::getSingleton().remove(matName)
or unload it before reparsing, or alternatively derive a unique matName per file
(e.g., append file mtime or a hash) so Ogre::ScriptCompilerManager::parseScript
is invoked for updated files; ensure you still register the file path with
Ogre::ResourceGroupManager::addResourceLocation and then call
MaterialPreviewRenderer::instance()->renderPreviewAsDataUri(updatedMatName)
after reload.
In `@src/PropertiesPanelController.cpp`:
- Around line 738-744: The preview generation is mutating the live selection
because PropertiesPanelController.cpp calls
MaterialPresetLibrary::applyPreset(presetName), which both creates the material
and applies it to selection; change this to use a non-mutating creation path
instead. Modify MaterialPresetLibrary to expose a creation-only API (e.g.,
createMaterialFromPreset(presetName) or add an optional flag to applyPreset like
applyPreset(presetName, bool applyToSelection=false)), update
PropertiesPanelController::materialPresetPreview (or the preview call site) to
call the creation-only method so the material is created without altering the
current selection.
---
Duplicate comments:
In `@src/AssetBrowserController.cpp`:
- Line 91: Replace direct breadcrumb uses that emit absolute paths with standard
categories and redacted identifiers: change calls to
SentryReporter::addBreadcrumb(...) that currently use "asset_browser" and full
m_rootPath (and similar calls in the importMeshRequested handlers between lines
137-157) to use "ui.action" for navigation/root-change events and "file.import"
for import events, and pass only a basename or sanitized identifier (e.g.,
std::filesystem::path(m_rootPath).filename() or a redacted token) in the message
instead of the full absolute path; update SentryReporter::addBreadcrumb
invocations in the AssetBrowserController methods where root changes and
mesh-import requests are reported to follow this convention.
- Line 154: The code uses QFileInfo::suffix() when classifying extensions which
only returns the last suffix (so "model.mesh.xml" -> "xml"); replace calls to
fi.suffix().toLower() with fi.completeSuffix().toLower() in classifyExtension
call sites (e.g., the call in AssetBrowserController where QString type =
classifyExtension(fi.suffix().toLower())) and the other occurrences noted (the
similar usages around the other two locations) so multi-part extensions like
"mesh.xml" are matched against s_meshExtensions and imported correctly.
- Around line 62-66: AssetBrowserController::qmlInstance currently dereferences
the QQmlEngine* parameter unconditionally; add a null check for engine before
calling engine->setObjectOwnership to avoid crashes when qmlInstance is invoked
with a null pointer. In the qmlInstance function (which calls instance() and
engine->setObjectOwnership), early-return the singleton pointer if engine is
null (or skip setObjectOwnership) so instance() is still returned safely without
calling setObjectOwnership on a null engine.
🪄 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: 2011d130-d8a4-4093-bd77-0a89c27040e3
📒 Files selected for processing (11)
qml/AssetBrowser.qmlqml/PropertiesPanel.qmlsrc/AssetBrowserController.cppsrc/AssetBrowserController.hsrc/CMakeLists.txtsrc/MaterialPreviewRenderer.cppsrc/MaterialPreviewRenderer.hsrc/MaterialPreviewRenderer_test.cppsrc/PropertiesPanelController.cppsrc/PropertiesPanelController.hsrc/mainwindow.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- src/PropertiesPanelController.h
- src/CMakeLists.txt
- src/mainwindow.cpp
Comment on lines
+272
to
+299
| if (!matMgr->resourceExists(matName.toStdString())) { | ||
| QFileInfo fi(filePath); | ||
| try { | ||
| // Register the material's directory so Ogre can find referenced textures | ||
| Ogre::ResourceGroupManager::getSingleton().addResourceLocation( | ||
| fi.absolutePath().toStdString(), "FileSystem", | ||
| Ogre::ResourceGroupManager::DEFAULT_RESOURCE_GROUP_NAME, false); | ||
| } catch (...) {} | ||
|
|
||
| // Parse the .material script via Ogre's script compiler | ||
| try { | ||
| std::string scriptContent; | ||
| QFile f(filePath); | ||
| if (f.open(QIODevice::ReadOnly | QIODevice::Text)) { | ||
| scriptContent = f.readAll().toStdString(); | ||
| f.close(); | ||
| } | ||
| if (!scriptContent.empty()) { | ||
| Ogre::DataStreamPtr ds(new Ogre::MemoryDataStream( | ||
| const_cast<char*>(scriptContent.c_str()), | ||
| scriptContent.size(), false)); | ||
| Ogre::ScriptCompilerManager::getSingleton().parseScript( | ||
| ds, Ogre::ResourceGroupManager::DEFAULT_RESOURCE_GROUP_NAME); | ||
| } | ||
| } catch (...) {} | ||
| } | ||
|
|
||
| return MaterialPreviewRenderer::instance()->renderPreviewAsDataUri(matName); |
There was a problem hiding this comment.
Material previews go stale after the first successful load.
After the first successful preview, later refreshFiles() passes reuse the old Ogre material because this branch skips reparsing as soon as resourceExists(matName) is true. Updated .material files—or a different directory that reuses the same material name—will keep showing the old thumbnail until restart. Please invalidate/reload the material and preview cache when the source file changes, or key previews by file identity/mtime instead of just material name.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/AssetBrowserController.cpp` around lines 272 - 299, The code currently
skips reparsing .material scripts when matMgr->resourceExists(matName) is true,
causing stale previews; change the logic to detect file changes (e.g., compare
QFileInfo(filePath).lastModified() or use file inode/mtime) and when the file
has been modified or the same material name is used from a different path,
either force reload/remove the existing Ogre resource via
Ogre::MaterialManager::getSingleton().remove(matName) or unload it before
reparsing, or alternatively derive a unique matName per file (e.g., append file
mtime or a hash) so Ogre::ScriptCompilerManager::parseScript is invoked for
updated files; ensure you still register the file path with
Ogre::ResourceGroupManager::addResourceLocation and then call
MaterialPreviewRenderer::instance()->renderPreviewAsDataUri(updatedMatName)
after reload.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
src/WelcomeDialog.cpp (1)
23-147: Consider keeping this onboarding surface in QML.This adds a new QWidget-based welcome UI alongside the QML onboarding work in the same PR, which increases duplication and makes styling behavior harder to keep consistent.
Based on learnings: "New UI should be built in QML (Qt Quick), not Qt Widgets."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WelcomeDialog.cpp` around lines 23 - 147, This PR adds a QWidget-based WelcomeDialog (class WelcomeDialog and its constructor WelcomeDialog::WelcomeDialog) that duplicates the QML onboarding surface; remove this Widget implementation and its usage (delete WelcomeDialog.cpp and corresponding header and any instantiation) and instead implement the onboarding in QML or wire the onboarding logic into the existing QML component; preserve the behaviors currently implemented here (RecentFiles handling via QSettings, actions NewScene/OpenFile/OpenRecent/Dismissed, selection flow and SentryReporter breadcrumbs used in the newSceneBtn/openFileBtn/recentList/getStartedBtn handlers) by exposing equivalent hooks (signals/properties or a small C++ backend) to the QML onboarding so styling and single-source-of-truth remain in Qt Quick.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@qml/MaterialListModal.qml`:
- Around line 149-156: MaterialEditorQML.materialPreview(modelData) is doing
synchronous render+encode and is bound directly to the delegate Image.source,
causing UI freezes during delegate creation/recycle (scroll/filter/open); change
the implementation so the delegate no longer calls materialPreview
synchronously: instead cache generated preview URLs per material or expose
previews via an async image provider and update the delegate Image.source
asynchronously (i.e., keep Image.source set to a cached/url provider value and
have MaterialEditorQML expose an async getPreview or image provider that
populates the cache and emits signals when ready), updating code paths that
reference MaterialEditorQML.materialPreview and the delegate Image binding to
use the async/cached preview API.
In `@src/main.cpp`:
- Around line 248-259: The WelcomeDialog choice is recorded but not applied
before MainWindow triggers constructor-time/positional file imports (e.g.,
MainWindow::initToolBar), causing New Scene or OpenRecent selections to be
ignored or duplicated; to fix, ensure the dialog decision is resolved before
MainWindow construction or defer import/queueing until MainWindow startup:
either move the WelcomeDialog invocation into MainWindow startup logic and apply
welcome.userAction()/welcome.selectedFile() there, or detect positional startup
files and skip showing the dialog in main() so constructor-time imports
(initToolBar and any positional-file queueing) honor the user's choice; update
both occurrences referenced (including the similar block at the other reported
lines).
In `@src/mainwindow.cpp`:
- Around line 452-456: The Asset Browser import handler currently calls
importMeshs(paths) directly and skips recent-files bookkeeping; update the
lambda connected to AssetBrowserController::importMeshRequested (the
abController callback) to call addToRecentFiles(...) for each path (or otherwise
add all paths to recent files) before invoking importMeshs(paths), then keep the
SentryReporter breadcrumb; this preserves the same recent-files behavior as the
other open/import flows.
In `@src/MaterialPreviewRenderer.cpp`:
- Around line 178-198: The cache in
MaterialPreviewRenderer::renderPreviewAsDataUri is keyed only by materialName
causing stale previews; fix by invalidating or augmenting the key: either (A)
subscribe to the material change/reload signal (e.g., connect to a
Material::changed or project reload signal) and call
m_cache.remove(materialName) inside that handler so updated materials clear
their cached entry, or (B) compute and use a version/fingerprint when caching
(e.g., fetch material version or compute a content checksum via renderPreview or
a provided Material::version()/hash()) and store/lookup m_cache using
materialName + ":" + version so renderPreviewAsDataUri returns a new data URI
whenever the material content changes. Ensure updates touch m_cache and that
renderPreview is still used when no valid cache entry exists.
- Around line 44-61: The destructor currently only cleans up when m_initialized
is true, which leaves partially-created Ogre objects leaking if ensureScene()
throws; modify the error path in ensureScene() to explicitly teardown any
partial state (destroy m_sceneMgr via
Ogre::Root::getSingletonPtr()->destroySceneManager, remove m_rttTexture via
Ogre::TextureManager::getSingleton().remove and reset the smart pointer, and set
m_sceneMgr = nullptr) before returning false, or alternatively change
MaterialPreviewRenderer::~MaterialPreviewRenderer to perform the same cleanup
unconditionally (remove the m_initialized guard) so both the catch path and
destructor will remove any lingering m_rttTexture and destroy m_sceneMgr;
reference members m_rttTexture, m_sceneMgr, m_initialized and the method
ensureScene()/MaterialPreviewRenderer::~MaterialPreviewRenderer when making the
change.
- Around line 141-198: Add Sentry breadcrumbs around preview generation and
material parsing: in MaterialPreviewRenderer::renderPreview, call
SentryReporter::addBreadcrumb("ui.action", QString("renderPreview request:
%1").arg(materialName)) at the start, and add success/failure breadcrumbs before
each early return and inside each catch (e.g., "renderPreview success: %1" and
"renderPreview error: %1") referencing materialName; likewise in
MaterialPreviewRenderer::renderPreviewAsDataUri add a breadcrumb when serving
from cache and when generating a new preview (e.g., "renderPreviewAsDataUri
cache_hit: %1" and "renderPreviewAsDataUri generate: %1"); also apply the same
pattern around the .material parsing routine referenced in the diff (add
breadcrumbs for "file.import" at parse start, parse success, and parse failure)
so failures and requests are recorded—use the existing
SentryReporter::addBreadcrumb(category, message) API and the existing member
symbols (renderPreview, renderPreviewAsDataUri, m_cache, m_sphere,
m_renderTarget) to locate insertion points.
In `@src/WelcomeDialog.cpp`:
- Around line 94-110: The recent-files list only handles mouse double-clicks via
the connect call using QListWidget::itemDoubleClicked, making it inaccessible to
keyboard/assistive activation; add an additional connection for
QListWidget::itemActivated (or connect both signals to the same lambda) on
recentList so Enter/keyboard activation performs the same actions (set m_action
= OpenRecent, set m_selectedFile from item->data(Qt::UserRole), call
SentryReporter::addBreadcrumb("ui.action","Welcome: Open Recent") and accept()).
Ensure the new connection references the same lambda or extracts the shared
logic so both itemDoubleClicked and itemActivated trigger identical behavior.
- Around line 59-77: The dialog only saves the "WelcomeScreen/dontShowAgain"
preference in the Get Started path; persist it on every exit path by
centralizing the write where the dialog is closed. Update the New Scene lambda
(connected to newSceneBtn), the Open File lambda (openFileBtn), the Open Recent
handlers, and the dialog close/reject path to check the same checkbox state
(e.g., m_dontShowAgain or the checkbox widget) and write the setting
"WelcomeScreen/dontShowAgain" before calling accept()/reject(); alternatively,
implement a single helper (e.g., saveDontShowAgain()) and call it from each
lambda and from the dialog's rejectEvent/closeEvent or
WelcomeDialog::accept()/reject() so the preference is saved regardless of how
the dialog exits.
---
Nitpick comments:
In `@src/WelcomeDialog.cpp`:
- Around line 23-147: This PR adds a QWidget-based WelcomeDialog (class
WelcomeDialog and its constructor WelcomeDialog::WelcomeDialog) that duplicates
the QML onboarding surface; remove this Widget implementation and its usage
(delete WelcomeDialog.cpp and corresponding header and any instantiation) and
instead implement the onboarding in QML or wire the onboarding logic into the
existing QML component; preserve the behaviors currently implemented here
(RecentFiles handling via QSettings, actions
NewScene/OpenFile/OpenRecent/Dismissed, selection flow and SentryReporter
breadcrumbs used in the newSceneBtn/openFileBtn/recentList/getStartedBtn
handlers) by exposing equivalent hooks (signals/properties or a small C++
backend) to the QML onboarding so styling and single-source-of-truth remain in
Qt Quick.
🪄 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: fb73b9a4-270c-4a46-bfee-8370b129d6d8
📒 Files selected for processing (13)
qml/MaterialListModal.qmlsrc/CMakeLists.txtsrc/MaterialEditorQML.cppsrc/MaterialEditorQML.hsrc/MaterialPreviewRenderer.cppsrc/PropertiesPanelController.cppsrc/PropertiesPanelController.hsrc/WelcomeDialog.cppsrc/WelcomeDialog.hsrc/main.cppsrc/mainwindow.cppsrc/mainwindow.htests/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/CMakeLists.txt
- src/CMakeLists.txt
- src/PropertiesPanelController.cpp
- src/mainwindow.h
Comment on lines
+149
to
+156
| Image { | ||
| anchors.horizontalCenter: parent.horizontalCenter | ||
| width: 52; height: 52 | ||
| source: MaterialEditorQML.materialPreview(modelData) | ||
| fillMode: Image.PreserveAspectFit | ||
| asynchronous: true | ||
| sourceSize.width: 52 | ||
| sourceSize.height: 52 |
There was a problem hiding this comment.
Don’t generate RTT previews directly from the delegate binding.
MaterialEditorQML.materialPreview(modelData) does synchronous render + encode work, and binding it straight to Image.source means every delegate creation/recycle during open, filter, or scroll can block the UI thread. Cache preview URLs per material, or expose them through an async image provider instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@qml/MaterialListModal.qml` around lines 149 - 156,
MaterialEditorQML.materialPreview(modelData) is doing synchronous render+encode
and is bound directly to the delegate Image.source, causing UI freezes during
delegate creation/recycle (scroll/filter/open); change the implementation so the
delegate no longer calls materialPreview synchronously: instead cache generated
preview URLs per material or expose previews via an async image provider and
update the delegate Image.source asynchronously (i.e., keep Image.source set to
a cached/url provider value and have MaterialEditorQML expose an async
getPreview or image provider that populates the cache and emits signals when
ready), updating code paths that reference MaterialEditorQML.materialPreview and
the delegate Image binding to use the async/cached preview API.
Comment on lines
+452
to
+456
| auto* abController = AssetBrowserController::instance(); | ||
| connect(abController, &AssetBrowserController::importMeshRequested, this, [this](const QStringList& paths) { | ||
| SentryReporter::addBreadcrumb("asset_browser", "Importing mesh from Asset Browser"); | ||
| importMeshs(paths); | ||
| }); |
There was a problem hiding this comment.
Asset Browser imports skip recent-files bookkeeping.
This path goes straight to importMeshs(paths), while the other open/import flows call addToRecentFiles(...) first. Meshes opened from the new browser therefore won't show up in File > Recent Files or in the welcome screen list.
🧩 Suggested fix
auto* abController = AssetBrowserController::instance();
connect(abController, &AssetBrowserController::importMeshRequested, this, [this](const QStringList& paths) {
SentryReporter::addBreadcrumb("asset_browser", "Importing mesh from Asset Browser");
+ for (const QString& path : paths)
+ addToRecentFiles(path);
importMeshs(paths);
});📝 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
| auto* abController = AssetBrowserController::instance(); | |
| connect(abController, &AssetBrowserController::importMeshRequested, this, [this](const QStringList& paths) { | |
| SentryReporter::addBreadcrumb("asset_browser", "Importing mesh from Asset Browser"); | |
| importMeshs(paths); | |
| }); | |
| auto* abController = AssetBrowserController::instance(); | |
| connect(abController, &AssetBrowserController::importMeshRequested, this, [this](const QStringList& paths) { | |
| SentryReporter::addBreadcrumb("asset_browser", "Importing mesh from Asset Browser"); | |
| for (const QString& path : paths) | |
| addToRecentFiles(path); | |
| importMeshs(paths); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mainwindow.cpp` around lines 452 - 456, The Asset Browser import handler
currently calls importMeshs(paths) directly and skips recent-files bookkeeping;
update the lambda connected to AssetBrowserController::importMeshRequested (the
abController callback) to call addToRecentFiles(...) for each path (or otherwise
add all paths to recent files) before invoking importMeshs(paths), then keep the
SentryReporter breadcrumb; this preserves the same recent-files behavior as the
other open/import flows.
Comment on lines
+44
to
+61
| MaterialPreviewRenderer::~MaterialPreviewRenderer() | ||
| { | ||
| if (m_initialized) { | ||
| auto* root = Ogre::Root::getSingletonPtr(); | ||
| if (root) { | ||
| // Remove the render texture first | ||
| if (m_rttTexture) { | ||
| Ogre::TextureManager::getSingleton().remove(m_rttTexture); | ||
| m_rttTexture.reset(); | ||
| } | ||
|
|
||
| // Destroy the preview scene manager (cleans up all its nodes/entities/lights) | ||
| if (m_sceneMgr) { | ||
| root->destroySceneManager(m_sceneMgr); | ||
| m_sceneMgr = nullptr; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Clean up partial Ogre state when ensureScene() fails.
If any step after Line 75 or Line 117 throws, the catch just returns false while m_initialized stays false. That skips the destructor cleanup path, leaks the named scene manager/RTT texture, and makes later retries prone to duplicate-name failures. Tear down partially created Ogre objects in the catch path, or make the destructor clean up regardless of m_initialized.
Also applies to: 73-138
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MaterialPreviewRenderer.cpp` around lines 44 - 61, The destructor
currently only cleans up when m_initialized is true, which leaves
partially-created Ogre objects leaking if ensureScene() throws; modify the error
path in ensureScene() to explicitly teardown any partial state (destroy
m_sceneMgr via Ogre::Root::getSingletonPtr()->destroySceneManager, remove
m_rttTexture via Ogre::TextureManager::getSingleton().remove and reset the smart
pointer, and set m_sceneMgr = nullptr) before returning false, or alternatively
change MaterialPreviewRenderer::~MaterialPreviewRenderer to perform the same
cleanup unconditionally (remove the m_initialized guard) so both the catch path
and destructor will remove any lingering m_rttTexture and destroy m_sceneMgr;
reference members m_rttTexture, m_sceneMgr, m_initialized and the method
ensureScene()/MaterialPreviewRenderer::~MaterialPreviewRenderer when making the
change.
Comment on lines
+141
to
+198
| QImage MaterialPreviewRenderer::renderPreview(const QString& materialName) | ||
| { | ||
| if (!ensureScene()) | ||
| return {}; | ||
|
|
||
| // Check that the material exists | ||
| auto* matMgr = Ogre::MaterialManager::getSingletonPtr(); | ||
| if (!matMgr) | ||
| return {}; | ||
|
|
||
| std::string stdName = materialName.toStdString(); | ||
| if (!matMgr->resourceExists(stdName)) | ||
| return {}; | ||
|
|
||
| try { | ||
| // Apply the material to the sphere | ||
| m_sphere->setMaterialName(stdName); | ||
|
|
||
| // Render | ||
| m_renderTarget->update(); | ||
|
|
||
| // Read pixels from the render target | ||
| QImage image(PREVIEW_SIZE, PREVIEW_SIZE, QImage::Format_RGBA8888); | ||
|
|
||
| Ogre::PixelBox pb(PREVIEW_SIZE, PREVIEW_SIZE, 1, Ogre::PF_BYTE_RGBA, image.bits()); | ||
| m_renderTarget->copyContentsToMemory( | ||
| Ogre::Box(0, 0, PREVIEW_SIZE, PREVIEW_SIZE), pb, | ||
| Ogre::RenderTarget::FB_AUTO); | ||
|
|
||
| return image; | ||
| } catch (const Ogre::Exception&) { | ||
| return {}; | ||
| } catch (...) { | ||
| return {}; | ||
| } | ||
| } | ||
|
|
||
| QString MaterialPreviewRenderer::renderPreviewAsDataUri(const QString& materialName) | ||
| { | ||
| // Check cache first | ||
| auto it = m_cache.find(materialName); | ||
| if (it != m_cache.end()) | ||
| return it.value(); | ||
|
|
||
| QImage image = renderPreview(materialName); | ||
| if (image.isNull()) | ||
| return {}; | ||
|
|
||
| // Convert to PNG base64 | ||
| QByteArray ba; | ||
| QBuffer buffer(&ba); | ||
| buffer.open(QIODevice::WriteOnly); | ||
| image.save(&buffer, "PNG"); | ||
| buffer.close(); | ||
|
|
||
| QString dataUri = QStringLiteral("data:image/png;base64,") + ba.toBase64(); | ||
| m_cache.insert(materialName, dataUri); | ||
| return dataUri; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add breadcrumbs around preview generation and material-file parsing.
These paths are user-visible/significant operations, but they currently emit no SentryReporter::addBreadcrumb(...). At minimum, breadcrumb preview requests/failures and .material file parse attempts/failures so production issues here are diagnosable.
As per coding guidelines "All user-facing actions and significant operations must be tracked with SentryReporter::addBreadcrumb(category, message) using categories like 'ui.action' for toolbar/menu clicks, 'ai.tool_call' for MCP tool invocations, 'file.import'/'file.export' for I/O operations".
Also applies to: 206-226
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MaterialPreviewRenderer.cpp` around lines 141 - 198, Add Sentry
breadcrumbs around preview generation and material parsing: in
MaterialPreviewRenderer::renderPreview, call
SentryReporter::addBreadcrumb("ui.action", QString("renderPreview request:
%1").arg(materialName)) at the start, and add success/failure breadcrumbs before
each early return and inside each catch (e.g., "renderPreview success: %1" and
"renderPreview error: %1") referencing materialName; likewise in
MaterialPreviewRenderer::renderPreviewAsDataUri add a breadcrumb when serving
from cache and when generating a new preview (e.g., "renderPreviewAsDataUri
cache_hit: %1" and "renderPreviewAsDataUri generate: %1"); also apply the same
pattern around the .material parsing routine referenced in the diff (add
breadcrumbs for "file.import" at parse start, parse success, and parse failure)
so failures and requests are recorded—use the existing
SentryReporter::addBreadcrumb(category, message) API and the existing member
symbols (renderPreview, renderPreviewAsDataUri, m_cache, m_sphere,
m_renderTarget) to locate insertion points.
Comment on lines
+178
to
+198
| QString MaterialPreviewRenderer::renderPreviewAsDataUri(const QString& materialName) | ||
| { | ||
| // Check cache first | ||
| auto it = m_cache.find(materialName); | ||
| if (it != m_cache.end()) | ||
| return it.value(); | ||
|
|
||
| QImage image = renderPreview(materialName); | ||
| if (image.isNull()) | ||
| return {}; | ||
|
|
||
| // Convert to PNG base64 | ||
| QByteArray ba; | ||
| QBuffer buffer(&ba); | ||
| buffer.open(QIODevice::WriteOnly); | ||
| image.save(&buffer, "PNG"); | ||
| buffer.close(); | ||
|
|
||
| QString dataUri = QStringLiteral("data:image/png;base64,") + ba.toBase64(); | ||
| m_cache.insert(materialName, dataUri); | ||
| return dataUri; |
There was a problem hiding this comment.
Invalidate cached previews when the material changes.
This cache is keyed only by materialName, so editing or reloading a material under the same name keeps returning the old data URI until some caller explicitly clears it. That will leave the UI showing stale previews. Either invalidate the entry on material mutation/reload or key the cache by a version/content fingerprint instead of name alone.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MaterialPreviewRenderer.cpp` around lines 178 - 198, The cache in
MaterialPreviewRenderer::renderPreviewAsDataUri is keyed only by materialName
causing stale previews; fix by invalidating or augmenting the key: either (A)
subscribe to the material change/reload signal (e.g., connect to a
Material::changed or project reload signal) and call
m_cache.remove(materialName) inside that handler so updated materials clear
their cached entry, or (B) compute and use a version/fingerprint when caching
(e.g., fetch material version or compute a content checksum via renderPreview or
a provided Material::version()/hash()) and store/lookup m_cache using
materialName + ":" + version so renderPreviewAsDataUri returns a new data URI
whenever the material content changes. Ensure updates touch m_cache and that
renderPreview is still used when no valid cache entry exists.
Comment on lines
+59
to
+77
| connect(newSceneBtn, &QPushButton::clicked, this, [this]() { | ||
| m_action = NewScene; | ||
| SentryReporter::addBreadcrumb("ui.action", "Welcome: New Scene"); | ||
| accept(); | ||
| }); | ||
| btnLayout->addWidget(newSceneBtn); | ||
|
|
||
| auto* openFileBtn = new QPushButton("Open File..."); | ||
| openFileBtn->setMinimumHeight(36); | ||
| connect(openFileBtn, &QPushButton::clicked, this, [this]() { | ||
| QString file = QFileDialog::getOpenFileName( | ||
| this, "Open 3D File", QString(), | ||
| "3D Files (*.fbx *.gltf *.glb *.obj *.dae *.stl *.mesh *.3ds *.x);;All Files (*)"); | ||
| if (!file.isEmpty()) { | ||
| m_action = OpenFile; | ||
| m_selectedFile = file; | ||
| SentryReporter::addBreadcrumb("ui.action", "Welcome: Open File"); | ||
| accept(); | ||
| } |
There was a problem hiding this comment.
Persist “Don’t show again” on every exit path.
WelcomeScreen/dontShowAgain is only written from Get Started. If the user checks the box and then chooses New Scene, Open File, Open Recent, or closes the window, the dialog will still reappear on the next launch.
💡 One way to centralize the setting write
+ auto persistDismissPreference = [this, dontShowCheck]() {
+ QSettings settings;
+ settings.setValue("WelcomeScreen/dontShowAgain", dontShowCheck->isChecked());
+ };
+
connect(newSceneBtn, &QPushButton::clicked, this, [this]() {
+ persistDismissPreference();
m_action = NewScene;
SentryReporter::addBreadcrumb("ui.action", "Welcome: New Scene");
accept();
});
@@
connect(openFileBtn, &QPushButton::clicked, this, [this]() {
QString file = QFileDialog::getOpenFileName(
this, "Open 3D File", QString(),
"3D Files (*.fbx *.gltf *.glb *.obj *.dae *.stl *.mesh *.3ds *.x);;All Files (*)");
if (!file.isEmpty()) {
+ persistDismissPreference();
m_action = OpenFile;
m_selectedFile = file;
SentryReporter::addBreadcrumb("ui.action", "Welcome: Open File");
accept();
}
});
@@
- connect(getStartedBtn, &QPushButton::clicked, this, [this, dontShowCheck]() {
- if (dontShowCheck->isChecked()) {
- QSettings settings;
- settings.setValue("WelcomeScreen/dontShowAgain", true);
- }
+ connect(getStartedBtn, &QPushButton::clicked, this, [this]() {
+ persistDismissPreference();
m_action = Dismissed;
SentryReporter::addBreadcrumb("ui.action", "Welcome: Dismissed");
accept();
});Also applies to: 105-110, 135-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/WelcomeDialog.cpp` around lines 59 - 77, The dialog only saves the
"WelcomeScreen/dontShowAgain" preference in the Get Started path; persist it on
every exit path by centralizing the write where the dialog is closed. Update the
New Scene lambda (connected to newSceneBtn), the Open File lambda (openFileBtn),
the Open Recent handlers, and the dialog close/reject path to check the same
checkbox state (e.g., m_dontShowAgain or the checkbox widget) and write the
setting "WelcomeScreen/dontShowAgain" before calling accept()/reject();
alternatively, implement a single helper (e.g., saveDontShowAgain()) and call it
from each lambda and from the dialog's rejectEvent/closeEvent or
WelcomeDialog::accept()/reject() so the preference is saved regardless of how
the dialog exits.
6 tasks
First-launch overlay with quick-start buttons and feature tips. Shows on startup unless "Don't show again" is checked (QSettings). - WelcomeScreenController: QML_SINGLETON exposing recent files, openFile/openFileDialog/newScene/dismiss methods - WelcomeScreen.qml: themed overlay card with New Scene, Open File, recent files list, feature tips (AI Chat, Shortcuts, CLI), checkbox - MainWindow: QQuickWidget overlay, auto-hides on file import, repositions on resize, deferred show via QTimer - Sentry breadcrumbs for all welcome screen interactions Part of #257 (Phase 2: UX Polish & Onboarding) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Searchable cheat sheet dialog accessible via Help menu or Ctrl+/. - ShortcutReference.qml: search bar, 6 categories (Transform, Navigation, Editing, File, View, Help), keyboard-key badges, hover highlights, theme-aware - PropertiesPanelController::shortcutData(): returns 17 shortcuts as QVariantList for QML consumption - MainWindow: Help menu action, QQuickWidget dialog, Sentry breadcrumb Part of #257 (Phase 2: UX Polish & Onboarding) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Preferences Dialog (items 2): - 4 tabs: General, Appearance, Viewport, AI - Generic QSettings wrapper: getSetting/setSetting on PropertiesPanelController - Edit menu → Preferences... (Ctrl+,) - Sentry breadcrumbs for setting changes Context-Sensitive Tooltips (item 5): - All toolbar actions now show descriptive tooltips with shortcut hints (e.g. "Translate Mode (W)", "Undo (Ctrl+Z)", "Duplicate (Ctrl+D)") Part of #257 (Phase 2: UX Polish & Onboarding) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
New dock widget for browsing textures, materials, and meshes on disk. - AssetBrowserController: QML_SINGLETON with file listing, type classification (mesh/texture/material), filter/search, QFileSystemWatcher for auto-refresh, QSettings persistence of last directory - AssetBrowser.qml: path bar with up-nav, filter pills, search field, file list with type icons and sizes, click to load meshes - MainWindow: bottom dock (hidden by default), Options > View toggle, native QFileDialog for browse, mesh import forwarding - 23 unit tests - Sentry breadcrumbs for all interactions Part of #257 (Phase 2: UX Polish & Onboarding) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Welcome screen now covers the entire MainWindow rect (not offset below toolbar) — the QML overlay has its own semi-transparent background for proper visual layering - ViewCube (WindowStaysOnTopHint) hidden while welcome screen is showing, restored on dismiss based on menu toggle state Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Card height now capped to window height - 40px. Content wrapped in a Flickable so it scrolls when the window is too small to show all items (recent files, feature tips, etc.). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…nails - Single click navigates directories; double-click loads files (was: single click loaded meshes immediately) - Added all Assimp-supported mesh extensions (.x, .x3d, .lwo, .ac, .ms3d, .md2, .md3, .smd, .ogex, .b3d, .mesh.xml, and more) - Texture files now show actual image thumbnails (28x28, async loaded) instead of emoji icons Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
.material files in the Asset Browser now show a rendered 64x64 sphere with the actual Ogre material applied, instead of a blue circle emoji. - MaterialPreviewRenderer: singleton managing an offscreen Ogre scene (dedicated SceneManager, camera, light, procedural sphere, RTT target). renderPreview() applies material, renders, copies pixels to QImage. renderPreviewAsDataUri() returns base64 PNG for QML Image.source. firstMaterialNameInFile() parses .material scripts. Results cached in memory. - AssetBrowserController: generates previews when listing .material files, auto-registers Ogre resource locations for material directories - AssetBrowser.qml: material preview Image element alongside texture thumbs - 11 unit tests (file parsing, cache, RTT with graceful Ogre skip) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
initialiseAllResourceGroups() doesn't re-parse material scripts in already-initialized resource groups. Now uses ScriptCompilerManager::parseScript() to directly parse the .material file content, and registers the directory for texture resolution. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Material Presets section now uses a GridView of rendered sphere cards (48x48 Ogre RTT) instead of the previous Canvas 2D procedural drawing. - PropertiesPanelController::materialPresetPreview(): creates the Ogre preset material if needed, returns RTT data URI via MaterialPreviewRenderer - GridView with responsive cell sizing, hover highlight, active border - Short label extracted from "Category (Name)" format - Removed ~100 lines of Canvas wireframe/gradient sphere code Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This reverts commit 47389bf.
MaterialListModal.qml rewritten from flat ListView to a GridView of cards, each showing a 52x52 rendered sphere with the actual Ogre material applied via MaterialPreviewRenderer. - MaterialEditorQML::materialPreview(): returns RTT data URI - Search bar to filter materials by name - Cards: hover highlight, selection border, double-click to edit - Fallback blue circle if preview fails Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replaced the QML overlay welcome screen with a native QDialog that shows before MainWindow is created, matching the tracking consent dialog pattern. - WelcomeDialog: QDialog with New Scene, Open File, recent files list, tips, "Don't show again" checkbox - main.cpp: shows dialog before MainWindow, passes file choice via MainWindow::loadFile() after show - Disabled the QML overlay auto-show on startup (kept for programmatic use) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Bump project version from 2.23.0 to 2.24.0 - Add WelcomeDialog_test.cpp with tests for shouldShow() and defaults - Add AssetBrowserController and MaterialPreviewRenderer to tests/CMakeLists.txt Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- WelcomeDialog: "Don't show again" now persists on ALL exit paths (New Scene, Open File, Recent, Get Started) via QDialog::accepted signal - Recent files: added itemActivated for keyboard (Enter key) activation - MaterialPreviewRenderer: clean up partial Ogre state on ensureScene failure Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
fernandotonon
force-pushed
the
feat/phase2-ux-polish
branch
from
April 11, 2026 04:55
cc1258a to
c3479f8
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (9)
src/MaterialPreviewRenderer.cpp (3)
44-61:⚠️ Potential issue | 🟠 MajorTear down partial Ogre state on every initialization failure path.
This still leaks partially created preview resources when
ensureScene()fails after creating the RTT texture or when the genericcatch (...)path is taken. Becausem_initializedstaysfalse, the destructor won’t clean that state up later.Suggested fix
MaterialPreviewRenderer::~MaterialPreviewRenderer() { - if (m_initialized) { - auto* root = Ogre::Root::getSingletonPtr(); - if (root) { - // Remove the render texture first - if (m_rttTexture) { - Ogre::TextureManager::getSingleton().remove(m_rttTexture); - m_rttTexture.reset(); - } - - // Destroy the preview scene manager (cleans up all its nodes/entities/lights) - if (m_sceneMgr) { - root->destroySceneManager(m_sceneMgr); - m_sceneMgr = nullptr; - } - } - } + auto* root = Ogre::Root::getSingletonPtr(); + if (m_rttTexture) { + Ogre::TextureManager::getSingleton().remove(m_rttTexture); + m_rttTexture.reset(); + } + if (root && m_sceneMgr) { + root->destroySceneManager(m_sceneMgr); + m_sceneMgr = nullptr; + } } @@ - } catch (const Ogre::Exception&) { - // Clean up partial state on failure - if (m_sceneMgr) { - Ogre::Root::getSingletonPtr()->destroySceneManager(m_sceneMgr); - m_sceneMgr = nullptr; - } - m_camera = nullptr; - m_light = nullptr; - m_sphere = nullptr; - m_sphereNode = nullptr; - m_renderTarget = nullptr; - return false; - } catch (...) { - return false; + } catch (...) { + if (m_rttTexture) { + Ogre::TextureManager::getSingleton().remove(m_rttTexture); + m_rttTexture.reset(); + } + if (auto* cleanupRoot = Ogre::Root::getSingletonPtr(); cleanupRoot && m_sceneMgr) { + cleanupRoot->destroySceneManager(m_sceneMgr); + m_sceneMgr = nullptr; + } + m_camera = nullptr; + m_light = nullptr; + m_sphere = nullptr; + m_sphereNode = nullptr; + m_renderTarget = nullptr; + return false; }Also applies to: 117-148
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MaterialPreviewRenderer.cpp` around lines 44 - 61, The destructor currently only cleans up preview resources when m_initialized is true, causing leaks if ensureScene() fails after creating m_rttTexture or m_sceneMgr; add a deterministic teardown path for partial state by implementing and using a helper (e.g., cleanupPreviewResources or teardownPreviewScene) that removes m_rttTexture via Ogre::TextureManager::getSingleton().remove(...), resets m_rttTexture, destroys m_sceneMgr via root->destroySceneManager(m_sceneMgr) and nulls it, and call that helper both from the destructor (unconditionally or before checking m_initialized) and from every failure path in ensureScene() and the catch(...) block so partial resources are always released and m_initialized/state is left consistent.
151-209:⚠️ Potential issue | 🟠 MajorAdd breadcrumbs around preview generation and material parsing.
These are user-visible/significant operations, but they still emit no breadcrumbs, which makes production failures here hard to diagnose. As per coding guidelines "Track all user-facing actions and significant operations with SentryReporter::addBreadcrumb(category, message). Use ui.action for toolbar/menu clicks, ai.tool_call for MCP tool invocations, file.import/file.export for I/O operations".
Also applies to: 216-236
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MaterialPreviewRenderer.cpp` around lines 151 - 209, Add Sentry breadcrumbs around the significant operations in renderPreview and renderPreviewAsDataUri: call SentryReporter::addBreadcrumb before checking/looking up the material (around Ogre::MaterialManager::resourceExists(stdName)) with category "ui.action" and a message like "preview: material lookup <materialName>", before applying the material (around m_sphere->setMaterialName(stdName)) with message "preview: apply material <materialName>", before rendering/updating the render target (around m_renderTarget->update()) with message "preview: render", and in renderPreviewAsDataUri before/after image save/encode steps with messages like "preview: encode png" so failures include these breadcrumbs for diagnosis.
188-208:⚠️ Potential issue | 🟠 MajorCache invalidation is still too coarse.
The cache key is only
materialName, so editing or reloading a material under the same name keeps returning the old data URI until something explicitly callsclearCache().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MaterialPreviewRenderer.cpp` around lines 188 - 208, The cache key is only materialName in MaterialPreviewRenderer::renderPreviewAsDataUri which causes stale entries in m_cache after edits; update the lookup/insert to use a composite key including a material version token (e.g., last-modified timestamp or version/hash) so the key becomes materialName + ":" + version; obtain that version from the authoritative source (e.g., MaterialRepository::lastModified(materialName) or Material::version()) before checking m_cache, or compute a quick checksum of the freshly rendered QImage via QImage::bits()/size() -> QCryptographicHash if no repo API exists, and use that checksum as the version; then change the m_cache access in renderPreviewAsDataUri (lookup, insert) to use the composite key and remove the old single-name entries as needed to avoid duplicates.src/AssetBrowserController.cpp (4)
154-154:⚠️ Potential issue | 🟠 MajorUse
completeSuffix()so.mesh.xmlstays classified as a mesh.
QFileInfo::suffix()only returns the last segment, somodel.mesh.xmlbecomesxmlhere even thoughs_meshExtensionsexplicitly includesmesh.xml. That breaks filtering, file-type reporting, and import for those assets.Suggested fix
- QString type = classifyExtension(fi.suffix().toLower()); + QString type = classifyExtension(fi.completeSuffix().toLower()); @@ - return classifyExtension(fi.suffix().toLower()); + return classifyExtension(fi.completeSuffix().toLower()); @@ - type = classifyExtension(fi.suffix().toLower()); + type = classifyExtension(fi.completeSuffix().toLower());Also applies to: 183-183, 206-206
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AssetBrowserController.cpp` at line 154, The code calls classifyExtension with fi.suffix().toLower(), which only returns the last segment (breaking multi-part extensions like "mesh.xml"); replace those uses with fi.completeSuffix().toLower() so classifyExtension receives the full compound suffix (e.g., "mesh.xml"). Update every occurrence where classifyExtension is called with fi.suffix() (e.g., the call site using variable fi and classifyExtension) to use completeSuffix(), and run the asset-filtering/import tests to verify mesh.xml files are correctly classified (also update the other similar call sites noted in the review).
62-66:⚠️ Potential issue | 🔴 CriticalGuard
enginebefore callingsetObjectOwnership().
src/AssetBrowserController_test.cppalready exercisesqmlInstance(nullptr, nullptr), so Line 65 is a straight null dereference today.Suggested fix
AssetBrowserController* AssetBrowserController::qmlInstance(QQmlEngine* engine, QJSEngine* /*scriptEngine*/) { auto* inst = instance(); - engine->setObjectOwnership(inst, QQmlEngine::CppOwnership); + if (engine) + engine->setObjectOwnership(inst, QQmlEngine::CppOwnership); return inst; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AssetBrowserController.cpp` around lines 62 - 66, The qmlInstance function calls engine->setObjectOwnership without checking engine, causing a null dereference when qmlInstance(nullptr, ... ) is used; update AssetBrowserController::qmlInstance to guard the engine pointer (e.g., if (engine) { engine->setObjectOwnership(inst, QQmlEngine::CppOwnership); }) before calling setObjectOwnership, then return inst as before so tests exercising qmlInstance(nullptr, nullptr) no longer crash.
91-91:⚠️ Potential issue | 🟠 MajorUse standard breadcrumb categories and stop sending full local paths.
These breadcrumbs still use the ad-hoc
"asset_browser"category, and Line 91 logs the absolute directory path. That makes traces harder to query and leaks more filesystem detail than needed. As per coding guidelines "Track all user-facing actions and significant operations with SentryReporter::addBreadcrumb(category, message). Use ui.action for toolbar/menu clicks, ai.tool_call for MCP tool invocations, file.import/file.export for I/O operations".Also applies to: 137-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AssetBrowserController.cpp` at line 91, Breadcrumbs are using a nonstandard category and exposing full local paths; update calls to SentryReporter::addBreadcrumb to use standard categories (e.g., "ui.action" for user-driven root changes or "file.import"/"file.export" for explicit I/O) and stop emitting the absolute m_rootPath directly—replace the message with a sanitized string (e.g., "Root directory changed" plus either a redacted indicator or just basename/ID) in the SentryReporter::addBreadcrumb calls (change the call that references m_rootPath and the similar calls in the 137-157 region), ensuring you only include non-sensitive, queryable info while keeping the category standard.
272-299:⚠️ Potential issue | 🟠 MajorReloading is still skipped for existing material names.
This branch still short-circuits on
resourceExists(matName), so an updated.materialfile—or a different file reusing the same material name—keeps the old Ogre resource and old preview.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AssetBrowserController.cpp` around lines 272 - 299, The code currently skips parsing when matMgr->resourceExists(matName) is true, so updated .material files or different files using the same material name won't refresh; change the logic to always parse the script and ensure any existing Ogre resource is unloaded/removed before parsing — locate the check using matMgr->resourceExists(matName) and instead, if the resource exists obtain it via matMgr->getByName/getResourceByName (or the MaterialManager equivalent), call unload(true) and/or remove the resource from the manager, then register the file path with Ogre::ResourceGroupManager::addResourceLocation and call Ogre::ScriptCompilerManager::getSingleton().parseScript(...) as before; finally call MaterialPreviewRenderer::instance()->renderPreviewAsDataUri(matName) to generate the updated preview.qml/MaterialListModal.qml (1)
149-156:⚠️ Potential issue | 🟠 MajorDon’t render previews synchronously from the delegate binding.
This still calls
MaterialEditorQML.materialPreview(modelData)as delegates are created/recycled, so opening, filtering, or scrolling the grid can stall the UI thread.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@qml/MaterialListModal.qml` around lines 149 - 156, The delegate is synchronously calling MaterialEditorQML.materialPreview(modelData) in the Image.source binding which blocks during delegate creation/recycling; change to an async assignment: add a property string previewSource (unique symbol) on the delegate, set Image.source to previewSource instead of calling MaterialEditorQML.materialPreview directly, and schedule filling previewSource from MaterialEditorQML.materialPreview(modelData) via a short one-shot Timer (e.g., previewTimer with interval 0) or from Component.onCompleted/onVisibleChanged to defer work to the event loop; this keeps the binding fast while still populating the preview asynchronously.src/mainwindow.cpp (1)
453-456:⚠️ Potential issue | 🟡 MinorAsset Browser import path still skips recent-files sync and uses a non-standard breadcrumb category.
This flow bypasses
addToRecentFiles(...), so files imported from the Asset Browser won’t appear in Recent Files / welcome recents. Also, the breadcrumb category should befile.importfor this I/O path.Suggested fix
connect(abController, &AssetBrowserController::importMeshRequested, this, [this](const QStringList& paths) { - SentryReporter::addBreadcrumb("asset_browser", "Importing mesh from Asset Browser"); + SentryReporter::addBreadcrumb("file.import", "Importing mesh from Asset Browser"); + for (const QString& path : paths) + addToRecentFiles(path); importMeshs(paths); });As per coding guidelines: "Track all user-facing actions and significant operations with SentryReporter::addBreadcrumb(category, message)... use file.import/file.export for I/O operations."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mainwindow.cpp` around lines 453 - 456, The Asset Browser import handler bypasses recent-files sync and uses a non-standard breadcrumb category; update the lambda connected to AssetBrowserController::importMeshRequested to call addToRecentFiles(paths) before invoking importMeshs(paths) and change the SentryReporter::addBreadcrumb category from "asset_browser" to "file.import" so imports are tracked in recent files and use the correct I/O breadcrumb category.
🧹 Nitpick comments (3)
src/AssetBrowserController_test.cpp (1)
225-235: Add a.mesh.xmlregression case here.The controller explicitly supports
"mesh.xml", but this test only covers single-segment extensions, so it won’t catch the current suffix-classification bug. As per coding guidelines "Add Google Test unit tests for new functionality. Test files live alongside source in src/ with _test.cpp suffix".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AssetBrowserController_test.cpp` around lines 225 - 235, The test misses a regression for multi-segment extensions like "mesh.xml": update the TEST_F AssetBrowserControllerTests/FileTypeClassification to add an expectation that calling AssetBrowserController::instance()->fileTypeForPath("/foo/bar.mesh.xml") returns "mesh" (similar to other cases) to cover the suffix-classification bug; locate the test function named FileTypeClassification and add the new EXPECT_EQ assertion alongside the existing fileTypeForPath checks so the unit suite catches the regression.src/AssetBrowserController.cpp (1)
186-245: Avoid generating every material thumbnail synchronously insiderefreshFiles().
refreshFiles()runs for filter changes, every search keystroke, and directory watcher events. Doing.materialparsing plus preview rendering inline here will make large folders feel sluggish; this wants lazy or async preview population instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AssetBrowserController.cpp` around lines 186 - 245, refreshFiles() is synchronously calling materialPreview() for every .material file which blocks UI on filter/search; remove the inline preview generation and instead populate m_files with a placeholder entry (set "previewUrl" empty or "loading") inside AssetBrowserController::refreshFiles(), then kick off async preview generation (use QtConcurrent::run or a dedicated QThread/worker) to call materialPreview(const QString&) for each material entry and, when each preview is ready, update the corresponding QVariantMap in m_files (or a preview cache keyed by path) and emit a specific signal (e.g., materialPreviewReady(path, previewUrl) or reuse filesChanged) so the view can update; ensure to reference AssetBrowserController::refreshFiles, materialPreview(), m_files, and the new async worker/signal when implementing.src/mainwindow.cpp (1)
473-482: UseloadFile(...)here to avoid divergent file-routing logic.This lambda duplicates the new helper’s behavior; routing through
loadFile(path)keeps scene/import handling and recent-file updates centralized.Suggested refactor
connect(m_welcomeController, &WelcomeScreenController::requestOpenFile, this, [this](const QString& path) { - if (QFileInfo::exists(path)) { - addToRecentFiles(path); - if (path.endsWith(".scene.glb") || path.endsWith(".scene.gltf")) - MeshImporterExporter::sceneImporter(path); - else - mUriList.append(path); - } + if (QFileInfo::exists(path)) + loadFile(path); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mainwindow.cpp` around lines 473 - 482, The lambda connected to WelcomeScreenController::requestOpenFile duplicates file-routing logic; replace the body to call the centralized loadFile(path) helper instead of manually checking QFileInfo, addToRecentFiles, MeshImporterExporter::sceneImporter, or appending to mUriList. Ensure loadFile(path) covers adding to recent files and handles scene import vs URI list so the single call from the m_welcomeController connection centralizes behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@qml/MaterialListModal.qml`:
- Line 123: The cellWidth calculation in MaterialListModal.qml can divide by
zero when Math.floor((width - 20) / 90) evaluates to 0; change the logic to
compute a safe columns value (e.g., columns = Math.max(1, Math.floor((width -
20) / 90))) and then set cellWidth using (width - 20) / columns (or Math.max(90,
(width - 20) / columns)) so the denominator is never zero; update the expression
referenced by the cellWidth property to use this guarded columns variable.
In `@src/mainwindow.cpp`:
- Around line 635-645: The Asset Browser "Browse…" handler connected to
AssetBrowserController::browseRequested should record a Sentry breadcrumb;
inside the lambda (the connect callback that calls
QFileDialog::getExistingDirectory) call
SentryReporter::addBreadcrumb("ui.action", "Asset Browser: Browse…") before
showing the dialog (and optionally add another breadcrumb when a new root is set
by calling SentryReporter::addBreadcrumb("ui.action", QString("Asset Browser:
Set rootPath to %1").arg(dir)) right before
AssetBrowserController::instance()->setRootPath(dir)). Ensure you add these
calls in the same lambda used in the connect to
AssetBrowserController::browseRequested so the user action is tracked.
In `@src/WelcomeDialog_test.cpp`:
- Around line 10-21: SetUp() currently unconditionally removes the
"WelcomeScreen/dontShowAgain" key and TearDown() unconditionally removes it
again, which can erase a developer's real preference; change SetUp() to save the
current value (use QSettings::contains and value) into a member variable, then
set QCoreApplication::setOrganizationName(...) and setApplicationName(...) as
other tests do, and explicitly set the test-specific value for
"WelcomeScreen/dontShowAgain"; change TearDown() to restore the saved value (if
it existed, using settings.setValue) or remove the key if it did not previously
exist. Ensure you reference the SetUp and TearDown methods and use QSettings and
QCoreApplication in your changes so the original preference is preserved and
QSettings are isolated for the test.
---
Duplicate comments:
In `@qml/MaterialListModal.qml`:
- Around line 149-156: The delegate is synchronously calling
MaterialEditorQML.materialPreview(modelData) in the Image.source binding which
blocks during delegate creation/recycling; change to an async assignment: add a
property string previewSource (unique symbol) on the delegate, set Image.source
to previewSource instead of calling MaterialEditorQML.materialPreview directly,
and schedule filling previewSource from
MaterialEditorQML.materialPreview(modelData) via a short one-shot Timer (e.g.,
previewTimer with interval 0) or from Component.onCompleted/onVisibleChanged to
defer work to the event loop; this keeps the binding fast while still populating
the preview asynchronously.
In `@src/AssetBrowserController.cpp`:
- Line 154: The code calls classifyExtension with fi.suffix().toLower(), which
only returns the last segment (breaking multi-part extensions like "mesh.xml");
replace those uses with fi.completeSuffix().toLower() so classifyExtension
receives the full compound suffix (e.g., "mesh.xml"). Update every occurrence
where classifyExtension is called with fi.suffix() (e.g., the call site using
variable fi and classifyExtension) to use completeSuffix(), and run the
asset-filtering/import tests to verify mesh.xml files are correctly classified
(also update the other similar call sites noted in the review).
- Around line 62-66: The qmlInstance function calls engine->setObjectOwnership
without checking engine, causing a null dereference when qmlInstance(nullptr,
... ) is used; update AssetBrowserController::qmlInstance to guard the engine
pointer (e.g., if (engine) { engine->setObjectOwnership(inst,
QQmlEngine::CppOwnership); }) before calling setObjectOwnership, then return
inst as before so tests exercising qmlInstance(nullptr, nullptr) no longer
crash.
- Line 91: Breadcrumbs are using a nonstandard category and exposing full local
paths; update calls to SentryReporter::addBreadcrumb to use standard categories
(e.g., "ui.action" for user-driven root changes or "file.import"/"file.export"
for explicit I/O) and stop emitting the absolute m_rootPath directly—replace the
message with a sanitized string (e.g., "Root directory changed" plus either a
redacted indicator or just basename/ID) in the SentryReporter::addBreadcrumb
calls (change the call that references m_rootPath and the similar calls in the
137-157 region), ensuring you only include non-sensitive, queryable info while
keeping the category standard.
- Around line 272-299: The code currently skips parsing when
matMgr->resourceExists(matName) is true, so updated .material files or different
files using the same material name won't refresh; change the logic to always
parse the script and ensure any existing Ogre resource is unloaded/removed
before parsing — locate the check using matMgr->resourceExists(matName) and
instead, if the resource exists obtain it via
matMgr->getByName/getResourceByName (or the MaterialManager equivalent), call
unload(true) and/or remove the resource from the manager, then register the file
path with Ogre::ResourceGroupManager::addResourceLocation and call
Ogre::ScriptCompilerManager::getSingleton().parseScript(...) as before; finally
call MaterialPreviewRenderer::instance()->renderPreviewAsDataUri(matName) to
generate the updated preview.
In `@src/mainwindow.cpp`:
- Around line 453-456: The Asset Browser import handler bypasses recent-files
sync and uses a non-standard breadcrumb category; update the lambda connected to
AssetBrowserController::importMeshRequested to call addToRecentFiles(paths)
before invoking importMeshs(paths) and change the SentryReporter::addBreadcrumb
category from "asset_browser" to "file.import" so imports are tracked in recent
files and use the correct I/O breadcrumb category.
In `@src/MaterialPreviewRenderer.cpp`:
- Around line 44-61: The destructor currently only cleans up preview resources
when m_initialized is true, causing leaks if ensureScene() fails after creating
m_rttTexture or m_sceneMgr; add a deterministic teardown path for partial state
by implementing and using a helper (e.g., cleanupPreviewResources or
teardownPreviewScene) that removes m_rttTexture via
Ogre::TextureManager::getSingleton().remove(...), resets m_rttTexture, destroys
m_sceneMgr via root->destroySceneManager(m_sceneMgr) and nulls it, and call that
helper both from the destructor (unconditionally or before checking
m_initialized) and from every failure path in ensureScene() and the catch(...)
block so partial resources are always released and m_initialized/state is left
consistent.
- Around line 151-209: Add Sentry breadcrumbs around the significant operations
in renderPreview and renderPreviewAsDataUri: call SentryReporter::addBreadcrumb
before checking/looking up the material (around
Ogre::MaterialManager::resourceExists(stdName)) with category "ui.action" and a
message like "preview: material lookup <materialName>", before applying the
material (around m_sphere->setMaterialName(stdName)) with message "preview:
apply material <materialName>", before rendering/updating the render target
(around m_renderTarget->update()) with message "preview: render", and in
renderPreviewAsDataUri before/after image save/encode steps with messages like
"preview: encode png" so failures include these breadcrumbs for diagnosis.
- Around line 188-208: The cache key is only materialName in
MaterialPreviewRenderer::renderPreviewAsDataUri which causes stale entries in
m_cache after edits; update the lookup/insert to use a composite key including a
material version token (e.g., last-modified timestamp or version/hash) so the
key becomes materialName + ":" + version; obtain that version from the
authoritative source (e.g., MaterialRepository::lastModified(materialName) or
Material::version()) before checking m_cache, or compute a quick checksum of the
freshly rendered QImage via QImage::bits()/size() -> QCryptographicHash if no
repo API exists, and use that checksum as the version; then change the m_cache
access in renderPreviewAsDataUri (lookup, insert) to use the composite key and
remove the old single-name entries as needed to avoid duplicates.
---
Nitpick comments:
In `@src/AssetBrowserController_test.cpp`:
- Around line 225-235: The test misses a regression for multi-segment extensions
like "mesh.xml": update the TEST_F
AssetBrowserControllerTests/FileTypeClassification to add an expectation that
calling AssetBrowserController::instance()->fileTypeForPath("/foo/bar.mesh.xml")
returns "mesh" (similar to other cases) to cover the suffix-classification bug;
locate the test function named FileTypeClassification and add the new EXPECT_EQ
assertion alongside the existing fileTypeForPath checks so the unit suite
catches the regression.
In `@src/AssetBrowserController.cpp`:
- Around line 186-245: refreshFiles() is synchronously calling materialPreview()
for every .material file which blocks UI on filter/search; remove the inline
preview generation and instead populate m_files with a placeholder entry (set
"previewUrl" empty or "loading") inside AssetBrowserController::refreshFiles(),
then kick off async preview generation (use QtConcurrent::run or a dedicated
QThread/worker) to call materialPreview(const QString&) for each material entry
and, when each preview is ready, update the corresponding QVariantMap in m_files
(or a preview cache keyed by path) and emit a specific signal (e.g.,
materialPreviewReady(path, previewUrl) or reuse filesChanged) so the view can
update; ensure to reference AssetBrowserController::refreshFiles,
materialPreview(), m_files, and the new async worker/signal when implementing.
In `@src/mainwindow.cpp`:
- Around line 473-482: The lambda connected to
WelcomeScreenController::requestOpenFile duplicates file-routing logic; replace
the body to call the centralized loadFile(path) helper instead of manually
checking QFileInfo, addToRecentFiles, MeshImporterExporter::sceneImporter, or
appending to mUriList. Ensure loadFile(path) covers adding to recent files and
handles scene import vs URI list so the single call from the m_welcomeController
connection centralizes behavior.
🪄 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: 3c40c7da-db79-4a0b-9cbc-811ed3941ddb
📒 Files selected for processing (28)
CMakeLists.txtqml/AssetBrowser.qmlqml/MaterialListModal.qmlqml/PreferencesDialog.qmlqml/ShortcutReference.qmlqml/WelcomeScreen.qmlsrc/AssetBrowserController.cppsrc/AssetBrowserController.hsrc/AssetBrowserController_test.cppsrc/CMakeLists.txtsrc/MaterialEditorQML.cppsrc/MaterialEditorQML.hsrc/MaterialPreviewRenderer.cppsrc/MaterialPreviewRenderer.hsrc/MaterialPreviewRenderer_test.cppsrc/PropertiesPanelController.cppsrc/PropertiesPanelController.hsrc/WelcomeDialog.cppsrc/WelcomeDialog.hsrc/WelcomeDialog_test.cppsrc/WelcomeScreenController.cppsrc/WelcomeScreenController.hsrc/main.cppsrc/mainwindow.cppsrc/mainwindow.hsrc/qml_resources.qrctests/CMakeLists.txtui_files/mainwindow.ui
✅ Files skipped from review due to trivial changes (6)
- CMakeLists.txt
- src/qml_resources.qrc
- src/CMakeLists.txt
- src/WelcomeDialog.h
- qml/PreferencesDialog.qml
- src/WelcomeScreenController.h
🚧 Files skipped from review as they are similar to previous changes (11)
- tests/CMakeLists.txt
- src/MaterialEditorQML.cpp
- src/MaterialEditorQML.h
- src/PropertiesPanelController.h
- src/mainwindow.h
- src/main.cpp
- src/PropertiesPanelController.cpp
- qml/WelcomeScreen.qml
- src/WelcomeDialog.cpp
- src/MaterialPreviewRenderer.h
- ui_files/mainwindow.ui
| GridView { | ||
| id: materialGrid | ||
| clip: true | ||
| cellWidth: Math.max(90, (width - 20) / Math.floor((width - 20) / 90)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n qml/MaterialListModal.qml | sed -n '115,135p'Repository: fernandotonon/QtMeshEditor
Length of output: 876
🏁 Script executed:
python3 <<'EOF'
# Test the mathematical expression to confirm the division-by-zero scenario
def test_cellwidth_calculation():
test_widths = [10, 20, 50, 90, 100, 110, 200, 500]
for width in test_widths:
# Original formula
available = width - 20
divisor = (width - 20) / 90
floor_result = int(divisor) # Math.floor equivalent
print(f"width={width}:")
print(f" available={available}, divisor={divisor:.2f}, floor={floor_result}")
if floor_result == 0:
cellwidth = float('inf')
print(f" ⚠️ DIVISION BY ZERO! cellWidth = {cellwidth}")
else:
cellwidth = max(90, available / floor_result)
print(f" cellWidth = {cellwidth:.2f}")
print()
test_cellwidth_calculation()
EOFRepository: fernandotonon/QtMeshEditor
Length of output: 728
Guard the column calculation against zero divisions.
Line 123 evaluates Math.floor((width - 20) / 90) to 0 when the view width is less than 110 pixels (during initialization or narrow window resizing), causing cellWidth to become Infinity and breaking the grid layout.
Suggested fix
- cellWidth: Math.max(90, (width - 20) / Math.floor((width - 20) / 90))
+ cellWidth: {
+ var available = Math.max(0, width - 20)
+ var columns = Math.max(1, Math.floor(available / 90))
+ return Math.max(90, available / columns)
+ }📝 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
| cellWidth: Math.max(90, (width - 20) / Math.floor((width - 20) / 90)) | |
| cellWidth: { | |
| var available = Math.max(0, width - 20) | |
| var columns = Math.max(1, Math.floor(available / 90)) | |
| return Math.max(90, available / columns) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@qml/MaterialListModal.qml` at line 123, The cellWidth calculation in
MaterialListModal.qml can divide by zero when Math.floor((width - 20) / 90)
evaluates to 0; change the logic to compute a safe columns value (e.g., columns
= Math.max(1, Math.floor((width - 20) / 90))) and then set cellWidth using
(width - 20) / columns (or Math.max(90, (width - 20) / columns)) so the
denominator is never zero; update the expression referenced by the cellWidth
property to use this guarded columns variable.
Comment on lines
+635
to
+645
| connect(AssetBrowserController::instance(), &AssetBrowserController::browseRequested, | ||
| this, [this]() { | ||
| QTimer::singleShot(0, this, [this]() { | ||
| QString dir = QFileDialog::getExistingDirectory( | ||
| this, tr("Select Asset Directory"), | ||
| AssetBrowserController::instance()->rootPath(), | ||
| QFileDialog::DontUseNativeDialog | QFileDialog::ShowDirsOnly); | ||
| if (!dir.isEmpty()) | ||
| AssetBrowserController::instance()->setRootPath(dir); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Add breadcrumb for Asset Browser “Browse…” action.
This user-facing action currently has no breadcrumb, which reduces interaction traceability.
Suggested fix
connect(AssetBrowserController::instance(), &AssetBrowserController::browseRequested,
this, [this]() {
+ SentryReporter::addBreadcrumb("ui.action", "Asset Browser browse directory");
QTimer::singleShot(0, this, [this]() {
QString dir = QFileDialog::getExistingDirectory(
this, tr("Select Asset Directory"),
AssetBrowserController::instance()->rootPath(),
QFileDialog::DontUseNativeDialog | QFileDialog::ShowDirsOnly);As per coding guidelines: "Track all user-facing actions and significant operations with SentryReporter::addBreadcrumb(category, message). Use ui.action for toolbar/menu clicks..."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mainwindow.cpp` around lines 635 - 645, The Asset Browser "Browse…"
handler connected to AssetBrowserController::browseRequested should record a
Sentry breadcrumb; inside the lambda (the connect callback that calls
QFileDialog::getExistingDirectory) call
SentryReporter::addBreadcrumb("ui.action", "Asset Browser: Browse…") before
showing the dialog (and optionally add another breadcrumb when a new root is set
by calling SentryReporter::addBreadcrumb("ui.action", QString("Asset Browser:
Set rootPath to %1").arg(dir)) right before
AssetBrowserController::instance()->setRootPath(dir)). Ensure you add these
calls in the same lambda used in the connect to
AssetBrowserController::browseRequested so the user action is tracked.
Comment on lines
+10
to
+21
| void SetUp() override { | ||
| ASSERT_NE(qobject_cast<QApplication*>(QCoreApplication::instance()), nullptr); | ||
| // Clear the setting before each test | ||
| QSettings settings; | ||
| settings.remove("WelcomeScreen/dontShowAgain"); | ||
| } | ||
|
|
||
| void TearDown() override { | ||
| // Clean up the setting after each test | ||
| QSettings settings; | ||
| settings.remove("WelcomeScreen/dontShowAgain"); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether QSettings isolation is configured in tests.
rg -nP --type=cpp -C3 '\bsetOrganizationName\s*\(|\bsetApplicationName\s*\(|QSettings::setPath\s*\(|QSettings::setDefaultFormat\s*\(' src
rg -nP --type=cpp -C2 'WelcomeScreen/dontShowAgain' srcRepository: fernandotonon/QtMeshEditor
Length of output: 7927
🏁 Script executed:
# Find and examine test_main.cpp
fd -t f "test_main.cpp" srcRepository: fernandotonon/QtMeshEditor
Length of output: 87
🏁 Script executed:
# Check the full content of test_main.cpp if it exists
if [ -f src/test_main.cpp ]; then
cat -n src/test_main.cpp
fiRepository: fernandotonon/QtMeshEditor
Length of output: 3639
🏁 Script executed:
# Also check if WelcomeDialog_test.cpp has any setup we missed
cat -n src/WelcomeDialog_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 1941
Save and restore pre-existing QSettings value to avoid erasing developer preferences
SetUp() and TearDown() unconditionally remove WelcomeScreen/dontShowAgain (lines 14, 20). Running tests locally will erase a developer's real "don't show again" preference. Snapshot the prior value in SetUp() and restore it in TearDown().
Additionally, this test should follow the pattern established by peer test fixtures (SDManager_test.cpp, mainwindow_test.cpp, CLIPipeline_test.cpp) and explicitly set QCoreApplication organization and application names in SetUp() to ensure QSettings isolation.
Proposed fix
+#include <optional>
`#include` <QApplication>
`#include` <QCoreApplication>
`#include` <QSettings>
class WelcomeDialogTests : public ::testing::Test {
protected:
+ std::optional<QVariant> previousDontShowAgain;
+
void SetUp() override {
ASSERT_NE(qobject_cast<QApplication*>(QCoreApplication::instance()), nullptr);
- // Clear the setting before each test
QSettings settings;
+ if (settings.contains("WelcomeScreen/dontShowAgain")) {
+ previousDontShowAgain = settings.value("WelcomeScreen/dontShowAgain");
+ } else {
+ previousDontShowAgain.reset();
+ }
settings.remove("WelcomeScreen/dontShowAgain");
}
void TearDown() override {
- // Clean up the setting after each test
QSettings settings;
- settings.remove("WelcomeScreen/dontShowAgain");
+ if (previousDontShowAgain.has_value()) {
+ settings.setValue("WelcomeScreen/dontShowAgain", *previousDontShowAgain);
+ } else {
+ settings.remove("WelcomeScreen/dontShowAgain");
+ }
}
};📝 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
| void SetUp() override { | |
| ASSERT_NE(qobject_cast<QApplication*>(QCoreApplication::instance()), nullptr); | |
| // Clear the setting before each test | |
| QSettings settings; | |
| settings.remove("WelcomeScreen/dontShowAgain"); | |
| } | |
| void TearDown() override { | |
| // Clean up the setting after each test | |
| QSettings settings; | |
| settings.remove("WelcomeScreen/dontShowAgain"); | |
| } | |
| `#include` <optional> | |
| `#include` <QApplication> | |
| `#include` <QCoreApplication> | |
| `#include` <QSettings> | |
| class WelcomeDialogTests : public ::testing::Test { | |
| protected: | |
| std::optional<QVariant> previousDontShowAgain; | |
| void SetUp() override { | |
| ASSERT_NE(qobject_cast<QApplication*>(QCoreApplication::instance()), nullptr); | |
| QSettings settings; | |
| if (settings.contains("WelcomeScreen/dontShowAgain")) { | |
| previousDontShowAgain = settings.value("WelcomeScreen/dontShowAgain"); | |
| } else { | |
| previousDontShowAgain.reset(); | |
| } | |
| settings.remove("WelcomeScreen/dontShowAgain"); | |
| } | |
| void TearDown() override { | |
| QSettings settings; | |
| if (previousDontShowAgain.has_value()) { | |
| settings.setValue("WelcomeScreen/dontShowAgain", *previousDontShowAgain); | |
| } else { | |
| settings.remove("WelcomeScreen/dontShowAgain"); | |
| } | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/WelcomeDialog_test.cpp` around lines 10 - 21, SetUp() currently
unconditionally removes the "WelcomeScreen/dontShowAgain" key and TearDown()
unconditionally removes it again, which can erase a developer's real preference;
change SetUp() to save the current value (use QSettings::contains and value)
into a member variable, then set QCoreApplication::setOrganizationName(...) and
setApplicationName(...) as other tests do, and explicitly set the test-specific
value for "WelcomeScreen/dontShowAgain"; change TearDown() to restore the saved
value (if it existed, using settings.setValue) or remove the key if it did not
previously exist. Ensure you reference the SetUp and TearDown methods and use
QSettings and QCoreApplication in your changes so the original preference is
preserved and QSettings are isolated for the test.
… tab - Settings now apply immediately: grid visibility, camera speed, near/far clip, telemetry toggle - Replaced Qt Controls CheckBox with themed custom checkboxes matching snap settings style (Rectangle + checkmark + MouseArea) - Added "Show welcome screen on startup" toggle in General tab - Removed AI tab (use dedicated AI Settings dialog instead) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…iewport - Theme Light/Dark now uses QApplication::setPalette() matching the Options > Editor Palette behavior (immediate, no restart needed) - Camera speed and clip distances now access viewports via EditorViewport list instead of findChildren<OgreWidget*> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Theme Light/Dark buttons now styled like snap preset buttons (border, dark base, hover highlight, Behavior on color animation) - Writes to both "palette" and "Appearance/theme" keys so both mainwindow.cpp and setSetting() handlers pick it up - Removed Custom option (use Options > Editor Palette for custom colors) - Tab buttons also restyled with model-reset repaint trick - Removed "restart required" text — theme applies immediately Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Camera speed: setCameraSpeed updates base speed; Ctrl press/release uses 10% of base (was hardcoded 0.01/0.1 overwriting any setting) - Recent files count: addToRecentFiles reads General/recentFilesCount from QSettings instead of hardcoded 10 - Removed Default Save Directory option (not wired, not useful) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Use both TransformOperator::getActiveWidget() and findChildren to ensure the speed reaches all viewports. Added null safety for mainWindow access. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
mCameraSpeed previously only affected mouse drag orbit/pan. Now also scales wheel zoom and trackpad pan deltas by mCameraSpeed/0.5 (normalized around the default speed). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Trackpad zoom goes through QNativeGestureEvent → zoomByDelta(), not wheelEvent(). zoomByDelta() now scales the delta by mCameraSpeed/0.5 so the preferences slider affects trackpad zoom too. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Tests added (49 new): - WelcomeScreenController: 18 tests (singleton, recentFiles, shouldShow, dismiss persistence, visibility, action signals) - PropertiesPanelController: 12 new (shortcutData, getSetting/setSetting round-trip, undoHistory/clearUndoHistory) - WelcomeDialog: 3 new (settings edge cases, multi-instance state) - AssetBrowserController: 11 new (extension classification for .x/.dds/.hdr, case-insensitive, materialPreview empty cases, filter by materials) - MaterialPreviewRenderer: 5 new (clearCache, comment lines, quoted names, whitespace-only, multiple materials) Review fixes: - Breadcrumb categories standardized to "ui.action" (was "asset_browser") - WelcomeScreen card dimensions clamped to min 200px (prevents negative) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Ctrl press now sets speed to baseCameraSpeed * 0.1 (was hardcoded 0.01), Ctrl release restores baseCameraSpeed (was hardcoded 0.1). Updated test expectations: 0.01→0.05, 0.1→0.5 for base speed of 0.5. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ControlKeySpeedTransitionCycle (base=1.0): 0.05→0.1, 0.5→1.0 SpeedChangesWithControlKey (base=2.0): 0.05→0.2, 0.5→2.0 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Preferences checkbox now reads/writes "Sentry/enabled" matching what SentryReporter actually uses. The setSetting handler accepts both key names for backward compatibility. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
This was referenced May 8, 2026
6 tasks
5 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
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
Phase 2: UX Polish & Onboarding (#257) — all 6 items complete.
Features
Bonus
Stats
Test plan