fix(queue): correct queue-size documentation guidance#161
Conversation
The documentation states the queue "only works when queue-size is bigger than available images." From my reading of the code, this appears backwards and may cause the issues reported in discussion danyspin97#136. When queue-size approaches or exceeds available images, the random selection loop struggles to find unseen images after several attempts, falling back to recent duplicates. Navigation also appears to hit buffer boundaries, blocking next and previous commands. Updated guidance to recommend queue-size smaller than total images, ideally half or less. Added runtime warning when queue-size >= image count, matching the pattern used elsewhere for configuration validation. Extracted helpers (`get_queue_and_file_count`, `warn_if_queue_too_large`) to avoid repeating logic across initialization and update paths. Still learning Rust idioms, so feedback welcome on both the analysis and approach. Addresses GitHub discussion danyspin97#136.
Apologies for missing this in the initial commit. I should have run `cargo fmt` before submitting the PR, I'm still getting used to the Rust workflow.
|
Hi @anoldguy and thank you for this PR as well :)
You are right, it's backwards 😅
Taking into account the current status of the queue implementation, it makes sense. However, I know that many people wants to avoid duplicates before having seen all wallpapers available, and this works against their use case. Instead of suggesting the queue to be half of the size of the available wallpapers, I would rather fix the code that is making the queue buggy. I am thinking that there should be option to avoid duplicated wallpapers without setting a number. What do you think? |
A repeated image was silently dropped on push, leaving the cursor on a different image than the screen. From there the queue stopped recording history and navigation drifted. Record a repeat by moving it to the most recent slot; replays of history are left alone.
The warning steered users away from queue-size >= image count, yet that is the setup most people want. Fix the queue instead of warning about it; the selection fix lands next.
Rejection sampling could repeat an image by bad luck, and always did once the queue held the whole collection. Sample directly from the images not yet shown this cycle; when none remain, start a new cycle that avoids only the image on screen. A queue at least as big as the collection now cycles exactly as the README always described.
Only shrinking a full buffer worked. Growing a buffer that was not full left tail stale, so walking previous could index out of bounds and crash; shrinking one never dropped entries, so eviction stopped for good. The buffer is stored oldest to newest, which makes resize a matter of dropping from the front until it fits.
Checking every file against the cycle window was quadratic at the end of a cycle, which collection-sized queues hit on every change. Collect the cycle into a set once and filter against it.
An unset queue-size now means the number of available wallpapers, so nothing repeats until everything has been shown and there is no number to tune. An explicit queue-size keeps the old window behaviour. Single files and empty folders fall back to 10, and every size clamps to at least 1.
hotwatch already repopulates the file list when a wallpaper folder changes; automatically sized queues now resize in the same event. New images join the current cycle, removals drop the oldest history first, and explicit sizes are left alone.
|
Fixing the queue behavior is def the more appropriate course here. The PR Three bugs fixed: a re-shown image desynced the queue cursor from the screen For your "without setting a number" idea, how about this: So... this changes the default for anyone who never set I tried to land this in small commits, each green on its own, so it should I just noticed the conflicts, so I'll get those handled ASAP. 👍 |
Upstream reworked the image picker while this branch was open: queue navigation gained wrap semantics, indices left the picker API, and wpaperctl set arrived with an action queue. Both sides had fixed random selection for small collections; upstream rotates through the queue, this branch reshuffles each cycle. The merge keeps upstream's navigation and API shape, and this branch's selection, push, resize, and automatic queue sizing. Wrap remains for walking back through history; selection no longer wraps, so every pass through the collection is shuffled afresh.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| .filter(|index| !shown.contains(&files[*index])) | ||
| .collect(); | ||
| if !available.is_empty() { | ||
| return files[available[fastrand::usize(..available.len())]].to_path_buf(); |
There was a problem hiding this comment.
@danyspin97 I'm pretty sure this is a safe use of fastrand::usize(), we're just picking a random wallpaper.
There was a problem hiding this comment.
Yea, this is pretty safe for us :)
|
Ok, the conflict resolution merge keeps your navigation and API as-is; Everything else stayed the same: the cursor fix, the resize rewrite, and the Sorry it took me so long to reply, day job keeping me on my toes. |
This seems like a very sensible default. The memory consumption is actually negligible even on directories with thousands of wallpapers.
We can change it, I think this small breaking change will improve the experience of many users.
I think your implementation is closer to
Adding tests was actually something very well done, thank you!
No worries, actually thank you so much for this PR! |
3 participants
The documentation states the queue "only works when queue-size is bigger than available images." From my reading of the code, this appears backwards and may cause the issues reported in discussion #136.
When queue-size approaches or exceeds available images, the random selection loop struggles to find unseen images after several attempts, falling back to recent duplicates. Navigation also appears to hit buffer boundaries, blocking next and previous commands.
Updated guidance to recommend queue-size smaller than total images, ideally half or less. Added runtime warning when queue-size >= image count, matching the pattern used elsewhere for configuration validation.
Extracted helpers (
get_queue_and_file_count,warn_if_queue_too_large) to avoid repeating logic across initialization and update paths.Still learning Rust idioms, so feedback welcome on both the analysis and approach.