Skip to content
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

Implement getting HTML on X11 #130

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions examples/get_html.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
use arboard::Clipboard;

fn main() {
let mut ctx = Clipboard::new().unwrap();

let html_data = ctx.get_html().unwrap();

Check warning on line 7 in examples/get_html.rs

View workflow job for this annotation

GitHub Actions / rustfmt

Diff in /home/runner/work/arboard/arboard/examples/get_html.rs
println!("HTML data is:\n{:?}\nAlt text is:\n{:?}", html_data.html, html_data.alt_text);
}
15 changes: 15 additions & 0 deletions examples/hello_html.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
use arboard::Clipboard;

fn main() {
env_logger::init();
let mut clipboard = Clipboard::new().unwrap();
println!("Clipboard text was: {:?}", clipboard.get_html());

let html = "<h1>Hello, world!</h1><p>This is HTML.</p>";
let alt_text = "Hello, world!\nThis is HTML.";
clipboard.set_html(html, Some(alt_text)).unwrap();
println!(
"But now the clipboard text should be this HTML: \"{}\" with this alternate text: \"{}\"",
html, alt_text

Check warning on line 13 in examples/hello_html.rs

View workflow job for this annotation

GitHub Actions / rustfmt

Diff in /home/runner/work/arboard/arboard/examples/hello_html.rs
);
}
16 changes: 16 additions & 0 deletions src/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,22 @@ impl<'a> ImageData<'a> {
}
}

#[derive(Debug, Default)]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Let's remove the Default implementation here. It makes it harder to maintain version compatibility in the future.

pub struct HTMLData {
pub html: String,
pub alt_text: String,
}

impl HTMLData {
pub fn from_html(html: String) -> Self {
Self { html, ..Default::default() }
}

pub fn from_alt_text(alt_text: String) -> Self {
Self { alt_text, ..Default::default() }
}
Comment on lines +160 to +162
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a valid function to expose? If the user doesn't have any HTML in the first place it seems like they'd be better off using set_text() instead.

}

#[cfg(any(windows, all(unix, not(target_os = "macos"))))]
pub(crate) struct ScopeGuard<F: FnOnce()> {
callback: Option<F>,
Expand Down
11 changes: 11 additions & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ mod common;
use std::borrow::Cow;

pub use common::Error;
use common::HTMLData;
#[cfg(feature = "image-data")]
pub use common::ImageData;

Expand Down Expand Up @@ -78,6 +79,11 @@ impl Clipboard {
self.set().text(text)
}

/// Fetches HTML from the clipboard and returns it.
pub fn get_html(&mut self) -> Result<HTMLData, Error> {
self.get().html()
}
Comment on lines +82 to +85
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Do you think HTML is a common enough case to add this to the main Clipboard structure? I would prefer to expose this only through the Get builder to keep it tidy.


/// Places the HTML as well as a plain-text alternative onto the clipboard.
///
/// Any valid UTF-8 string is accepted.
Expand Down Expand Up @@ -145,6 +151,11 @@ impl Get<'_> {
self.platform.text()
}

/// Completes the "get" operation by fetching HTML data from the clipboard.
pub fn html(self) -> Result<HTMLData, Error> {
self.platform.html()
}

/// Completes the "get" operation by fetching image data from the clipboard and returning the
/// decoded pixels.
///
Expand Down
10 changes: 10 additions & 0 deletions src/platform/linux/mod.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
use std::borrow::Cow;

use crate::HTMLData;

#[cfg(feature = "wayland-data-control")]
use log::{trace, warn};

Expand Down Expand Up @@ -114,6 +116,14 @@ impl<'clipboard> Get<'clipboard> {
}
}

pub(crate) fn html(self) -> Result<HTMLData, Error> {
match self.clipboard {
Clipboard::X11(clipboard) => clipboard.get_html(self.selection),
#[cfg(feature = "wayland-data-control")]
Clipboard::WlDataControl(clipboard) => Err("not implemented"),
}
}

#[cfg(feature = "image-data")]
pub(crate) fn image(self) -> Result<ImageData<'static>, Error> {
match self.clipboard {
Expand Down
38 changes: 37 additions & 1 deletion src/platform/linux/x11.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,10 @@ use super::encode_as_png;
use super::{into_unknown, LinuxClipboardKind};
#[cfg(feature = "image-data")]
use crate::ImageData;
use crate::{common::ScopeGuard, Error};
use crate::{
common::{HTMLData, ScopeGuard},
Error,
};

type Result<T, E = Error> = std::result::Result<T, E>;

Expand Down Expand Up @@ -884,6 +887,39 @@ impl Clipboard {
self.inner.write(data, selection, wait)
}

pub(crate) fn get_html(&self, selection: LinuxClipboardKind) -> Result<HTMLData> {
let html_format = [self.inner.atoms.HTML];
let alt_text_formats = [
self.inner.atoms.UTF8_STRING,
self.inner.atoms.UTF8_MIME_0,
self.inner.atoms.UTF8_MIME_1,
self.inner.atoms.STRING,
self.inner.atoms.TEXT,
self.inner.atoms.TEXT_MIME_UNKNOWN,
];
Comment on lines +892 to +899
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we find a way to extract this into a reusable construct that get_text can use as well?


let mut data = HTMLData::default();

let html_result = self.inner.read(&html_format, selection);
if let Ok(html_data) = html_result {
let html: String = html_data.bytes.into_iter().map(|c| c as char).collect();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this correct? My understanding that HTML can contain UTF-8 so we should just use String::from_utf8 here.

data.html = html;
}

let alt_text_result = self.inner.read(&alt_text_formats, selection);
if let Ok(alt_text_data) = alt_text_result {
data.alt_text = if alt_text_data.format == self.inner.atoms.STRING {
// ISO Latin-1
// See: https://stackoverflow.com/questions/28169745/what-are-the-options-to-convert-iso-8859-1-latin-1-to-a-string-utf-8
alt_text_data.bytes.into_iter().map(|c| c as char).collect::<String>()
} else {
String::from_utf8(alt_text_data.bytes).map_err(|_| Error::ConversionFailure)?
};
}
Comment on lines +910 to +918
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, because the logic is also duplicated from get_text.


Ok(data)
}

pub(crate) fn set_html(
&self,
html: Cow<'_, str>,
Expand Down
Loading