Files App - Copy, Cut, Paste Actions#510
Files App - Copy, Cut, Paste Actions#510Shadowtrance wants to merge 6 commits intoTactilityProject:mainfrom
Conversation
and some bonus symbol exports
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds clipboard-based file operations (copy, cut, paste) to the Files app: new State fields and APIs, View UI elements and handlers, unified showActions flow, paste implementation with recursive copy/move helpers and conflict checks, and UI/state refresh after operations. Appends LVGL module symbols (lv_obj_set_style_min_height, lv_obj_set_style_max_height, lv_group_get_editing, lv_anim_path_ease_out, lv_anim_path_ease_in, lv_async_call). Exports libc symbols strcasecmp and strncasecmp. Removes a documentation TODO entry and an unused include. 🚥 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)
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: 4
🧹 Nitpick comments (4)
Modules/lvgl-module/source/symbols.c (1)
413-414: Consider reversing theease_in/ease_outordering for consistency.Both
lv_anim_path_ease_inandlv_anim_path_ease_outare valid, confirmed LVGL path callbacks. However,ease_outis listed beforeease_in— the reverse of the convention used bylv_anim_path_ease_in_outdirectly above. Minor cosmetic nit.✨ Proposed reorder
- DEFINE_MODULE_SYMBOL(lv_anim_path_ease_out), - DEFINE_MODULE_SYMBOL(lv_anim_path_ease_in), + DEFINE_MODULE_SYMBOL(lv_anim_path_ease_in), + DEFINE_MODULE_SYMBOL(lv_anim_path_ease_out),Tactility/Private/Tactility/app/files/State.h (1)
77-78:pending_paste_dstaccessors lack mutex protection unlike sibling clipboard methods.
setClipboard/getClipboard/hasClipboard/clearClipboardall acquire the mutex, butgetPendingPasteDst()andsetPendingPasteDst()do not, even though they participate in the same cross-callback paste workflow (set inonPastePressed, read inonResult). This is likely safe today because both run on the UI thread, but the inconsistency could bite if the threading model changes. Consider either wrapping these inmutex.withLockfor consistency or adding a comment documenting the single-thread assumption.Tactility/Source/app/files/View.cpp (2)
350-378:showActionsForDirectoryandshowActionsForFileshare nearly identical code.Both methods now have four identical button creations (Copy, Cut, Rename, Delete). Consider extracting common action buttons into a shared helper, adding file-specific buttons as needed.
658-663: Lock acquired onsrc, but the existence check is againstdst— the lock doesn't protect the TOCTOU gap on the destination.
file::getLock(src)guards the source path, whilestat(dst.c_str(), &st)checks the destination. Betweenlock->unlock()(Line 663) and the actual write insidedoPaste, another operation could create a file atdst. On a single-user embedded device this is low-risk, but the lock here gives a false sense of safety. Consider either lockingdstinstead (or additionally), or removing the source lock from this check since it doesn't protect anything meaningful here.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
Modules/lvgl-module/source/symbols.c (1)
411-414: Optional: reorderease_in/ease_outbeforeease_in_outThe existing
lv_anim_path_ease_in_out(line 411) was inserted beforeease_inandease_out, producing a slightly counter-intuitive read order. Consider reordering for consistency:♻️ Proposed reorder
- DEFINE_MODULE_SYMBOL(lv_anim_path_ease_in_out), DEFINE_MODULE_SYMBOL(lv_anim_path_linear), + DEFINE_MODULE_SYMBOL(lv_anim_path_ease_in), + DEFINE_MODULE_SYMBOL(lv_anim_path_ease_out), + DEFINE_MODULE_SYMBOL(lv_anim_path_ease_in_out), - DEFINE_MODULE_SYMBOL(lv_anim_path_ease_in), - DEFINE_MODULE_SYMBOL(lv_anim_path_ease_out),Tactility/Source/app/files/View.cpp (1)
124-143:copyRecursiveleaves a partial destination directory on failure.
copyFileContentscorrectly removes a partial file on error (line 119). But whencopyRecursivefails mid-directory traversal, the partially-populated destination directory (and any already-copied children) are not removed. The caller never cleans up the partial tree.Consider rolling back on failure, e.g.:
♻️ Proposed fix
static bool copyRecursive(const std::string& src, const std::string& dst) { if (file::isDirectory(src)) { if (!file::findOrCreateDirectory(dst, 0755)) { return false; } bool success = true; file::listDirectory(src, [&](const dirent& entry) { if (!success) return; if (strcmp(entry.d_name, ".") == 0 || strcmp(entry.d_name, "..") == 0) return; auto child_src = file::getChildPath(src, entry.d_name); auto child_dst = file::getChildPath(dst, entry.d_name); if (!copyRecursive(child_src, child_dst)) { success = false; } }); + if (!success) { + file::deleteRecursively(dst); // remove partial copy + } return success; } else { return copyFileContents(src, dst); } }
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Tactility/Source/app/files/View.cpp (1)
539-544:pendingActionnot reset before the "Rename failed" informational dialog — inconsistent withdoPastepattern.When the destination already exists,
alertdialog::start("Rename failed", ...)is shown butpendingActionremainsActionRename. On dismiss,onResultre-entersActionRenameand callsinputdialog::getResulton an alertdialog bundle; if that returns empty string the guard!new_name.empty()saves it — but this is fragile.doPasteconsistently callsstate->setPendingAction(State::ActionNone)before any informational (non-choice) alert. Apply the same pattern here:♻️ Proposed fix
if (stat(rename_to.c_str(), &st) == 0) { LOGGER.warn("Rename: destination already exists: \"{}\"", rename_to); lock->unlock(); + state->setPendingAction(State::ActionNone); alertdialog::start("Rename failed", "\"" + new_name + "\" already exists."); break; }
1 participant
Files App - Copy, Cut, Paste Actions added + overwrite detection alert prompt.
and some bonus symbol exports
Summary by CodeRabbit
New Features
Improvements
Documentation