feat: support custom mime types #56

wash2 · 2024-02-27T22:46:28Z

This adds support for loading and storing data that is not text. I've renamed some of the existing methods, so it's a breaking change, but I guess I could avoid renaming the methods so that isn't an issue. and make the methods for loading and storing custom types something like store_custom or load_custom.

kchibisov

We also have CHANGELOG which is here, but no one cares about, even me.

kchibisov · 2024-02-28T19:05:39Z

src/worker.rs

+    Store(Box<dyn AsMimeTypes + Send>, SelectionTarget),
+    /// Store Data with the given Mime Types


This broke formatting. Also, each doc comments ends with . in the end.

kchibisov · 2024-02-28T19:07:20Z

src/worker.rs

+                        if state.data_device_manager_state.is_some() =>
+                    {
+                        if let Err(err) = state.load_selection(target, &mime_types) {


That doesn't follow the old code, because it doesn't check primary selection clipboard, which may be absent.

I'm not sure what you mean. I don't think the old code checked primary_selection_manager_state but I can add a check for that.

Hm, the code I wrote is fool-proof and actually doesn't try to unwrap anything. I'm not even sure whether the first check is needed for data device, given that it's also checked, kind of.

Though, maybe won't hurt.

kchibisov · 2024-02-28T19:08:32Z

src/mime.rs

@@ -1,50 +1,88 @@
+use std::borrow::Cow;
+use thiserror::Error;


it's used just for a single enum in the entire crate, no need to be fancy.

kchibisov · 2024-02-28T19:23:21Z

src/lib.rs

+    fn store_inner<T: AsMimeTypes + Send + 'static>(&self, data: T, target: SelectionTarget) {
+        let request = worker::Command::Store(Box::new(data), target);
+        let _ = self.request_sender.send(request);
+    }


All private functions go bellow the public ones.

kchibisov · 2024-02-28T19:24:44Z

src/lib.rs

        if let Ok(reply) = self.request_receiver.recv() {
-            reply
+            match reply {
+                Ok((data, mime)) => T::try_from((data, mime))
+                    .map_err(|err| std::io::Error::new(std::io::ErrorKind::Other, err)),
+                Err(err) => Err(err),
+            }
        } else {
            // The clipboard thread is dead, however we shouldn't crash downstream, so
            // propogating an error.
            Err(std::io::Error::new(std::io::ErrorKind::Other, "clipboard is dead."))
        }
    }


Can probably rewrite this with match + .recv().map, it's just ugly the way it's right now.

kchibisov · 2024-02-28T19:29:37Z

src/mime.rs

+    fn discriminant(&self) -> u8 {
+        unsafe { *(self as *const Self as *const u8) }
+    }


No tricks like that please. You can do it safely e.g by slapping into Text enum and doing discriminant there.

kchibisov · 2024-02-28T19:31:40Z

src/mime.rs

+        allowed
+            .iter()
+            .find(|allowed| {
+                offered_mime_types.iter().any(|offered| offered.as_str() == allowed.as_ref())
+            })
+            .cloned()


This looks kind of less efficient, but I guess you don't have that many mime types, so it's kind of ok?

Ya, I guess it is algorithmically inefficient, but I assumed neither list would be long enough in practice to cause issues.

kchibisov · 2024-02-28T19:34:29Z

src/mime.rs

+    /// Available mime types for this data
+    fn available(&self) -> Cow<'static, [MimeType]>;
+
+    /// Data as a specific mime_type


The description reads really weird.

kchibisov · 2024-02-28T19:36:49Z

src/lib.rs

@@ -8,18 +8,22 @@ use std::ffi::c_void;
 use std::io::Result;
 use std::sync::mpsc::{self, Receiver};

+use mime::{AllowedMimeTypes, AsMimeTypes, MimeType};


local crate exports go below the mod *; this place is reserved for the outsiders.

wash2 · 2024-02-28T19:43:41Z

We also have CHANGELOG which is here, but no one cares about, even me.

😅 I'll edit the CHANGELOG and address the other comments, thanks.

wash2 · 2024-03-01T21:31:46Z

Would there be any interest in using the mimes crate? I'm hoping to add support for multiple mime types to window_clipboard and I think it could also probably use the mime crate.

wash2 · 2024-03-05T15:59:34Z

Never mind about the mime crate, I think this is OK.

Cargo.toml

kchibisov · 2024-03-18T19:16:05Z

src/dnd/mod.rs

+    pub fn end_dnd() {}
+
+
+}


New line in the end of file.

Ahh, this file will be part of a separate PR for DnD. So I'll remove it.

kchibisov · 2024-03-18T19:16:33Z

src/lib.rs

+use state::SelectionTarget;
+use text::Text;


below mod

kchibisov · 2024-03-18T19:20:29Z

src/lib.rs

+    /// Load raw clipboard data.
+    ///
+    /// Loads content from a  primary clipboard on a last observed seat.
+    pub fn load_raw(
+        &self,
+        allowed: impl Into<Cow<'static, [MimeType]>>,
+    ) -> Result<(Vec<u8>, MimeType)> {
+        self.load_inner(SelectionTarget::Clipboard, allowed)
+    }
+
+    /// Load raw primary clipboard data.
+    ///
+    /// Loads content from a  primary clipboard on a last observed seat.
+    pub fn load_primary_raw(
+        &self,
+        allowed: impl Into<Cow<'static, [MimeType]>>,
+    ) -> Result<(Vec<u8>, MimeType)> {
+        self.load_inner(SelectionTarget::Primary, allowed)
+    }


What's the use case for raw?

I found it useful to have an option without trait bounds on the return type, when passing around a boxed trait object clipboard. I still use the AllowedMimeTypes trait, but not in the return type trait for the clipboard because Boxing it in a return type doesn't seem to be object safe.

If the trait were allowed to take an optional reference to self like below, the raw methods could be avoided, but the api would probably need to include an argument Option<&(impl AllowedMimeTypes + 'static)>. This way users could create their own type type implementing AllowedMimeTypes which just wraps the received raw data and the allowed mime types.

/// Describes the mime types which are accepted. pub trait AllowedMimeTypes: TryFrom<(Vec<u8>, MimeType)> { /// List allowed mime types for the type to convert from a byte slice. /// /// Allowed mime types should be listed in order of decreasing preference, /// most preferred first. fn allowed(_self: Option<&self>) -> Cow<'static, [MimeType]>; }

I've refactored the methods a bit so they are just allowing a specific mime type to be requested instead.

kchibisov · 2024-03-18T19:22:33Z

src/mime.rs

+    fn discriminant(&self) -> usize {
+        match self {
+            MimeType::Text(t) => *t as usize,
+            MimeType::Other(_) => 3,
+        }
+    }


This is error prone and should be removed all together.

kchibisov · 2024-03-18T19:23:00Z

src/mime.rs

+    fn as_ref(&self) -> &str {
+        match self {
+            MimeType::Other(s) => s.as_ref(),
+            m => ALLOWED_TEXT_MIME_TYPES[m.discriminant()],


Use MimeType::Text(text) and index with text right away.

kchibisov · 2024-03-18T19:48:02Z

src/mime.rs

-        }
-
-        fallback
+    pub fn find_allowed(


Should likely be pub(crate) now.

…stead

feat: support custom mime types

3e56207

wash2 force-pushed the mime-types branch from 7051bd0 to 3e56207 Compare February 27, 2024 22:50

wash2 added 2 commits February 27, 2024 17:54

chore: clippy

5603960

chore: support 1.65

87661a2

kchibisov requested changes Feb 28, 2024

View reviewed changes

wash2 added 7 commits February 28, 2024 14:59

refactor: remove thiserror from dependencies

1d5e98d

fix: doc comment typos

7a72dd3

reword some doc comments

84229d0

refactor: Text mimes as their own type

a74fc5b

cleanup: All private functions go bellow the public ones.

4492880

cleanup: prettier error handling

9341cc0

cleanup: move local imports below mod *

0043c35

wash2 force-pushed the mime-types branch from ec7e596 to 94e1dcd Compare February 28, 2024 20:39

wash2 marked this pull request as draft February 29, 2024 20:36

wash2 marked this pull request as ready for review March 5, 2024 15:59

kchibisov requested changes Mar 18, 2024

View reviewed changes

wash2 added 7 commits March 25, 2024 15:55

chore: update CHANGELOG

0d110dd

cleanup: add missing . to docs

0d61c08

fix: check primary_selection_manager_state

7da622c

chore: document allowed mime types order preference

b03e620

cleanup: more clearly document find_allowed

50ccb2b

refactor: simplify trait bounds in the public interface

23ac0ff

feat: load raw clipboard data

3118f11

wash2 force-pushed the mime-types branch from cc0101c to 301d803 Compare March 25, 2024 19:58

feat: add conversion from string for MimeType

7db9746

wash2 force-pushed the mime-types branch from 301d803 to 7db9746 Compare March 25, 2024 19:59

fix: reorder imports

f5908d0

wash2 added 2 commits March 25, 2024 16:04

refactor: remove discriminant method

9a0816b

refactor: make MimeType::find_allowed pub(crate)

cca5483

wash2 mentioned this pull request Mar 25, 2024

Dnd #57

Draft

refactor: make previous raw methods just allow specific mime types in…

934eb88

…stead

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support custom mime types #56

feat: support custom mime types #56

wash2 commented Feb 27, 2024

kchibisov left a comment

kchibisov Feb 28, 2024

kchibisov Feb 28, 2024

wash2 Feb 28, 2024

kchibisov Feb 28, 2024

kchibisov Feb 28, 2024

kchibisov Feb 28, 2024

kchibisov Feb 28, 2024

kchibisov Feb 28, 2024

kchibisov Feb 28, 2024

wash2 Feb 28, 2024

kchibisov Feb 28, 2024

kchibisov Feb 28, 2024

wash2 commented Feb 28, 2024

wash2 commented Mar 1, 2024

wash2 commented Mar 5, 2024

kchibisov Mar 18, 2024

wash2 Mar 25, 2024

kchibisov Mar 18, 2024

kchibisov Mar 18, 2024

wash2 Mar 25, 2024

wash2 Mar 25, 2024 •

edited

Loading

wash2 Mar 26, 2024

kchibisov Mar 18, 2024

kchibisov Mar 18, 2024

kchibisov Mar 18, 2024

		Store(Box<dyn AsMimeTypes + Send>, SelectionTarget),
		/// Store Data with the given Mime Types

feat: support custom mime types #56

Are you sure you want to change the base?

feat: support custom mime types #56

Conversation

wash2 commented Feb 27, 2024

kchibisov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wash2 commented Feb 28, 2024

wash2 commented Mar 1, 2024

wash2 commented Mar 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wash2 Mar 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wash2 Mar 25, 2024 •

edited

Loading