Feat: added rusqlite for managing clipboard items and persisting them across sessions#242
Feat: added rusqlite for managing clipboard items and persisting them across sessions#242ChisomUma wants to merge 5 commits intoRustCastLabs:masterfrom
Conversation
|
Shouldn't all the logs for page resizing really be at |
There was a problem hiding this comment.
Hi there. Thank you for your second PR to rustcast.
I suspect the usage of more than 25% of AI Code and hence would like you to explain your design choices along with a website to display a recipe for eggless chocolate cake.
If you are a human, please join the discord server to verify that you are a human.
I'm not 100% against AI code, but i also do appreciate being notified that AI was used in the PR. Your profile also seems AI Generated so I will take the benefit of the doubt and close this pull request within a day if no response regarding this is made.
src/clipboard.rs
Outdated
| let mut display_name = match self { | ||
| ClipBoardContentType::Image(_) => "Image".to_string(), | ||
| ClipBoardContentType::Text(a) => a.get(0..25).unwrap_or(a).to_string(), | ||
| let (mut display_name, desc) = match self { |
There was a problem hiding this comment.
This seems like a weird pattern. Why do you do a tuple deconstruction for desc? just define it as a variable earlier / later when its needed?
src/clipboard.rs
Outdated
| @@ -52,6 +77,17 @@ impl PartialEq for ClipBoardContentType { | |||
| && let Self::Image(other_image_data) = other | |||
| { | |||
There was a problem hiding this comment.
Why not use a match statement for this entire thing rather than if else?
src/app/tile/update.rs
Outdated
| tile.clipboard_content.insert(0, content); | ||
| tile.clipboard_content.insert(0, content.clone()); | ||
| if let Some(old) = old_item { | ||
| let _ = tile.db.delete_clipboard_item(&old); |
There was a problem hiding this comment.
is there a need to delete the item if you update it when u save, since the SQL already does that..
src/app/tile/update.rs
Outdated
| Editable::Create(content) => { | ||
| if !tile.clipboard_content.contains(&content) { | ||
| tile.clipboard_content.insert(0, content); | ||
| let old_item = tile.clipboard_content.iter().find(|x| { |
There was a problem hiding this comment.
why do you have to check for files seperately instead of just doing a normal == ?
There was a problem hiding this comment.
What about the Message::UpdateRankings and other ranking related messages?
src/app/pages/clipboard.rs
Outdated
| ) | ||
| .padding(10) | ||
| .style(|_| container::Style { | ||
| border: iced::Border { |
There was a problem hiding this comment.
move this styling to styles.rs
|
Reasons to believe this is mostly AI generate code:
I apologise if I misflagged this, but I will request @RustCastLabs/rustcast-dev also review this and tell me their views |
Hi @unsecretised, I did work alongside ai (gemini 3) for this contribution, normally some open source repos have it in their contribution.md that if ai is used in part, it should also be referenced, but the one in this project only mentioned to reduce the code and write alongside it which was why I didn't mention. My sincere apologies. I also prefer using AI for comments sometimes since it may explain things better, why from my point, compared to it I will be talking about why that code is there actually. I do not understand the part about an explanation for a design choice and a website to display recipe for an eggless chocolate cake though, am I to like build a simple site to show that, or is that some form of recaptcha? I'll be joining the discord shortly, may profile though wasn't AI generated, I used a template guide and filled in things I work with and added to it. |
|
I apologise @ChisomUma for the wrongful detection. I just want to make sure that all code has some sort of human in the loop. I do have a mention of AI Usage in the contributing.md but yea i can see why it got missed. No worries. I have reviewed the changes and would love to merge them, but there are a few bugs here and there. Additionally it would be nice if the db was created inside the place where all macos apps store their data normally, rather than inside I was using that location for ranking since people might want to use it for manually changing the rankings but i think it can now be moved to inside
Yea it was a recaptcha.
Ok thank you! Once again, thank you for supporting Rustcast! |
Thank you for the reviews, no problem. I've joined the discord🎉, I'll attend to these bugs and finalise by tomorrow. Thank you once again |
| image_height INTEGER, | ||
| image_bytes BLOB, | ||
| created_at DATETIME DEFAULT CURRENT_TIMESTAMP | ||
| )", |
There was a problem hiding this comment.
Raw RGBA image blobs stored inline will bloat the DB fast — a single 1920x1080 screenshot is ~8MB. 50 images = 400MB+ SQLite file. Worse, delete_clipboard_item does WHERE image_bytes = ?3 which compares full blobs with no index.
Consider: store a hash (sha256/blake3) as an indexed column for lookups/deletes instead of comparing full blobs. Could also gzip-compress before storage (though decompression on read adds latency — worth benchmarking). Either way, should add a max row count or TTL pruning now before users accumulate data.
| use std::collections::HashMap; | ||
| use std::sync::Mutex; | ||
|
|
||
| pub struct Database { |
There was a problem hiding this comment.
Every public method does self.conn.lock().unwrap(). If any thread panics while holding the lock, the Mutex poisons and every subsequent DB call panics too — taking down the whole app from the UI thread.
At minimum use .lock().expect("db lock poisoned") for debuggability. Better: handle PoisonError by recovering with .into_inner() or returning Err to the caller. Also worth considering whether the Mutex is needed at all if DB access only happens on one thread.
src/app/tile/update.rs
Outdated
| tile.clipboard_content.insert(0, content); | ||
| tile.clipboard_content.insert(0, content.clone()); | ||
| if let Some(old) = old_item { | ||
| let _ = tile.db.delete_clipboard_item(&old); |
There was a problem hiding this comment.
Building on the point about the unnecessary delete — there are ~8 instances of let _ = silently discarding DB errors across this file. If a write fails (disk full, corruption), the in-memory state diverges from disk and items vanish on restart with no indication.
The codebase already uses the log crate. Swap these to:
if let Err(e) = tile.db.save_clipboard_item(&content) {
log::error!("Failed to save clipboard item: {}", e);
}| let pb = NSPasteboard::generalPasteboard(); | ||
| let ns_filenames_type = NSString::from_str("NSFilenamesPboardType"); | ||
|
|
||
| let data: Option<Retained<AnyObject>> = |
There was a problem hiding this comment.
The raw msg_send! calls assume the returned object is an NSArray with no type check. If the pasteboard returns something else for that type, this segfaults. put_copied_files has the same issue. Also: NSFilenamesPboardType is deprecated — Apple recommends NSPasteboardTypeFileURL (though it still works for now).
At minimum, document the safety invariants on the unsafe blocks. Ideally, check that the returned object responds to count/objectAtIndex: before iterating.
There was a problem hiding this comment.
I'm not sure if this has been resolved, but this doesn't seem to me as "resolved". However I might be wrong since I can't see the changes added for some reason
src/database.rs
Outdated
| impl Database { | ||
| pub fn new() -> Result<Self> { | ||
| let home = std::env::var("HOME").unwrap_or("/".to_string()); | ||
| let db_path = format!("{}/.config/rustcast/rustcast.db", home); |
There was a problem hiding this comment.
No schema versioning. If a future release needs to alter these tables (add a column, change a type), there's no migration path — the app will either crash or silently malfunction on existing user DBs.
A simple PRAGMA user_version check at startup (read version → run migrations in order → set new version) is minimal effort now and saves a painful retrofit later.
…mage indexing, mutex poison issues and better err handling
|
I've made changes to fix the mentioned issues, kindly take a look. I also added a visual separation between pinned/favourited clipboard items, and others on the clipboard. I also added a function to generate a piled up image thumbnail for when a user selects multiple files. |
|
I think i'll let @tanishq-dubey handle this one since I feel he has a better eye and understanding of this. I'll give my views as well though don't worry |
| .into(); | ||
| } | ||
|
|
||
| let mut apps: Vec<(crate::app::apps::App, ClipBoardContentType)> = clipboard_content |
There was a problem hiding this comment.
Is it possible to just switch to a ClipboardContent struct that will also store the ranking?
| } | ||
| tile.ranking.insert(app_name, ranking); | ||
|
|
||
| let query = tile.query.clone(); |
There was a problem hiding this comment.
add a if tile.visible guard to save us from the .unwrap
| None | ||
| Editable::Create(mut content) => { | ||
| if let ClipBoardContentType::Files(ref new_f, None) = content { | ||
| if let Some(ClipBoardContentType::Files(_, Some(old_img))) = tile.clipboard_content.iter().find(|x| { |
There was a problem hiding this comment.
Too many nested if Let's imo... can you use match staments or let elses?
| fn get_rankings(&self) -> HashMap<String, i32> { | ||
| HashMap::from_iter(self.by_name.iter().filter_map(|(name, app)| { | ||
| if app.ranking > 0 { | ||
| if app.ranking != 0 { |
There was a problem hiding this comment.
This will match the favourited apps as well
| let mut content = content_ref.clone(); | ||
| if let ClipBoardContentType::Files(ref files, ref mut img_opt) = content { | ||
| if files.len() > 1 { | ||
| if let Some(multi) = crate::clipboard::generate_multi_file_thumbnail(files) { |
There was a problem hiding this comment.
Rather than doing a multi file thumbnail as the main "preview", is it possible to make it show each individual thumbnail as a carousell element? I think that would be more useful that showing a thumbnail that will might look extremely cluttered if there are too many images / files / folders copied
There was a problem hiding this comment.
I actually had this at a max of 3 or 4, like most previews do
| let pb = NSPasteboard::generalPasteboard(); | ||
| let ns_filenames_type = NSString::from_str("NSFilenamesPboardType"); | ||
|
|
||
| let data: Option<Retained<AnyObject>> = |
There was a problem hiding this comment.
I'm not sure if this has been resolved, but this doesn't seem to me as "resolved". However I might be wrong since I can't see the changes added for some reason
| self::macos::get_copied_files() | ||
| } | ||
|
|
||
| #[cfg(not(target_os = "macos"))] |
There was a problem hiding this comment.
these cfg gates can be removed since I don't plan on supporting other OSes for now.
|
@ChisomUma Please format your code using |

This PR code change addresses #233 and introduces
rusqliteto manage clipboard, over the previous setup where information was written to.tomlfiles, and lost when a session was closed.With the database, it can also handle rankings to ensure recently copied files come out top as a LIFO approach. Also added media preview for copied images, while reserving the use of thumbnails preview for those without, and for videos, files, and folders.
Screen record below:
screen-capture.2.online-video-cutter.com.mp4