Fix ViewCube z-order, enable SD in CI, lighter background, snap CLI#200
Fix ViewCube z-order, enable SD in CI, lighter background, snap CLI#200fernandotonon merged 1 commit intomasterfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughEnabled Stable Diffusion in CI for Linux and macOS, bumped project version to 2.15.1 and pinned SD dependency commit, adjusted OGRE viewport background color, changed ViewCube widget window flags on Linux/Windows, extended CLI description in snap, and expanded CLI-mode detection in startup arguments. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/deploy.yml:
- Line 223: The repo currently configures stable-diffusion.cpp as an unpinned
git submodule in CMakeLists (GIT_TAG master) which makes release builds
non-reproducible; update the CMake add_subdirectory or ExternalProject_Add entry
that uses GIT_TAG for stable-diffusion.cpp to reference a specific tag or commit
hash (e.g., GIT_TAG vX.Y.Z or GIT_TAG <commit-hash>) and do the same for the
llama_cpp entry that also uses GIT_TAG unpinned, ensuring the build option
ENABLE_STABLE_DIFFUSION remains unchanged but now references fixed revisions.
In `@src/ViewCube/ViewCubeController.cpp`:
- Around line 92-98: The unconditional call to raise() in
updateWidgetVisibility() undoes the intended lower() behavior applied to
m_cubeWidget on Linux/Windows; change updateWidgetVisibility() so that raise()
is only called on non-Linux/Windows builds (i.e., guard the raise() with the
same platform check used where lower() is applied), ensuring m_cubeWidget
remains lower()ed on X11/Wayland and Windows while preserving current behavior
on other platforms and when setVisible(true) triggers visibility updates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c9c5bad4-6c5f-4c32-8fc6-fe15cd7643e8
📒 Files selected for processing (5)
.github/workflows/deploy.ymlCMakeLists.txtsnap/snapcraft.yamlsrc/OgreWidget.cppsrc/ViewCube/ViewCubeController.cpp
.github/workflows/deploy.yml
Outdated
| gcc --version | ||
| cmake --version | ||
| cmake -S . -DCMAKE_BUILD_TYPE=${{env.BUILD_TYPE}} -DCMAKE_CXX_FLAGS="-g" -DCMAKE_C_FLAGS="-g" -DQT_QMAKE_EXECUTABLE=qmake -DCMAKE_C_COMPILER="D:/a/QtMeshEditor/Qt/Tools/mingw1310_64/bin/gcc.exe" -DCMAKE_CXX_COMPILER="D:/a/QtMeshEditor/Qt/Tools/mingw1310_64/bin/g++.exe" -DCMAKE_EXE_LINKER_FLAGS=-static -DQt6_DIR=D:/a/QtMeshEditor/Qt/${{env.QT_VERSION}}/mingw_64/lib/cmake/Qt6 -DQT_DIR=D:/a/QtMeshEditor/Qt/${{env.QT_VERSION}}/mingw_64/lib/cmake/Qt6 -DQt6GuiTools_DIR=D:/a/QtMeshEditor/Qt/${{env.QT_VERSION}}/mingw_64/lib/cmake/Qt6GuiTools -DOGRE_DIR=${{github.workspace}}/ogre-build/SDK/CMake/ -DASSIMP_DIR=C:/PROGRA~2/Assimp/lib/cmake/assimp-${{ env.ASSIMP_DIR_VERSION }} | ||
| cmake -S . -DCMAKE_BUILD_TYPE=${{env.BUILD_TYPE}} -DCMAKE_CXX_FLAGS="-g" -DCMAKE_C_FLAGS="-g" -DENABLE_STABLE_DIFFUSION=ON -DQT_QMAKE_EXECUTABLE=qmake -DCMAKE_C_COMPILER="D:/a/QtMeshEditor/Qt/Tools/mingw1310_64/bin/gcc.exe" -DCMAKE_CXX_COMPILER="D:/a/QtMeshEditor/Qt/Tools/mingw1310_64/bin/g++.exe" -DCMAKE_EXE_LINKER_FLAGS=-static -DQt6_DIR=D:/a/QtMeshEditor/Qt/${{env.QT_VERSION}}/mingw_64/lib/cmake/Qt6 -DQT_DIR=D:/a/QtMeshEditor/Qt/${{env.QT_VERSION}}/mingw_64/lib/cmake/Qt6 -DQt6GuiTools_DIR=D:/a/QtMeshEditor/Qt/${{env.QT_VERSION}}/mingw_64/lib/cmake/Qt6GuiTools -DOGRE_DIR=${{github.workspace}}/ogre-build/SDK/CMake/ -DASSIMP_DIR=C:/PROGRA~2/Assimp/lib/cmake/assimp-${{ env.ASSIMP_DIR_VERSION }} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm workflow enables SD and CMake fetches from an unpinned branch.
rg -n -C2 'ENABLE_STABLE_DIFFUSION=ON' .github/workflows/deploy.yml
rg -n -C4 'stable_diffusion_cpp|GIT_TAG' CMakeLists.txtRepository: fernandotonon/QtMeshEditor
Length of output: 2867
🏁 Script executed:
# Check the full conditional block in CMakeLists.txt
sed -n '230,260p' CMakeLists.txt
# Check if stable-diffusion.cpp is properly guarded in source files
rg -n '#ifdef.*ENABLE_STABLE_DIFFUSION' --type cpp --type hpp
# Look for includes of stable-diffusion headers
rg -n '#include.*stable' --type cpp --type hpp | head -20
# Check the default state of ENABLE_STABLE_DIFFUSION option
rg -n 'option.*ENABLE_STABLE_DIFFUSION|set.*ENABLE_STABLE_DIFFUSION' CMakeLists.txtRepository: fernandotonon/QtMeshEditor
Length of output: 1326
🏁 Script executed:
# Retry with correct syntax - search for ifdef guards
rg -n '#ifdef.*ENABLE_STABLE_DIFFUSION'
# Search for stable-diffusion includes
rg -n '#include.*stable.*diffusion'
# Search for usage of stable-diffusion classes/functions
rg -n 'stable_diffusion|StableDiffusion' --max-count 10Repository: fernandotonon/QtMeshEditor
Length of output: 2556
Pin stable-diffusion.cpp revision in CMakeLists.txt before deploying with it enabled.
With deploy.yml lines 223, 491, and 1210 setting -DENABLE_STABLE_DIFFUSION=ON in release builds, the builds depend on whatever master points to at configure time. CMakeLists.txt line 244 uses GIT_TAG master without a pinned commit or release tag, making release artifacts non-reproducible and vulnerable to upstream breakage.
Change GIT_TAG master to a pinned tag or commit hash (e.g., GIT_TAG v1.2.3 or GIT_TAG abc123def456).
Note: llama_cpp at line 201 has the same unpinned dependency issue.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/deploy.yml at line 223, The repo currently configures
stable-diffusion.cpp as an unpinned git submodule in CMakeLists (GIT_TAG master)
which makes release builds non-reproducible; update the CMake add_subdirectory
or ExternalProject_Add entry that uses GIT_TAG for stable-diffusion.cpp to
reference a specific tag or commit hash (e.g., GIT_TAG vX.Y.Z or GIT_TAG
<commit-hash>) and do the same for the llama_cpp entry that also uses GIT_TAG
unpinned, ensuring the build option ENABLE_STABLE_DIFFUSION remains unchanged
but now references fixed revisions.
src/ViewCube/ViewCubeController.cpp
Outdated
| #if defined(Q_OS_LINUX) || defined(Q_OS_WIN) | ||
| // On X11/Wayland and Windows, Qt::Tool windows stay on top of all app windows | ||
| // by default. Remove WindowStaysOnTopHint explicitly and lower the window so | ||
| // it doesn't float above dialogs, material editor, and other OS windows. | ||
| m_cubeWidget->setWindowFlags(m_cubeWidget->windowFlags() & ~Qt::WindowStaysOnTopHint); | ||
| m_cubeWidget->lower(); | ||
| #endif |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the lower/raise call chain affecting ViewCube z-order.
rg -n -C3 'm_cubeWidget->(lower|raise|show)\(' src/ViewCube/ViewCubeController.cpp
rg -n -C5 'setVisible\(true\)' src/mainwindow.cppRepository: fernandotonon/QtMeshEditor
Length of output: 3148
Platform-conditional raise() needed in updateWidgetVisibility()—current unconditional raise() undoes the Linux/Windows z-order fix.
The lower() call at line 97 is negated when setVisible(true) is invoked (mainwindow.cpp:391), which triggers updateWidgetVisibility() to unconditionally call raise() at line 306. On Linux/Windows, this counteracts the intended behavior of keeping the widget below other application windows.
Guard the raise() call in updateWidgetVisibility() with the platform condition:
Proposed fix
void ViewCubeController::updateWidgetVisibility()
{
if (isVisible()) {
reposition();
m_cubeWidget->show();
- m_cubeWidget->raise();
+#if defined(Q_OS_LINUX) || defined(Q_OS_WIN)
+ m_cubeWidget->lower();
+#else
+ m_cubeWidget->raise();
+#endif
} else {
m_cubeWidget->hide();
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ViewCube/ViewCubeController.cpp` around lines 92 - 98, The unconditional
call to raise() in updateWidgetVisibility() undoes the intended lower() behavior
applied to m_cubeWidget on Linux/Windows; change updateWidgetVisibility() so
that raise() is only called on non-Linux/Windows builds (i.e., guard the raise()
with the same platform check used where lower() is applied), ensuring
m_cubeWidget remains lower()ed on X11/Wayland and Windows while preserving
current behavior on other platforms and when setVisible(true) triggers
visibility updates.
![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: 0c248512dd
ℹ️ 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".
| run: | | ||
| sudo cmake -S . -DCMAKE_BUILD_TYPE=${{env.BUILD_TYPE}} \ | ||
| -DCMAKE_CXX_FLAGS="-g" -DCMAKE_C_FLAGS="-g" \ | ||
| -DENABLE_STABLE_DIFFUSION=ON \ |
There was a problem hiding this comment.
Enabling -DENABLE_STABLE_DIFFUSION=ON here makes the release workflow pull stable-diffusion.cpp through FetchContent, and CMakeLists.txt:241-245 pins that dependency to GIT_TAG master. Because this code path was previously off in CI, the change makes Windows/Linux/macOS release builds non-reproducible: any upstream push can now break packaging or silently change shipped behavior without a corresponding change in this repo. Please pin a specific tag or commit before turning this on in deploy.yml.
Useful? React with 👍 / 👎.
src/ViewCube/ViewCubeController.cpp
Outdated
| m_cubeWidget->setWindowFlags(m_cubeWidget->windowFlags() & ~Qt::WindowStaysOnTopHint); | ||
| m_cubeWidget->lower(); |
There was a problem hiding this comment.
lower() to fix the ViewCube stacking bug
This Linux/Windows branch does not actually change the cube's z-order in the way the comment describes. The widget is created with only Qt::FramelessWindowHint | Qt::Tool, so clearing WindowStaysOnTopHint is a no-op, and m_cubeWidget is still a top-level QQuickWidget with no parent. Qt documents QWidget::lower() as only reordering a widget within its parent/sibling stack, so this call will not move the cube behind dialogs or other top-level windows; the reported always-on-top behavior can still reproduce on those platforms.
Useful? React with 👍 / 👎.
fernandotonon
force-pushed
the
fix/misc-issues-2.15.1
branch
3 times, most recently
from
816f536 to
b6605ab
Compare
- Fix ViewCube staying on top of other windows on Linux and Windows by removing WindowStaysOnTopHint and lowering the window after creation - Enable ENABLE_STABLE_DIFFUSION=ON in CI deploy builds for all three platforms (Linux, macOS, Windows) so released binaries include SD - Lighten viewport background from 0.18 to 0.27 (less dark) - Document snap CLI usage in snapcraft.yaml description: users need qtmesheditor.qtmesh or can create an alias with sudo snap alias - Bump version to 2.15.1 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
fernandotonon
force-pushed
the
fix/misc-issues-2.15.1
branch
from
b6605ab to
0427ea3
Compare
|
1 participant
Summary
WindowStaysOnTopHintandlower()the window after creation on Linux and Windows. macOS already had NSWindow level lowering.-DENABLE_STABLE_DIFFUSION=ONto all three platform builds (Linux, macOS, Windows) indeploy.yml0.18to0.27(was too dark)qtmeshcommand not found: The snap exposesqtmesheditor.qtmesh(not bareqtmesh). Added usage docs to snap description. Users can create an alias:sudo snap alias qtmesheditor.qtmesh qtmeshTest plan
qtmesheditor.qtmesh info model.fbxworks🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Style
Documentation
Chores