-
Notifications
You must be signed in to change notification settings - Fork 340
feat: Add image paste support and key binding #3088
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| #[derive(Debug)] | ||
| struct PasteStateInner { | ||
| paths: Vec<PathBuf>, | ||
| count: usize, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is there a separate paths and count?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The count field is used to generate the marker text [Image #N] but this probably can be simplified with paths.len()
| })?; | ||
|
|
||
| // Try to guess format from raw bytes, fallback to PNG | ||
| let format = guess_format(&image_data.bytes).unwrap_or(ImageFormat::Png); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a regression from the previous implementation? Checking the clipboard docs it looks like clipboard content is always encoded as a list of rgba values - https://docs.rs/arboard/latest/arboard/struct.ImageData.html
This will always just be png compatible, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good callout - let me go back to that approach. You're right, it's always rgba values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless I'm missing something, these tests don't seem to be verifying anything relevant in this PR? What are these doing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding more tests to existing image handling, primarily. If not desired, I can remove
|
Hi Josh. Thanks for the contribution. If I understand this correctly it looks this is:
I think the api client already support image types so we can probably just use that instead of relying on another round trip. What do you think? |
Issue #, if available: #2339
Description of changes:
Current state
Image support required saving a screenshot and referencing as a file in chat
New state
Image from clipboard is copied to a tmp directory and can be referenced directly via CTRL+V keybinding or a new
/paste commandto be read in chat. Multiple images are supported (in the case of CTRL+V).CTRL+V example:
/pasteexample:Multiple images example:
Help details (showing
pastecommand)Error cases
Large images (> 10MB)

Too many images (>10)

No image
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.