Remove debug logs, add Docker and GitHub Action docs - #186
Conversation
- Remove ~60 qDebug() statements from MaterialEditorQML file dialogs (left over from debugging dialog visibility issues) - Add Docker section to README.md with usage examples and Docker Hub link - Add GitHub Action usage example to README.md - Add Docker and GitHub Action quick start sections to docs/index.html - Add Docker section to CLAUDE.md with key files, usage, and notes - Update CI/CD section in CLAUDE.md to mention Docker image publishing Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughDocumentation additions introduce Docker deployment and GitHub Actions CI/CD examples. UI code changes remove debug logging, standardize file-dialog parenting and filename extraction, add an Ogre-availability helper, and wire an AI material-generation lifecycle (start/progress/complete/error) into the material editor. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant UI as MaterialEditorQML (UI)
participant LLM as LLM Service/Model
participant Ogre as Ogre Renderer
User->>UI: Request "Generate Material" with prompt
UI->>LLM: generateMaterialFromPrompt(prompt)
LLM-->>UI: llmModelLoaded (status)
LLM-->>UI: progress updates (llmGenerationProgress)
UI->>Ogre: validate/apply generated material (guarded by isOgreAvailable)
LLM-->>UI: generation finished / error
UI-->>User: emit completion / error signals
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 814e7796ae
ℹ️ 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".
| docker run --rm -v $(pwd):/workspace ghcr.io/fernandotonon/qtmesh info model.fbx --json | ||
|
|
||
| # Convert between formats | ||
| docker run --rm -v $(pwd):/workspace ghcr.io/fernandotonon/qtmesh convert model.fbx -o model.gltf2 |
There was a problem hiding this comment.
This command pattern is documented for conversions that produce output files, but the container image runs as a non-root user (USER qtmesh), so on Linux bind mounts where $(pwd) is owned by a different UID/GID (the common case) writes to /workspace will fail with permission errors. That means users following these examples for convert, fix, or anim may be unable to create output files unless they add user mapping (for example --user "$(id -u):$(id -g)") or pre-adjust directory permissions.
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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 173-179: The workflow example uses an unpinned action reference
"uses: fernandotonon/QtMeshEditor/.github/actions/qtmesh@master"; replace the
`@master` ref with a stable release tag or a commit SHA (e.g., `@v1.2.3` or
@<commit-sha>) to make the example reproducible, and apply the same change to
the duplicate snippet in docs/index.html referenced in the comment so both
locations use the identical pinned ref.
In `@src/MaterialEditorQML.cpp`:
- Around line 2389-2395: The file dialogs are created with nullptr as the parent
and showNativeFileDialog ignores its parentWindow, which lets the picker attach
to the wrong top-level window; update each
QFileDialog::getOpenFileName/getSaveFileName call in MaterialEditorQML (e.g.,
the QString selectedFile = QFileDialog::getOpenFileName(...) site) to pass the
invoking window as the parent (use the MaterialEditorQML instance or its
window() / parentWidget()), and modify showNativeFileDialog to accept and use
the passed parentWindow (or forward the correct QWidget/QWindow pointer) so all
dialog invocations (including the other occurrences noted) are parented to the
material editor window.
- Around line 2389-2399: The code currently calls QFileDialog::getOpenFileName
storing into selectedFile and then returns QFileInfo(selectedFile).fileName(),
which strips the directory; change this to return the full selectedFile path
instead (i.e., return selectedFile) so texture pickers preserve the source
directory, and apply the same change for the other occurrence that also uses
QFileDialog::getOpenFileName and QFileInfo(...).fileName(); keep deriving the
basename only where Ogre/resource-loading logic explicitly requires a resource
name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b478575a-fe78-4231-8398-7588d32345c3
📒 Files selected for processing (4)
CLAUDE.mdREADME.mddocs/index.htmlsrc/MaterialEditorQML.cpp
| ```yaml | ||
| - uses: fernandotonon/QtMeshEditor/.github/actions/qtmesh@master | ||
| with: | ||
| command: info | ||
| input-file: assets/player.fbx | ||
| options: --json | ||
| ``` |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does GitHub Actions documentation recommend pinning action references to a commit SHA or release tag instead of a branch name like @master?
💡 Result:
Yes. GitHub’s Actions security guidance recommends pinning third-party actions to a full-length commit SHA as the only way to use an immutable version, and notes that tags are more convenient but can be moved or deleted (so they’re less secure than SHAs). This guidance is specifically aimed at avoiding mutable refs like branches (for example, @master/@main). [1][2]
Sources:
[1] GitHub Docs – “Security hardening for GitHub Actions” (Secure use reference) (docs.github.com)
[2] GitHub Well-Architected – “Securing GitHub Actions Workflows” (wellarchitected.github.com)
Citations:
- 1: https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions
- 2: https://wellarchitected.github.com/library/application-security/recommendations/actions-security/?utm_source=openai
🏁 Script executed:
#!/bin/bash
# Check if README.md exists and view lines 173-179
echo "=== Checking README.md lines 173-179 ==="
if [ -f README.md ]; then
sed -n '173,179p' README.md
else
echo "README.md not found"
fi
echo ""
echo "=== Searching for `@master` in README.md ==="
rg '@master' README.md -n -B2 -A2
echo ""
echo "=== Checking if docs/index.html exists ==="
if [ -f docs/index.html ]; then
echo "docs/index.html found"
echo "=== Searching for `@master` or similar pattern in docs/index.html ==="
rg '@master|\.github/actions/qtmesh' docs/index.html -n -B2 -A2
else
echo "docs/index.html not found"
fiRepository: fernandotonon/QtMeshEditor
Length of output: 774
Pin the documented action ref.
Using @master makes downstream workflows non-reproducible and lets future unrelated changes break consumers. Prefer a release tag or a commit SHA here. The same snippet in docs/index.html (line 849) should be updated too.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 173 - 179, The workflow example uses an unpinned
action reference "uses:
fernandotonon/QtMeshEditor/.github/actions/qtmesh@master"; replace the `@master`
ref with a stable release tag or a commit SHA (e.g., `@v1.2.3` or @<commit-sha>)
to make the example reproducible, and apply the same change to the duplicate
snippet in docs/index.html referenced in the comment so both locations use the
identical pinned ref.
| QString selectedFile = QFileDialog::getOpenFileName( | ||
| nullptr, // no parent to avoid issues | ||
| "Select Texture File", | ||
| startDir, | ||
| nullptr, | ||
| "Select Texture File", | ||
| startDir, | ||
| "Image files (*.jpg *.jpeg *.png *.dds *.tga *.bmp);;All files (*)", | ||
| nullptr, // no selected filter | ||
| QFileDialog::DontUseNativeDialog | QFileDialog::DontUseCustomDirectoryIcons // Force Qt dialog with simpler display | ||
| nullptr, | ||
| QFileDialog::DontUseNativeDialog | QFileDialog::DontUseCustomDirectoryIcons | ||
| ); | ||
|
|
||
| qDebug() << "QFileDialog finished, result:" << selectedFile; | ||
|
|
||
|
|
||
| if (!selectedFile.isEmpty()) { | ||
| qDebug() << "SUCCESS! File selected:" << selectedFile; | ||
| QString fileName = QFileInfo(selectedFile).fileName(); | ||
| qDebug() << "Extracted filename:" << fileName; | ||
| qDebug() << "=== openFileDialog SUCCESS ==="; | ||
| return fileName; | ||
| return QFileInfo(selectedFile).fileName(); |
There was a problem hiding this comment.
Return the full selected path from the texture pickers.
Stripping selectedFile down to fileName() loses the source directory, so textures chosen outside the default resource dir become ambiguous or impossible to resolve later. Keep the full path here and derive the basename only where Ogre actually needs a resource name.
Proposed fix
- return QFileInfo(selectedFile).fileName();
+ return selectedFile;
...
- return QFileInfo(selectedFile).fileName();
+ return selectedFile;Also applies to: 2497-2507
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MaterialEditorQML.cpp` around lines 2389 - 2399, The code currently calls
QFileDialog::getOpenFileName storing into selectedFile and then returns
QFileInfo(selectedFile).fileName(), which strips the directory; change this to
return the full selectedFile path instead (i.e., return selectedFile) so texture
pickers preserve the source directory, and apply the same change for the other
occurrence that also uses QFileDialog::getOpenFileName and
QFileInfo(...).fileName(); keep deriving the basename only where
Ogre/resource-loading logic explicitly requires a resource name.
- Pin GitHub Action ref to @2.11.1 instead of @master in docs - Parent file dialogs to QApplication::activeWindow() instead of nullptr - Use parentWindow argument in showNativeFileDialog with fallback chain - Add --user "$(id -u):$(id -g)" to Docker write examples for correct file ownership on Linux bind mounts - Remove debug console.log and testConnection() call from QML Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/MaterialEditorQML.cpp (2)
2398-2399:⚠️ Potential issue | 🟠 MajorPreserve the selected texture path here.
These helpers still collapse the picker result to
fileName(). That drops the source directory before the caller can add a resource location or import the asset, so textures chosen outside the existing resource folders stop resolving.Suggested change
- return QFileInfo(selectedFile).fileName(); + return selectedFile;Also applies to: 2509-2510
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MaterialEditorQML.cpp` around lines 2398 - 2399, The helper currently collapses the picker result to QFileInfo(selectedFile).fileName(), which strips the source directory; change both occurrences (the one using selectedFile and the similar one at the 2509-2510 location) to return or propagate the full selectedFile path (or QFileInfo(selectedFile).absoluteFilePath()) instead of fileName() so callers keep the original directory for resource location/import resolution.
2390-2395:⚠️ Potential issue | 🟡 MinorQt Quick callers can still end up with an unparented picker.
These sites still derive the parent from
QApplication::activeWindow()/qobject_cast<QWidget *>only. This file opens the editor as aQQuickWindow, so when the picker is invoked from that window it can still attach to the wrong top-level window or appear behind the editor.In Qt 6, does QApplication::activeWindow() only track QWidget windows, and what is the recommended way to parent or transient-parent a QFileDialog when the caller is a QQuickWindow/QWindow instead of a QWidget?Also applies to: 2423-2428, 2463-2468, 2486-2501
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MaterialEditorQML.cpp` around lines 2390 - 2395, The picker currently derives its parent only via QApplication::activeWindow() / qobject_cast<QWidget *>, which fails for QQuickWindow/QWindow callers (the editor), so replace the static QFileDialog::getOpenFileName usage with an explicit QFileDialog instance: determine QWidget* wp = qobject_cast<QWidget*>(QApplication::activeWindow()) and QWindow* qw = qobject_cast<QWindow*>(QApplication::activeWindow()) or use QGuiApplication::focusWindow(); if wp is non-null pass it as the dialog parent, otherwise construct the QFileDialog with nullptr and after creation call dialog->windowHandle()->setTransientParent(qw) (using QWindow::setTransientParent) and then exec() so the file picker is correctly transient to QQuickWindow; update all occurrences (the calls around MaterialEditorQML.cpp lines using QFileDialog::getOpenFileName) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/MaterialEditorQML.cpp`:
- Around line 2398-2399: The helper currently collapses the picker result to
QFileInfo(selectedFile).fileName(), which strips the source directory; change
both occurrences (the one using selectedFile and the similar one at the
2509-2510 location) to return or propagate the full selectedFile path (or
QFileInfo(selectedFile).absoluteFilePath()) instead of fileName() so callers
keep the original directory for resource location/import resolution.
- Around line 2390-2395: The picker currently derives its parent only via
QApplication::activeWindow() / qobject_cast<QWidget *>, which fails for
QQuickWindow/QWindow callers (the editor), so replace the static
QFileDialog::getOpenFileName usage with an explicit QFileDialog instance:
determine QWidget* wp = qobject_cast<QWidget*>(QApplication::activeWindow()) and
QWindow* qw = qobject_cast<QWindow*>(QApplication::activeWindow()) or use
QGuiApplication::focusWindow(); if wp is non-null pass it as the dialog parent,
otherwise construct the QFileDialog with nullptr and after creation call
dialog->windowHandle()->setTransientParent(qw) (using
QWindow::setTransientParent) and then exec() so the file picker is correctly
transient to QQuickWindow; update all occurrences (the calls around
MaterialEditorQML.cpp lines using QFileDialog::getOpenFileName) accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7c224400-0263-49f9-844c-1bfd9b42f140
📒 Files selected for processing (4)
README.mddocs/index.htmlqml/TexturePropertiesPanel.qmlsrc/MaterialEditorQML.cpp
💤 Files with no reviewable changes (1)
- qml/TexturePropertiesPanel.qml
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/index.html
- README.md
|
1 participant
Summary
qDebug()statements fromMaterialEditorQML.cppfile dialog methods (debug markers like=== FIXED openFileDialog START ===)Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit