Repository navigation
Feature/core/1 handling file management - #9
Conversation
… the document struct.
… involves doc queue mutation and queries. Design the DocumentQueue here.
…ck string. Point to note: Ui takes in the AppModel by ref and writes into the outputpathBuffer text field. This is the only instance where the UI mutates the model. All other mutations flow through UiEvents and dispatch.
…lication. The plan is simple: UI never mutates domain model directly, only reports user intents.
…Model, and renders it's actual state.
…y DOCX to PDFs, this could change in the future, so the Output text is left broad.
…rper. Fix compile error with invalid file comparison for std::ranges::transform.
There was a problem hiding this comment.
Pull request overview
Adds Doculith’s first interactive “file management” layer by introducing a domain-level document queue, wiring native file dialogs through Application, and extending the UI to display/manage queued files and output path selection.
Changes:
- Introduces
DocumentandDocumentQueuecore types (add/remove/move/clear, status + thumbnail metadata, suggested output path). - Wires native file open/save dialogs (NFD) into
Application::dispatch()and updates UI to render/manage the queue and output destination. - Adds Catch2 unit tests for
DocumentQueueand updates CMake to build/link core logic for tests.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
CMakeLists.txt |
Adds DocumentQueue.cpp to app and introduces doculith_core library target |
README.md |
Updates project structure heading text |
src/AppModel.h |
Adds queue + output path buffer + feedback + refreshOutputPath() |
src/AppStrings.h |
Renames “Output PDF” label to “Output Destination” |
src/Application.h |
Adds event handler declarations and a filesystem alias |
src/Application.cpp |
Implements dispatch handlers for add/browse/clear/remove and queue move placeholder |
src/Document.h |
Adds Document value type and ConversionStatus enum |
src/DocumentQueue.h |
Declares DocumentQueue API and AddResult |
src/DocumentQueue.cpp |
Implements queue invariant enforcement, mutation, and status helpers |
src/platform/ImGuiContextGuard.cpp |
Initializes/quits NFD alongside ImGui context lifetime |
src/Ui.h |
Makes render() take AppModel& and splits main panel rendering into helper functions |
src/Ui.cpp |
Renders file list (status + remove) and controls panel (output path, feedback, merge enable) |
src/UiEvents.h |
Adds optional remove/move indices to UI event payload |
tests/CMakeLists.txt |
Adds document_queue_test.cpp and switches test linking to doculith_core |
tests/document_queue_test.cpp |
Adds unit tests for DocumentQueue behaviors |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -30,13 +30,22 @@ include(cmake/Dependencies.cmake) | |||
| add_executable(doculith | |||
| src/main.cpp | |||
| src/Application.cpp | |||
| src/DocumentQueue.cpp | |||
| src/Ui.cpp | |||
| src/platform/GlfwWindow.cpp | |||
| src/platform/ImGuiContextGuard.cpp | |||
| src/platform/PlatformContext.cpp | |||
|
|
|||
| ) | |||
|
|
|||
| add_library(doculith_core | |||
| src/DocumentQueue.cpp | |||
| ) | |||
There was a problem hiding this comment.
This CMake change builds DocumentQueue.cpp into both the doculith executable and the new doculith_core library. That duplicates compilation and can drift over time. Consider linking doculith against doculith_core and removing DocumentQueue.cpp from the executable sources so the core logic is built once.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| @@ -11,7 +12,13 @@ namespace Doculith | |||
| bool addFilesClicked = false; | |||
| bool browseOutputClicked = false; | |||
| bool clearAllClicked = false; | |||
|
|
|||
| std::optional<std::size_t> removeAtIndex; // Set when user clicks remove | |||
|
|
|||
There was a problem hiding this comment.
UiEvents uses std::size_t but this header only includes . This can fail to compile depending on include order (size_t is not guaranteed to be provided by ). Add an explicit #include (or <stddef.h>) here to keep the header self-contained.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| #include "DocumentQueue.h" | ||
| #include <algorithm> | ||
|
|
||
| namespace Doculith | ||
| { | ||
| namespace fs = std::filesystem; | ||
|
|
||
| AddResult DocumentQueue::addFiles(const std::vector<std::filesystem::path>& paths) | ||
| { | ||
| AddResult result; | ||
|
|
||
| for (const auto& rawPath : paths) | ||
| { | ||
| // Must be a regular file that exists | ||
| std::error_code ec; | ||
| if (!fs::exists(rawPath, ec) || !fs::is_regular_file(rawPath, ec)) | ||
| { | ||
| ++result.rejected; | ||
| continue; | ||
| } | ||
|
|
||
| // Canonicalise | ||
| const fs::path canonical = fs::canonical(rawPath, ec); | ||
| if (ec) { ++result.rejected; continue; } | ||
|
|
||
| // Must have DOCX extension (case-insensitive) | ||
| auto ext = canonical.extension().string(); | ||
| std::ranges::transform(ext, ext.begin(), | ||
| [](unsigned char c) -> char | ||
| { | ||
| return static_cast<char>(std::tolower(c)); | ||
| } | ||
| ); | ||
| if (ext != ".docx") { ++result.rejected; continue; } |
There was a problem hiding this comment.
DocumentQueue.cpp uses std::tolower but doesn't include . This is undefined/fragile depending on transitive includes. Add the missing standard include(s) (at least ; also consider <system_error> if relying on std::error_code definitions).
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| #pragma once | ||
| #include "DocumentQueue.h" | ||
|
|
||
| namespace Doculith | ||
| { | ||
| struct AppModel | ||
| { | ||
|
|
||
| DocumentQueue queue; | ||
|
|
||
| // Output path buffer, owned by AppModel, written to by ImGui::InputText. | ||
| // Ui::render() takes AppModel& (non-const) specifically to allow this. | ||
| char outputPathBuf[512] = {}; | ||
|
|
||
| // Human readable status message shown below the control panel. | ||
| std::string addFeedback; | ||
|
|
||
| // Called by Application::dispatch() after files are successfully added. | ||
| // Populate outputPathBuf from DocumentQueue::suggestedOutputPath() | ||
| void refreshOutputPath() | ||
| { | ||
| const auto suggested = queue.suggestedOutputPath(); | ||
| if (suggested.has_value() && outputPathBuf[0] == '\0') | ||
| { | ||
| const auto str = suggested->string(); | ||
| const auto len = std::min(str.size(), sizeof(outputPathBuf) - 1u); | ||
| std::memcpy(outputPathBuf, str.data(), len); | ||
| outputPathBuf[len] = '\0'; | ||
| } |
There was a problem hiding this comment.
AppModel now uses std::string, std::min, and std::memcpy, but this header doesn't include the corresponding standard headers. With -Werror builds, relying on transitive includes will break on some compilers. Please add the needed includes (e.g., , , ) so AppModel.h is self-contained.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
|
@sromic1990 I've opened a new pull request, #10, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@sromic1990 I've opened a new pull request, #11, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@sromic1990 I've opened a new pull request, #12, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: sromic1990 <13477194+sromic1990@users.noreply.github.com> Agent-Logs-Url: https://github.com/sromic1990/Doculith/sessions/8b57e503-717e-4206-8c12-7a8df273dcb2
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…pModel.h Co-authored-by: sromic1990 <13477194+sromic1990@users.noreply.github.com> Agent-Logs-Url: https://github.com/sromic1990/Doculith/sessions/75c0e793-22fe-4ef3-91c9-c148549d160b
|
@sromic1990 I've opened a new pull request, #15, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@sromic1990 I've opened a new pull request, #16, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@sromic1990 I've opened a new pull request, #17, to work on those changes. Once the pull request is ready, I'll request review from you. |
… link doculith against it Co-authored-by: sromic1990 <13477194+sromic1990@users.noreply.github.com> Agent-Logs-Url: https://github.com/sromic1990/Doculith/sessions/d7f5b5c9-65ce-4df8-b81a-4a32c71ebd5d
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@sromic1990 I've opened a new pull request, #18, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: sromic1990 <13477194+sromic1990@users.noreply.github.com> Agent-Logs-Url: https://github.com/sromic1990/Doculith/sessions/84e9cacb-1e77-45cc-b292-1618fdd21214
…ncurrent test collisions Co-authored-by: sromic1990 <13477194+sromic1990@users.noreply.github.com> Agent-Logs-Url: https://github.com/sromic1990/Doculith/sessions/ac8a38bd-ec1c-4ed3-bfa1-f88aa3624ed0
…-451f-bd80-be1efbbf3081 [WIP] [WIP] Address feedback on file management handling in Core feature PR
Co-authored-by: sromic1990 <13477194+sromic1990@users.noreply.github.com> Agent-Logs-Url: https://github.com/sromic1990/Doculith/sessions/2ddb8e64-9579-4945-9a3e-ed0f0001aba6
…-47b7-9ef8-364edc2dde7a [WIP] [WIP] Address feedback on file management handling in core feature PR
…-48dc-b35d-e06993d4278f [WIP] [WIP] Address feedback on file management implementation in core feature PR
[WIP] [WIP] Address feedback on file management handling in core feature PR
Fix missing `#include <cstddef>` in UiEvents.h
Eliminate duplicate compilation of DocumentQueue.cpp
Make AppModel.h self-contained by adding missing standard headers
Fix missing `#include <cctype>` in DocumentQueue.cpp
Summary
Implements the file selection layer for Doculith. Users can now pick
.docxfiles through the OS's native file picker, queue them for conversion, choose an output PDF path via a native save dialog, and manage the queue (remove individual files, clear all). This is the first chapter where the app becomes genuinely interactive.What's Changed
New:
DocumentQueueCore domain type that owns the list of files queued for conversion. Enforces a strict invariant: all entries must be unique, canonical
.docxpaths sharing a common parent directory.addFiles()validates every candidate against this invariant in a documented 7-step pipeline and returns anAddResultwith human-readable feedback. Also exposesremoveAt(),move()(for drag-and-drop, Chapter 9),clear(), and status/thumbnail mutators.New:
DocumentLightweight value type holding
sourcePath,pdfPath,displayName,ConversionStatus, andthumbnailTextureId. Serves as the single source of truth for per-file state throughout the pipeline.Modified:
AppModelExtended with
DocumentQueue queue,outputPathBuf[512],addFeedback, andrefreshOutputPath(). The output path buffer is the one place in the codebase where ImGui writes directly into the model — intentional, bounded, and documented.Modified:
UiEventsAdded
removeAtIndex,moveFromIndex, andmoveToIndex(allstd::optional<size_t>). The move fields are declared as a placeholder for Chapter 9; remove is live.Modified:
Uirender()is nowAppModel&(non-const, forInputText). AddedrenderFileList()(status dots, per-row remove with deferred mutation) andrenderControlsPanel()(live output path display, feedback label, Clear All).Modified:
Applicationdispatch()now routes all four live events to private handlers.onAddFilesClicked()callsNFD::OpenDialogMultiplewith a.docxfilter.onBrowseOutputClicked()callsNFD::SaveDialogwith a.pdffilter and amerged.pdfdefault name. NFD is never called fromUi— platform-level calls belong inApplication.Modified:
ImGuiContextGuardNFD::Init()added to constructor;NFD::Quit()added at the top of the destructor (before ImGui shutdown — correct teardown order).Architecture Notes
Uinever touches the domain model directly. It observesAppModel(read) and emitsUiEvents(write-intent). All mutations flow throughdispatch().renderFileList()records the index to remove afterEndChild()— not mid-loop — to avoid iterator invalidation.Init/Quitlive inImGuiContextGuardso the dialog is always called with a live GL context beneath it.Tests
17 unit tests in
tests/document_queue_tests.cppcovering:.docxextension rejectionremoveAt()bounds and mid-list removalmove()reorder correctnessclear()resets to emptyallConverted()withstd::all_ofCI runs on push and pull request via GitHub Actions (Windows + macOS).
Files Changed