Conversation
LeonMatthes
left a comment
There was a problem hiding this comment.
Just some initial thoughts.
In summary, I think we should try to limit the API surface of this where possible.
Maybe we can just use strings for mime types for now? That's probably what they need to end up as in Slint code anyhow.
|
|
||
| use crate::SharedString; | ||
|
|
||
| pub use mime; |
There was a problem hiding this comment.
We should not publicly export anything from mime.
This is a 3rd-party crate we do not control.
We can use it internally, but it should not show up as public API.
Can we just use strings instead?
There was a problem hiding this comment.
mime is licensed under MIT and I was already considering vendoring it (e.g. to slint-mime) so we can make it no_std-compatible. In that case it could be re-exported somewhere in the top-level slint crate. The reason we're not just using strings is that mime::Mime can be const-constructed and does not allocate for the vast majority of cases.
There was a problem hiding this comment.
It's not about the licensing, it's the fact that there's no stable API for it, so we shouldn't re-export it.
In this PR, mime is added as a dependency to pull in several other dependencies to merely have constants for "text/plain" and "text/plain; charset=utf-8".
For now, please remove the mime dependency and use plain strings - and for internal use of course it's fine to have a const for those two mime-types (that are very well known).
| fn read_plaintext( | ||
| &self, | ||
| clipboard: crate::platform::Clipboard, | ||
| ) -> Result<SharedString, ClipboardError> { |
There was a problem hiding this comment.
Should this return a Result<Option<SharedString>,...>, as "clipboard is empty" should be a valid return value, no?
There was a problem hiding this comment.
I think it should probably be something more like an explicit is_empty method. Just because this doesn't have anything to return doesn't mean that the clipboard is empty. I've never seen a case where "the clipboard is empty" has a different UX to "the clipboard contains something that can't be pasted into this space", but if you do want a different UX I think it's fine to just have an explicit method to check that.
There was a problem hiding this comment.
Actually, as far as I know every platform allows efficiently iterating over the available MIME types, although returning them as a slice might be a little more difficult. We could totally somehow have a method that allows iteration over the available MIME types, with is_empty having a default implementation of checking if that iterator returns no elements. has_type could also have a default implementation in terms of that, although that's something you'll probably want to have a custom impl for to improve efficiency.
| } | ||
| } | ||
|
|
||
| pub enum ClipboardError { |
There was a problem hiding this comment.
Do we really want this to be a separate type, or should it just reuse PlatformError?
There was a problem hiding this comment.
I think it's worth having ClipboardError::TypeMismatch specifically as a separate error that can be explicitly matched on. The rest were added so that they get explicitly tagged as being clipboard errors. In general I like having one error type per module so that the error scope remains clear when there's deep nesting, obviously that can often be too much but I think it somewhat makes sense to introduce a scope here. I don't mind adding ClipboardTypeMismatch to PlatformError though, that's the simpler solution it's just not my preferred solution.
There was a problem hiding this comment.
The enum has four variants, but only one is actually used in this PR. I suggest to keep it simple and agree with Leon: Just use PlatformError for now.
| /// This trait is intended to be implemented by platforms for some internal custom data type, or they | ||
| /// can use the `TypeMap` implementation which is for application-internal use only. For example, | ||
| /// most embedded platforms will be able to just use `TypeMap`. | ||
| pub trait ClipboardData { |
There was a problem hiding this comment.
This feels like it's duplicating the PlatformClipborad trait.
Maybe PlatformClipboard should just have two methods:
fn get(self, clipboard) -> Result<ClipboardData, ...>;
fn set(self, clipboard_data) -> Result<(), ...>;
There was a problem hiding this comment.
Yeah, I was just thinking about this. Definitely an oversight, I'll fix it.
There was a problem hiding this comment.
Update: I think this is actually correct. Returning a ClipboardData actually means that for backends that have special handling for strings (and, eventually, images) then they'll need to introduce an extra unnecessary allocation. This API means we can optimise the access when we know the target type.
| pub enum ClipboardType { | ||
| Internal(TypeId), | ||
| #[cfg(feature = "std")] | ||
| External(mime::Mime), | ||
| } |
There was a problem hiding this comment.
Do we really need this differentiation?
In my mind, mime types are mostly strings, so can't people just use their own special mime type name if they want to mark their own internal types?
There was a problem hiding this comment.
They can, yeah, but then the user will need to figure out their own custom-MIME-type-to-Rust-type mapping. Doing it like this (with TypeId) is automatic, and so for 99% of users they won't need to think about any of the clipboard type system stuff. The idea is that we'd have ClipboardData implementations for the different platforms we support as well as one that just requires alloc and works entirely in-memory so that embedded devices seamlessly get drag-and-drop/clipboard support. That way, the user will almost never have to actually implement MIME type handling themselves, we'll have some concrete type or a simplified trait with a blanket ClipboardData impl and we can hide the complexity of the different kinds of type away from the user.
There was a problem hiding this comment.
I agree with Leon, please just use a string. We can delegate to mime-type parsing if needed, we can add complexity to the API as needed, but we should start simple and for mime types the minimal data type is a string.
There was a problem hiding this comment.
On second thought: I don't think that we should have ClipboardType. The name itself is confusing because we also have pub enum Clipboard, and that describes the type of clipboard. But more importantly: I don't think we should try to optimise at all for a mime-type less internal storage of data. Let's just say, whether in-process of out-of-process, mime data in Slint requires a mime type as a string and a boxed dyn Any (or dyn ClipboardData).
|
|
||
| /// Read a string from the clipboard, in the specified MIME type. | ||
| #[cfg(feature = "std")] | ||
| fn read_string(self: Arc<Self>, type_: &mime::Mime) -> Result<SharedString, ClipboardError>; |
There was a problem hiding this comment.
Why do these all need to take Arc<Self>, and not just &self?
There was a problem hiding this comment.
Or Rc<Self> if it really needs to be a pointer, as Slint is single-threaded anyway.
There was a problem hiding this comment.
The reason it's Arc<Self> is to make it cloneable, since the API to read clipboard data should return the same thing for the same MIME type every time. I agree that it could be Rc<Self>, though.
There was a problem hiding this comment.
Also, using self: Rc<Self> allows a small optimisation of using &mut methods when there's only a single reference but & methods when there are multiple.
| } | ||
|
|
||
| // Dummy implementation of `ClipboardData` that does nothing, used to clear the clipboard. | ||
| impl ClipboardData for () { |
There was a problem hiding this comment.
So to clear the clipboard one has to allocate an Rc<()>. I'm not particular about allocations for this API, TBH, but it seems way easier to me to either accept an option in the setter or to have a clear() function :-)
There was a problem hiding this comment.
Yeah, to my understanding Rc<()> allocates. I'm trying to figure out if I can come up with an API that lets me use Box instead of Rc since Box<()> doesn't allocate and just in general being allocation-free for ZSTs would be nice. I'll investigate if Rc allocates first though
There was a problem hiding this comment.
Go straight for a Box. Unless you have evidence that sharing is required for our implementation?
Closes #11399 (that issue also explains the research and design decisions).
This is an initial implementation of an API for the clipboard that can support arbitrary MIME types. For now, the functionality is unchanged - only strings can be stored/retrieved from the clipboard. Actually handling multiple types is left for future work.