Featue: Add portable open- and save-file dialog.#29
Merged
kimkulling merged 5 commits intomainfrom Apr 11, 2026
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughConst-correctness and local-scope refinements across core and SDL renderer; added cross-platform file dialog APIs (open/save) in Widgets; small SDL init/cleanup change and sample init-statement cleanups. Changes
Sequence Diagram(s)sequenceDiagram
participant W as Widgets
participant Core as TinyUi/Core
participant OS as OperatingSystem
W->>Core: getOpenFileDialog(title, extensions, filename)
Core->>OS: invoke platform dialog (WinAPI or zenity)
OS-->>Core: dialog result (path or cancel)
Core-->>W: return ret_code and filename (if selected)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 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 docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tinyui.h`:
- Around line 241-248: The merge logic uses top.x + r.width and bottom.y +
r.height which derives from the current rect instead of r.top; change the
computed bounds to use r.top.x and r.top.y when calculating x2_ and y2_ (i.e.,
compare against r's top-based edges), update bottom/top accordingly, and then
recompute width and height as bottom.x - top.x and bottom.y - top.y (replace the
assignments width = r.width / height = r.height). Update the code paths that use
x2_ and y2_ (variables x2_ and y2_) and the width/height assignments to follow
this new calculation.
- Around line 76-79: Remove the Win32 includes from the public header tinyui.h
and add them only in the implementation file that needs them: delete the
conditional `#include` block for windows.h/Commdlg.h from tinyui.h, and instead
add a conditional `#include` for Windows.h and Commdlg.h (use the exact casing) at
the top of src/widgets.cpp so the Win32 dialog implementations
(getOpenFileDialog and getSaveFileDialog) can access the Win32 APIs without
polluting all consumers; keep the includes guarded by `#ifdef` TINYUI_WINDOWS to
preserve cross-platform builds.
In `@src/widgets.cpp`:
- Line 842: The build error comes from using an undefined type alias `c8`;
replace every occurrence of `c8` with the standard `char` (e.g., change the
declarations like `c8 buffer[BufferSize] = { '\0' };` and the other `c8`
declaration later) so the code compiles on non-Windows systems; update both
declarations referencing `c8` in this file (the buffer variable and the second
`c8` usage) to `char`.
- Around line 843-849: The Zenity branches misuse popen modes, omit pclose,
ignore title/extensions, fail to strip trailing newlines, and lack error
handling: change the save-dialog popen to read mode ("r") instead of "w", add
the flags "--save --confirm-overwrite" to the save command, include optional
"--title" and "--file-filter" (or "--file-filter='EXT'") when title/extensions
are provided for both open and save commands, check f for nullptr and verify
fgets/feof results before using buffer, strip a trailing '\n' from buffer before
assigning to filename, call pclose(f) on all control paths, and propagate/return
false on errors. Apply these fixes around the zenity popen/fgets/pclose usage
where filename, buffer, BufferSize, and f are referenced.
- Around line 813-840: The Windows and Linux dialog implementations have three
issues: define the missing alias c8 (e.g., using c8 = char;) so non-Windows
builds compile; in Widgets::getOpenFileDialog and getSaveFileDialog fix the
OPENFILENAME lpstrFilter usage by converting the incoming extensions string into
the double-null-terminated Windows filter format and call the explicit ANSI or
wide API consistently (use GetOpenFileNameA/GetSaveFileNameA with char buffers
or the W variants with wide strings) to match your buffer types; and in the
Linux path for getSaveFileDialog change the popen mode to "r" when reading
zenity output (or use the same read mode used in getOpenFileDialog) so fgets()
can read the result.
In `@src/widgets.h`:
- Around line 341-353: The two dialog APIs in Widgets (getOpenFileDialog and
getSaveFileDialog) must return a ret_code (int32_t) instead of bool so callers
can distinguish selected/cancelled/failed; change the declarations of
getOpenFileDialog(const char*, const char*, std::string&) and
getSaveFileDialog(const char*, const char*, std::string&) to return int32_t (or
the project's ret_code typedef), update their documentation comments to describe
the ret_code semantics (e.g., RET_OK/RET_CANCEL/RET_FAIL), update all callers to
handle the new ret_code values, and ensure the implementations map existing bool
outcomes to the appropriate ret_code constants and include any required header
for ret_code/int32_t.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 907b5fa3-9d47-4680-b5c5-6f95520678f7
📒 Files selected for processing (8)
samples/demo/main.cppsamples/hello_world/main.cppsrc/backends/sdl2_renderer.cppsrc/backends/sdl2_renderer.hsrc/tinyui.cppsrc/tinyui.hsrc/widgets.cppsrc/widgets.h
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/widgets.cpp (1)
584-588:⚠️ Potential issue | 🔴 Critical
fillRateis read from uninitialized payload bytes.Line 584 treats
payload->payloadas aFilledState, butWidgets::progressBaronly copiesEventPayload.typeintomContenton Lines 510-514. The first render can therefore read garbage and draw an arbitrary width.🔧 Suggested fix spanning the writer and reader paths
- FilledState state; - state.filledState = fillRate; child->mContent = new uint8_t[sizeof(EventPayload)]; child->mCallback = callback; - EventPayload payload; + EventPayload payload{}; payload.type = EventDataType::FillState; - memcpy(child->mContent, &payload, sizeof(EventPayload)); + auto *storedState = reinterpret_cast<FilledState *>(payload.payload); + storedState->filledState = fillRate; + memcpy(child->mContent, &payload, sizeof(payload));- const auto *state = reinterpret_cast<FilledState *>(payload->payload); + const auto *state = reinterpret_cast<const FilledState *>(payload->payload);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/widgets.cpp` around lines 584 - 588, The code reads a FilledState from payload->payload in Widgets::progressBar (using const auto *state = reinterpret_cast<FilledState *>(payload->payload) and then reading state->filledState) even though the writer only copied EventPayload.type into mContent; fix by ensuring the writer and reader agree on the payload layout: either update the writer that populates mContent to memcpy/serialize the entire FilledState (not just EventPayload.type) when emitting this event, or change the reader in Widgets::progressBar to validate payload size and safely deserialize into a local FilledState (e.g., check payload->size >= sizeof(FilledState) and memcpy into a local variable) before using filledState; reference FilledState, Widgets::progressBar, mContent, and EventPayload.type when making the change.
♻️ Duplicate comments (1)
src/widgets.cpp (1)
813-849:⚠️ Potential issue | 🔴 CriticalThe new dialog helpers still have cross-platform blockers.
Line 828/870 passes
extensionsstraight tolpstrFilter, but Win32 expects double-null-terminated filter pairs, and the genericGetOpenFileName/GetSaveFileNamemacros do not matchcharbuffers in UNICODE builds. On the non-Windows side, Line 842 uses undefinedc8, Line 888 uses undefinedcchar, and Line 884 opens a write-only pipe before reading it withfgets(). Both Zenity branches also skipfgets()/pclose()result handling, keep the trailing newline, and ignoretitle/extensions, so the implementation does not match the public API yet.#!/bin/bash set -e sed -n '811,895p' src/widgets.cpp | cat -n rg -n '\b(c8|cchar)\b' src/ --type cpp --type h rg -n 'lpstrFilter|GetOpenFileName|GetSaveFileName|popen|fgets|pclose' src/widgets.cppWhat format does OPENFILENAMEA::lpstrFilter require for GetOpenFileNameA/GetSaveFileNameA, and do the generic GetOpenFileName/GetSaveFileName macros work with char buffers when UNICODE is enabled?Also applies to: 854-892
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/widgets.cpp` around lines 813 - 849, Widgets::getOpenFileDialog currently passes extensions directly to ofn.lpstrFilter and uses the ANSI macros that break in UNICODE builds, references undefined types c8/cchar, and mishandles popen/fgets/pclose and returned newline/title/extension handling on non-Windows; fix by (1) on Windows explicitly use OPENFILENAMEA and GetOpenFileNameA when working with char buffers (or convert to wchar_t and use OPENFILENAMEW/GetOpenFileNameW), (2) build lpstrFilter as a double-null-terminated pair string ("Description\0*.ext\0\0") and assign it to ofn.lpstrFilter, (3) replace undefined c8/cchar with plain char (or std::string buffer), (4) when using popen call with "r", check fgets return for null, strip trailing newline before assigning filename, call pclose and check its result, and (5) include title and extensions when constructing the zenity command (e.g. --title and --file-filter) while escaping inputs; locate fixes around Widgets::getOpenFileDialog, ofn.lpstrFilter, GetOpenFileName/GetOpenFileNameA/GetOpenFileNameW, popen/fgets/pclose, and the local buffer variables.
🧹 Nitpick comments (1)
src/widgets.cpp (1)
813-815: Considerret_codehere instead ofbool.These new public APIs currently collapse cancel, OS/API failure, and validation errors into the same
falseresult. Returningret_codewould align them with the rest of the surface and let callers distinguish a canceled dialog from an actual failure. As per coding guidelines, "Use ret_code (int32_t) for return values" and "Return error codes for recoverable errors instead of throwing exceptions".Also applies to: 854-856
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/widgets.cpp` around lines 813 - 815, Change the public APIs from returning bool to returning ret_code (int32_t) so callers can distinguish success, user-cancel, and failures; specifically update the signature of Widgets::getOpenFileDialog and the analogous getSaveFileDialog to return ret_code, update all internal return points to return RET_OK on success, RET_CANCEL (or defined cancel code) when the user cancels, and an appropriate error code (e.g., RET_FAIL) for OS/API/validation failures, and adjust any callers to handle these ret_code values accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/widgets.cpp`:
- Around line 584-588: The code reads a FilledState from payload->payload in
Widgets::progressBar (using const auto *state = reinterpret_cast<FilledState
*>(payload->payload) and then reading state->filledState) even though the writer
only copied EventPayload.type into mContent; fix by ensuring the writer and
reader agree on the payload layout: either update the writer that populates
mContent to memcpy/serialize the entire FilledState (not just EventPayload.type)
when emitting this event, or change the reader in Widgets::progressBar to
validate payload size and safely deserialize into a local FilledState (e.g.,
check payload->size >= sizeof(FilledState) and memcpy into a local variable)
before using filledState; reference FilledState, Widgets::progressBar, mContent,
and EventPayload.type when making the change.
---
Duplicate comments:
In `@src/widgets.cpp`:
- Around line 813-849: Widgets::getOpenFileDialog currently passes extensions
directly to ofn.lpstrFilter and uses the ANSI macros that break in UNICODE
builds, references undefined types c8/cchar, and mishandles popen/fgets/pclose
and returned newline/title/extension handling on non-Windows; fix by (1) on
Windows explicitly use OPENFILENAMEA and GetOpenFileNameA when working with char
buffers (or convert to wchar_t and use OPENFILENAMEW/GetOpenFileNameW), (2)
build lpstrFilter as a double-null-terminated pair string
("Description\0*.ext\0\0") and assign it to ofn.lpstrFilter, (3) replace
undefined c8/cchar with plain char (or std::string buffer), (4) when using popen
call with "r", check fgets return for null, strip trailing newline before
assigning filename, call pclose and check its result, and (5) include title and
extensions when constructing the zenity command (e.g. --title and --file-filter)
while escaping inputs; locate fixes around Widgets::getOpenFileDialog,
ofn.lpstrFilter, GetOpenFileName/GetOpenFileNameA/GetOpenFileNameW,
popen/fgets/pclose, and the local buffer variables.
---
Nitpick comments:
In `@src/widgets.cpp`:
- Around line 813-815: Change the public APIs from returning bool to returning
ret_code (int32_t) so callers can distinguish success, user-cancel, and
failures; specifically update the signature of Widgets::getOpenFileDialog and
the analogous getSaveFileDialog to return ret_code, update all internal return
points to return RET_OK on success, RET_CANCEL (or defined cancel code) when the
user cancels, and an appropriate error code (e.g., RET_FAIL) for
OS/API/validation failures, and adjust any callers to handle these ret_code
values accordingly.
|
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 by CodeRabbit
New Features
Bug Fixes
Refactor
Chores