feat(asset): enhance file dialog with initial directory and output pa…#403
Open
drsnuggles8 wants to merge 1 commit into
Open
feat(asset): enhance file dialog with initial directory and output pa…#403drsnuggles8 wants to merge 1 commit into
drsnuggles8 wants to merge 1 commit into
Conversation
Contributor
📝 WalkthroughWalkthroughThe asset pack builder now opens a save dialog for output selection, validates the chosen pack path, tracks whether the built pack exists, and conditionally reveals that output in the system file manager using new Linux and Windows support. ChangesAsset pack output and reveal flow
Sequence DiagramssequenceDiagram
participant AssetPackBuilderPanel
participant FileDialogs_SaveFile as FileDialogs::SaveFile
participant ValidateOutputPath
AssetPackBuilderPanel->>FileDialogs_SaveFile: SaveFile("*.olopack", initialDirectory)
FileDialogs_SaveFile-->>AssetPackBuilderPanel: selected path or cancel
AssetPackBuilderPanel->>ValidateOutputPath: validate selected path
ValidateOutputPath-->>AssetPackBuilderPanel: accepted path or invalid
sequenceDiagram
participant AssetPackBuilderPanel
participant FileDialogs_ShowInFileManager as FileDialogs::ShowInFileManager
participant LaunchDetached
participant xdg_open as xdg-open
participant ShellExecuteW
AssetPackBuilderPanel->>FileDialogs_ShowInFileManager: ShowInFileManager(m_LastBuildResult.m_OutputPath)
alt Linux
FileDialogs_ShowInFileManager->>LaunchDetached: launch xdg-open target
LaunchDetached->>xdg_open: execvp target
else Windows
FileDialogs_ShowInFileManager->>ShellExecuteW: explore or /select
end
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@OloEditor/src/Panels/AssetPackBuilderPanel.cpp`:
- Around line 150-187: The Asset Pack output-path handling duplicates the same
normalization, buffer copy, and validation flow in both the dialog branch and
the InputText handler. Refactor the repeated logic in
AssetPackBuilderPanel::ValidateOutputPath and the output-path update path into a
small shared helper such as ApplyOutputPath(std::string path), then call it from
both entry points so .olopack normalization, m_OutputPathBuffer writes, and
m_BuildSettings.m_OutputPath updates stay consistent.
- Around line 179-186: The output path handling in AssetPackBuilderPanel is
silently truncating long paths, which can leave the displayed buffer out of sync
with the validated/stored value. Update the logic in the output-path update
paths (including the handler that writes m_OutputPathBuffer and the manual-entry
path) to reject any selected input that does not fit in m_OutputPathBuffer
instead of memcpy’ing a truncated string. Ensure ValidateOutputPath and
m_BuildSettings.m_OutputPath only accept the same exact, non-truncated path that
m_OutputPathBuffer.data() will later provide to StartBuild().
In `@OloEngine/src/Platform/Linux/LinuxPlatformUtils.cpp`:
- Around line 292-304: LaunchDetached currently hides grandchild execvp failures
because the intermediate child exits 0 as soon as the second fork succeeds, so
the caller can’t tell when the actual launch fails. Update LaunchDetached in
LinuxPlatformUtils.cpp to propagate the grandchild’s exec result back to the
parent, for example by using a close-on-exec pipe or another status handoff
between the intermediate child and the grandchild. Make sure ShowInFileManager
can observe a failed launch so its error path for xdg-open can fire when execvp
fails.
In `@OloEngine/src/Platform/Windows/WindowsPlatformUtils.cpp`:
- Around line 102-113: The Windows file-manager launch paths in
WindowsPlatformUtils::OpenInFileManager (the ShellExecuteW calls for opening a
directory or selecting a file) ignore launch failures, so add return-value
checks for both calls and emit a diagnostic when the HINSTANCE result is <= 32;
keep the existing directory-vs-file behavior, but surface a warning/error
consistent with the documented contract and the Linux backend when Explorer
cannot be launched.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0985a97a-f435-4437-83a5-c0ee5051df17
📒 Files selected for processing (4)
OloEditor/src/Panels/AssetPackBuilderPanel.cppOloEngine/src/OloEngine/Utils/PlatformUtils.hOloEngine/src/Platform/Linux/LinuxPlatformUtils.cppOloEngine/src/Platform/Windows/WindowsPlatformUtils.cpp
Comment on lines
+150
to
187
| // Default the dialog to the active project's asset directory, falling | ||
| // back to the directory of whatever path is already in the buffer. | ||
| std::string initialDir; | ||
| if (Project::GetActive()) | ||
| { | ||
| defaultPath = "Assets/AssetPack.olopack"; | ||
| initialDir = Project::GetAssetDirectory().string(); | ||
| } | ||
| else if (std::filesystem::path current(m_OutputPathBuffer.data()); current.has_parent_path()) | ||
| { | ||
| initialDir = current.parent_path().string(); | ||
| } | ||
|
|
||
| // Win32 commdlg double-NUL-terminated filter; the Linux backend parses | ||
| // the same format (see LinuxPlatformUtils::ParseWin32Filter). | ||
| std::string selected = FileDialogs::SaveFile( | ||
| "Asset Pack (*.olopack)\0*.olopack\0", | ||
| initialDir.empty() ? nullptr : initialDir.c_str()); | ||
|
|
||
| // Empty result == user cancelled — leave the current path untouched. | ||
| if (!selected.empty()) | ||
| { | ||
| // Mirror the manual-entry normalisation: append .olopack if missing. | ||
| if (std::filesystem::path fsPath(selected); fsPath.extension() != ".olopack") | ||
| { | ||
| selected += ".olopack"; | ||
| } | ||
|
|
||
| // Write the chosen path into the buffer and run it through the same | ||
| // validation the text field uses, so the error UX is identical. | ||
| sizet len = std::min(selected.length(), m_OutputPathBuffer.size() - 1); | ||
| std::memcpy(m_OutputPathBuffer.data(), selected.c_str(), len); | ||
| m_OutputPathBuffer[len] = '\0'; | ||
|
|
||
| if (ValidateOutputPath(selected, m_OutputPathError)) | ||
| { | ||
| m_BuildSettings.m_OutputPath = std::filesystem::path(selected); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Extension-normalization + buffer-write + validation is duplicated.
This block (Lines 171-186) repeats the logic already in the InputText handler (Lines 121-136): normalize the .olopack extension, copy into m_OutputPathBuffer, then ValidateOutputPath → update m_BuildSettings.m_OutputPath. Extracting a small helper (e.g. ApplyOutputPath(std::string path)) would keep the two entry points from diverging.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@OloEditor/src/Panels/AssetPackBuilderPanel.cpp` around lines 150 - 187, The
Asset Pack output-path handling duplicates the same normalization, buffer copy,
and validation flow in both the dialog branch and the InputText handler.
Refactor the repeated logic in AssetPackBuilderPanel::ValidateOutputPath and the
output-path update path into a small shared helper such as
ApplyOutputPath(std::string path), then call it from both entry points so
.olopack normalization, m_OutputPathBuffer writes, and
m_BuildSettings.m_OutputPath updates stay consistent.
Comment on lines
+179
to
+186
| sizet len = std::min(selected.length(), m_OutputPathBuffer.size() - 1); | ||
| std::memcpy(m_OutputPathBuffer.data(), selected.c_str(), len); | ||
| m_OutputPathBuffer[len] = '\0'; | ||
|
|
||
| if (ValidateOutputPath(selected, m_OutputPathError)) | ||
| { | ||
| m_BuildSettings.m_OutputPath = std::filesystem::path(selected); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Buffer truncation can desync the validated path from the stored buffer.
len truncates selected to m_OutputPathBuffer.size() - 1, but ValidateOutputPath(selected, ...) and m_BuildSettings.m_OutputPath = selected use the full, untruncated string. If the chosen path is longer than the buffer, validation/settings get the full path while the displayed buffer (and the value StartBuild() later reads from m_OutputPathBuffer.data() at Line 360) holds a truncated, possibly invalid path. The same truncation pattern exists in the manual-entry handler, so consider guarding against overflow rather than silently truncating.
🛡️ Reject paths that don't fit the buffer instead of silently truncating
- // Write the chosen path into the buffer and run it through the same
- // validation the text field uses, so the error UX is identical.
- sizet len = std::min(selected.length(), m_OutputPathBuffer.size() - 1);
- std::memcpy(m_OutputPathBuffer.data(), selected.c_str(), len);
- m_OutputPathBuffer[len] = '\0';
-
- if (ValidateOutputPath(selected, m_OutputPathError))
- {
- m_BuildSettings.m_OutputPath = std::filesystem::path(selected);
- }
+ if (selected.length() >= m_OutputPathBuffer.size())
+ {
+ m_OutputPathError = "Selected path is too long";
+ }
+ else
+ {
+ // Write the chosen path into the buffer and run it through the same
+ // validation the text field uses, so the error UX is identical.
+ std::memcpy(m_OutputPathBuffer.data(), selected.c_str(), selected.length());
+ m_OutputPathBuffer[selected.length()] = '\0';
+
+ if (ValidateOutputPath(selected, m_OutputPathError))
+ {
+ m_BuildSettings.m_OutputPath = std::filesystem::path(selected);
+ }
+ }📝 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
| sizet len = std::min(selected.length(), m_OutputPathBuffer.size() - 1); | |
| std::memcpy(m_OutputPathBuffer.data(), selected.c_str(), len); | |
| m_OutputPathBuffer[len] = '\0'; | |
| if (ValidateOutputPath(selected, m_OutputPathError)) | |
| { | |
| m_BuildSettings.m_OutputPath = std::filesystem::path(selected); | |
| } | |
| if (selected.length() >= m_OutputPathBuffer.size()) | |
| { | |
| m_OutputPathError = "Selected path is too long"; | |
| } | |
| else | |
| { | |
| // Write the chosen path into the buffer and run it through the same | |
| // validation the text field uses, so the error UX is identical. | |
| std::memcpy(m_OutputPathBuffer.data(), selected.c_str(), selected.length()); | |
| m_OutputPathBuffer[selected.length()] = '\0'; | |
| if (ValidateOutputPath(selected, m_OutputPathError)) | |
| { | |
| m_BuildSettings.m_OutputPath = std::filesystem::path(selected); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@OloEditor/src/Panels/AssetPackBuilderPanel.cpp` around lines 179 - 186, The
output path handling in AssetPackBuilderPanel is silently truncating long paths,
which can leave the displayed buffer out of sync with the validated/stored
value. Update the logic in the output-path update paths (including the handler
that writes m_OutputPathBuffer and the manual-entry path) to reject any selected
input that does not fit in m_OutputPathBuffer instead of memcpy’ing a truncated
string. Ensure ValidateOutputPath and m_BuildSettings.m_OutputPath only accept
the same exact, non-truncated path that m_OutputPathBuffer.data() will later
provide to StartBuild().
Comment on lines
+292
to
+304
| if (pid == 0) | ||
| { | ||
| // Intermediate child: fork again and exit immediately so the grandchild | ||
| // is orphaned onto init (PID 1), which reaps it — the parent never has | ||
| // to wait on the file manager itself. | ||
| const pid_t grandchild = ::fork(); | ||
| if (grandchild == 0) | ||
| { | ||
| ::execvp(cargv[0], cargv.data()); | ||
| _exit(127); // only reached if exec failed | ||
| } | ||
| _exit(grandchild < 0 ? 127 : 0); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
LaunchDetached reports success even when execvp fails in the grandchild.
The parent only reaps the intermediate child (which _exit(0) once the second fork() succeeds), so a failed execvp (_exit(127) at Line 301) is never observed by the caller — ShowInFileManager's "failed to launch xdg-open" error (Line 492-493) can therefore never fire for the most likely failure mode. This is acceptable for fire-and-forget, but if surfacing exec failures matters, the intermediate child should propagate the grandchild's launch result (e.g. via a close-on-exec pipe) rather than always exiting 0.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@OloEngine/src/Platform/Linux/LinuxPlatformUtils.cpp` around lines 292 - 304,
LaunchDetached currently hides grandchild execvp failures because the
intermediate child exits 0 as soon as the second fork succeeds, so the caller
can’t tell when the actual launch fails. Update LaunchDetached in
LinuxPlatformUtils.cpp to propagate the grandchild’s exec result back to the
parent, for example by using a close-on-exec pipe or another status handoff
between the intermediate child and the grandchild. Make sure ShowInFileManager
can observe a failed launch so its error path for xdg-open can fire when execvp
fails.
Comment on lines
+102
to
+113
| if (std::filesystem::is_directory(canonical, ec) && !ec) | ||
| { | ||
| // Open the directory itself in a new Explorer window. | ||
| ::ShellExecuteW(nullptr, L"explore", canonical.wstring().c_str(), nullptr, nullptr, SW_SHOWNORMAL); | ||
| } | ||
| else | ||
| { | ||
| // Select the file inside its parent folder. The path is quoted because | ||
| // /select treats everything after the comma as the (possibly spaced) path. | ||
| const std::wstring args = L"/select,\"" + canonical.wstring() + L"\""; | ||
| ::ShellExecuteW(nullptr, L"open", L"explorer.exe", args.c_str(), nullptr, SW_SHOWNORMAL); | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
ShellExecuteW results are not checked — failures are silent on Windows.
Both ShellExecuteW calls ignore the return value. The header contract documents a "warning when ... no file manager is available," and the Linux backend logs on launch failure, but here a failed launch produces no diagnostic. ShellExecuteW returns an HINSTANCE whose value <= 32 indicates failure; checking it would keep behavior consistent across platforms.
🔍 Log on launch failure
if (std::filesystem::is_directory(canonical, ec) && !ec)
{
// Open the directory itself in a new Explorer window.
- ::ShellExecuteW(nullptr, L"explore", canonical.wstring().c_str(), nullptr, nullptr, SW_SHOWNORMAL);
+ const HINSTANCE rc = ::ShellExecuteW(nullptr, L"explore", canonical.wstring().c_str(), nullptr, nullptr, SW_SHOWNORMAL);
+ if (reinterpret_cast<INT_PTR>(rc) <= 32)
+ OLO_CORE_WARN("ShowInFileManager: failed to open '{}' (code {})", canonical.string(), reinterpret_cast<INT_PTR>(rc));
}
else
{
// Select the file inside its parent folder. The path is quoted because
// /select treats everything after the comma as the (possibly spaced) path.
const std::wstring args = L"/select,\"" + canonical.wstring() + L"\"";
- ::ShellExecuteW(nullptr, L"open", L"explorer.exe", args.c_str(), nullptr, SW_SHOWNORMAL);
+ const HINSTANCE rc = ::ShellExecuteW(nullptr, L"open", L"explorer.exe", args.c_str(), nullptr, SW_SHOWNORMAL);
+ if (reinterpret_cast<INT_PTR>(rc) <= 32)
+ OLO_CORE_WARN("ShowInFileManager: failed to reveal '{}' (code {})", canonical.string(), reinterpret_cast<INT_PTR>(rc));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (std::filesystem::is_directory(canonical, ec) && !ec) | |
| { | |
| // Open the directory itself in a new Explorer window. | |
| ::ShellExecuteW(nullptr, L"explore", canonical.wstring().c_str(), nullptr, nullptr, SW_SHOWNORMAL); | |
| } | |
| else | |
| { | |
| // Select the file inside its parent folder. The path is quoted because | |
| // /select treats everything after the comma as the (possibly spaced) path. | |
| const std::wstring args = L"/select,\"" + canonical.wstring() + L"\""; | |
| ::ShellExecuteW(nullptr, L"open", L"explorer.exe", args.c_str(), nullptr, SW_SHOWNORMAL); | |
| } | |
| if (std::filesystem::is_directory(canonical, ec) && !ec) | |
| { | |
| // Open the directory itself in a new Explorer window. | |
| const HINSTANCE rc = ::ShellExecuteW(nullptr, L"explore", canonical.wstring().c_str(), nullptr, nullptr, SW_SHOWNORMAL); | |
| if (reinterpret_cast<INT_PTR>(rc) <= 32) | |
| OLO_CORE_WARN("ShowInFileManager: failed to open '{}' (code {})", canonical.string(), reinterpret_cast<INT_PTR>(rc)); | |
| } | |
| else | |
| { | |
| // Select the file inside its parent folder. The path is quoted because | |
| // /select treats everything after the comma as the (possibly spaced) path. | |
| const std::wstring args = L"/select,\"" + canonical.wstring() + L"\""; | |
| const HINSTANCE rc = ::ShellExecuteW(nullptr, L"open", L"explorer.exe", args.c_str(), nullptr, SW_SHOWNORMAL); | |
| if (reinterpret_cast<INT_PTR>(rc) <= 32) | |
| OLO_CORE_WARN("ShowInFileManager: failed to reveal '{}' (code {})", canonical.string(), reinterpret_cast<INT_PTR>(rc)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@OloEngine/src/Platform/Windows/WindowsPlatformUtils.cpp` around lines 102 -
113, The Windows file-manager launch paths in
WindowsPlatformUtils::OpenInFileManager (the ShellExecuteW calls for opening a
directory or selecting a file) ignore launch failures, so add return-value
checks for both calls and emit a diagnostic when the HINSTANCE result is <= 32;
keep the existing directory-vs-file behavior, but surface a warning/error
consistent with the documented contract and the Linux backend when Explorer
cannot be launched.
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.
…th validation
Summary by CodeRabbit