-
Notifications
You must be signed in to change notification settings - Fork 80
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 Get::html() for all platforms #163
Open
Gae24
wants to merge
7
commits into
1Password:master
Choose a base branch
from
Gae24:get-html
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
d42f5e6
implement get html operation
Gae24 aea5012
add test for html() get operation
Gae24 326ede7
cleanup wayland code
Gae24 0958834
fix fmt errors
Gae24 f9b466b
fix compilation on macOS
Gae24 d445228
macOS: extract html
Gae24 52ce714
fix windows
Gae24 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -121,6 +121,27 @@ impl Clipboard { | |
unsafe { self.pasteboard.clearContents() }; | ||
} | ||
|
||
fn string_from_type(&self, type_: &'static NSString) -> Result<String, Error> { | ||
// XXX: There does not appear to be an alternative for obtaining text without the need for | ||
// autorelease behavior. | ||
autoreleasepool(|_| { | ||
// XXX: We explicitly use `pasteboardItems` and not `stringForType` since the latter will concat | ||
// multiple strings, if present, into one and return it instead of reading just the first which is `arboard`'s | ||
// historical behavior. | ||
let contents = unsafe { self.pasteboard.pasteboardItems() }.ok_or_else(|| { | ||
Error::Unknown { description: String::from("NSPasteboard#pasteboardItems errored") } | ||
})?; | ||
|
||
for item in contents { | ||
if let Some(string) = unsafe { item.stringForType(type_) } { | ||
return Ok(string.to_string()); | ||
} | ||
} | ||
|
||
Err(Error::ContentNotAvailable) | ||
}) | ||
} | ||
|
||
// fn get_binary_contents(&mut self) -> Result<Option<ClipboardContent>, Box<dyn std::error::Error>> { | ||
// let string_class: Id<NSObject> = { | ||
// let cls: Id<Class> = unsafe { Id::from_ptr(class("NSString")) }; | ||
|
@@ -182,27 +203,12 @@ impl<'clipboard> Get<'clipboard> { | |
} | ||
|
||
pub(crate) fn text(self) -> Result<String, Error> { | ||
// XXX: There does not appear to be an alternative for obtaining text without the need for | ||
// autorelease behavior. | ||
autoreleasepool(|_| { | ||
// XXX: We explicitly use `pasteboardItems` and not `stringForType` since the latter will concat | ||
// multiple strings, if present, into one and return it instead of reading just the first which is `arboard`'s | ||
// historical behavior. | ||
let contents = | ||
unsafe { self.clipboard.pasteboard.pasteboardItems() }.ok_or_else(|| { | ||
Error::Unknown { | ||
description: String::from("NSPasteboard#pasteboardItems errored"), | ||
} | ||
})?; | ||
|
||
for item in contents { | ||
if let Some(string) = unsafe { item.stringForType(NSPasteboardTypeString) } { | ||
return Ok(string.to_string()); | ||
} | ||
} | ||
unsafe { self.clipboard.string_from_type(NSPasteboardTypeString) } | ||
} | ||
|
||
Err(Error::ContentNotAvailable) | ||
}) | ||
pub(crate) fn html(self) -> Result<String, Error> { | ||
let html = unsafe { self.clipboard.string_from_type(NSPasteboardTypeHTML) }?; | ||
extract_html(html).ok_or(Error::ConversionFailure) | ||
} | ||
|
||
#[cfg(feature = "image-data")] | ||
|
@@ -347,6 +353,18 @@ fn add_clipboard_exclusions(clipboard: &mut Clipboard, exclude_from_history: boo | |
} | ||
} | ||
|
||
fn extract_html(html: String) -> Option<String> { | ||
let start_tag = "<body>"; | ||
let end_tag = "</body>"; | ||
|
||
// Locate the start index of the <body> tag | ||
let start_index = html.find(start_tag)? + start_tag.len(); | ||
// Locate the end index of the </body> tag | ||
let end_index = html.find(end_tag)?; | ||
|
||
Some(html[start_index..end_index].to_string()) | ||
Comment on lines
+361
to
+365
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: |
||
} | ||
|
||
/// Apple-specific extensions to the [`Set`](crate::Set) builder. | ||
pub trait SetExtApple: private::Sealed { | ||
/// Excludes the data which will be set on the clipboard from being added to | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Question: Are these guaranteed to appear in enough cases? For example in Firefox I see
<body>
tags wrapping the content when I copy fromexample.com
. However if I copy the same domain out of Chrome it directly starts with<h1>
. If I copy a nested block out of Logseq it omits them and starts directly with<ul>
as the top tag too.As-is, IIUC, this will miss a lot of valid HTML cases from real browsers today because it returns
None
if the<body>
wrapper can't be found.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.
I actually made a PR on @Gae24's fork to remove this function. I also think it's not necessary and should be left to the consuming end, if needed.
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.
It's definitely a question worth considering either way. I don't have a Linux system available to test this currently but on Windows I am returned a
<div>
directly when copying from Firefox and the same<h1>
as Chrome returns on macOS.If we wanted to keep the behavior closer between the platforms we could strip out
<body>
and anything before it on both sides of the HTML.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.
I'd guess what's written to the clipboard is entirely application-dependent. Applications other than browsers (Word or similar) probably have a different format as well.
Which leads me to say transforming the clipboard HTML content would have to be left to the consumer.
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.
If I remember correctly it was added because some tests were failing. Testing
example.com
on Linux while Firefox wraps the content with<meta http-equiv="content-type" content="text/html; charset=utf-8"><div></div>
, Edge that's still Chromium, simply gives<h1>