feat: Prefer ".sleep" over "sleep" for custom image directory#948
feat: Prefer ".sleep" over "sleep" for custom image directory#948jonasdiemer merged 3 commits intocrosspoint-reader:masterfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughSleep Screen documentation was expanded with mode, cover, and custom-image guidance. Code now prefers Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
USER_GUIDE.md (1)
188-189: Consider clarifying the fallback behavior.Line 189 states
sleep.bmpis "only used if no.sleepdirectory is found," but the code also falls back tosleep.bmpwhen the directory exists but contains no valid BMP files (or if loading fails). Consider rewording to:Single Image: Place a file named
sleep.bmpin the root directory. This is used as a fallback if no valid images are found in the sleep directory.This is a minor point—the current wording is acceptable for most users.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@USER_GUIDE.md` around lines 188 - 189, Update the USER_GUIDE.md wording for the "Single Image" case to clarify fallback behavior: change the sentence that reads "This is only used if no `.sleep` directory is found" to indicate that `sleep.bmp` is used as a fallback when no valid BMP images are found in the `.sleep` directory (including when the directory exists but is empty or loading fails), referencing the `.sleep` directory and `sleep.bmp` terms so it's easy to locate and replace the line.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
USER_GUIDE.mdsrc/activities/boot_sleep/SleepActivity.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: cppcheck
🔇 Additional comments (3)
src/activities/boot_sleep/SleepActivity.cpp (2)
35-48: LGTM! Directory lookup logic is well-structured.The two-phase lookup correctly prefers
/.sleepover/sleep, handles all resource cleanup paths properly (closing the first handle before opening the second, and ensuring cleanup at line 102 for non-successful paths), and only proceeds with file iteration when a valid directory is found.
88-91: LGTM!Path construction and logging correctly use the resolved
sleepDirvariable, making it clear which directory the image was loaded from during debugging.USER_GUIDE.md (1)
166-175: LGTM! Comprehensive mode table.The mode table clearly documents all six sleep screen options with their fallback behaviors, which accurately reflects the code implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@USER_GUIDE.md`:
- Around line 188-189: Update the USER_GUIDE.md wording for the "Single Image"
case to clarify fallback behavior: change the sentence that reads "This is only
used if no `.sleep` directory is found" to indicate that `sleep.bmp` is used as
a fallback when no valid BMP images are found in the `.sleep` directory
(including when the directory exists but is empty or loading fails), referencing
the `.sleep` directory and `sleep.bmp` terms so it's easy to locate and replace
the line.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
USER_GUIDE.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cppcheck
- GitHub Check: build
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@USER_GUIDE.md`:
- Around line 181-182: The doc uses two different labels for the same filter:
"Inverted Contrast" in the Sleep Screen Cover Filter list and "Inverted" in the
Settings section; pick one label and make it consistent across the guide by
updating the Sleep Screen Cover Filter entry (or the Settings entry) so both use
the same exact option name ("Inverted" or "Inverted Contrast") and verify any
other occurrences of "Inverted" or "Inverted Contrast" (e.g., the "Sleep Screen
Cover Filter" heading and the Settings list) are identical.
|
conflicts with #887 which allows for images to be viewed from the file browser |
|
If we merge this I assure you there will be feedback from users who preferred it the old way 😄 |
That's the reason I made it backward compatible, it will read the old directory |
|
If we merge this in, I suggest adding a way to show/hide hidden folders and files directly from the file manager from hotspot/network (in a quick or convenient way). I currently use this file manager to activate and deactivate sleep covers quickly, without having to remove the SD card and use a computer to move the files. My current workflow is: To activate a sleep cover, I move it from a custom folder called To deactivate a sleep cover, I move it from the I know I’m probably the only one doing this, but if we can toggle hidden folders/files on and off, I’ll still be able to manage my sleep covers on the go, without needing to remove the SD card from the Xteink. This probably help more users in the future to be more comfortable with the system |
|
@elvisgastelum |
Yeah i understood, i just suggest that feature because I would like to migrate to a dot folder instead of sleep, i see the .sleep pretty useful when im looking for the books in the internal file explorer and I don’t want to see any other noisy files or folders |
Will support both directory structures |
|
If |
|
Does this have any blockers? |
| // Check if we have a /.sleep (preferred) or /sleep directory | ||
| const char* sleepDir = nullptr; | ||
| auto dir = Storage.open("/.sleep"); | ||
| if (dir && dir.isDirectory()) { | ||
| sleepDir = "/.sleep"; | ||
| } else { | ||
| if (dir) dir.close(); | ||
| dir = Storage.open("/sleep"); | ||
| if (dir && dir.isDirectory()) { | ||
| sleepDir = "/sleep"; | ||
| } | ||
| } |
There was a problem hiding this comment.
Small thought to reduce repetition here:
| // Check if we have a /.sleep (preferred) or /sleep directory | |
| const char* sleepDir = nullptr; | |
| auto dir = Storage.open("/.sleep"); | |
| if (dir && dir.isDirectory()) { | |
| sleepDir = "/.sleep"; | |
| } else { | |
| if (dir) dir.close(); | |
| dir = Storage.open("/sleep"); | |
| if (dir && dir.isDirectory()) { | |
| sleepDir = "/sleep"; | |
| } | |
| } | |
| // Check if we have a /.sleep (preferred) or /sleep directory | |
| static constexpr const char* kSleepDirs[] = {"/.sleep", "/sleep"}; | |
| const char* sleepDir = nullptr; | |
| FsFile dir; | |
| for (const auto* candidate : kSleepDirs) { | |
| dir = Storage.open(candidate); | |
| if (dir && dir.isDirectory()) { | |
| sleepDir = candidate; | |
| break; | |
| } | |
| if (dir) dir.close(); | |
| } |
7 participants
Summary
Additional Context
AI Usage
While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it
helps set the right context for reviewers.
Did you use AI tools to help write this code? NO