Skip to content

add file_list to Set interface#181

Merged
complexspaces merged 7 commits into1Password:masterfrom
Gae24:set-file-list
Aug 13, 2025
Merged

add file_list to Set interface#181
complexspaces merged 7 commits into1Password:masterfrom
Gae24:set-file-list

Conversation

@Gae24
Copy link
Contributor

@Gae24 Gae24 commented Mar 19, 2025

We should decide what type of input we accept.
On windows clipboard_win::raw::set_file_list is used which has as parameter &[impl AsRef<str>] and doesn't check if the input is actually a path; on linux and osx the parameter is of type &[impl AsRef<Path>] and canonicalize() is called so non existing paths get discarded

@Gae24 Gae24 mentioned this pull request Apr 1, 2025
@complexspaces
Copy link
Member

It seems like the best generic interface we could expose is impl Iterator<Item = impl AsRef<Path>>] because its the most typesafe, but that would require a re-collection (.collect<Vec<_>>) on Windows because of the interface clipboard-win gives us (despite internally just looping over the slice). I think its advantages are:

  • Allows callers with well-typed libraries/crates using Path/PathBuf no friction/extra unwraps.
  • Allows callers with borrowed data in their collections (&str) no friction.
  • Allows callers with owned data in their collections (String) no friction.
  • Allows arboard on non-Windows platforms to skip a collection allocation.

I was experimenting with the above in this playground link, for reference. Do you think its worth just dealing with the current limitation on Windows and seeing if it can be removed via either a PR to clipboard-win or re-implementing similar in arboard so that we can have a flexible interface for the future?

@Gae24
Copy link
Contributor Author

Gae24 commented Apr 4, 2025

It depends on which behavior we want. On Linux and MacOs the argument is &[impl AsRef<Path>] due to the use of canonicalize() since I thought we shouldn't write invalid paths.
If we want align Windows to the same behaviour, I say let's implement our own version.

@complexspaces
Copy link
Member

complexspaces commented Apr 7, 2025

That sounds reasonable to me; the Win32 code for this isn't super bad. We already have some primitives for clipboard memory allocation due to image support. I pretty heavily value cross-platform consistency in arboard where feasible.

One thought came to me over the weekend I'd like to ask about regarding invalid paths: Can you think of any use cases or needs for "dropping" a list of files on the clipboard that aren't proper file URIs, such as app-specific URIs (onepassword://, for example). It might be out of scope for this addition too, which is fine.

@Gae24
Copy link
Contributor Author

Gae24 commented Apr 8, 2025

Certainly, we can expand support for more uri types later on.

@Gae24
Copy link
Contributor Author

Gae24 commented Apr 8, 2025

Had to modify a bit Cargo.toml and move out of image_data a couple of functions.

For some reason, clipboard_win doesn't place the data in a STGMEDIUM, should we do it?

@complexspaces
Copy link
Member

It doesn't look like Chromium does, so I don't think we need to. They create a STDMEDIUM structure here but then when actually writing the data only the hGlobal part is actually used.

@Gae24
Copy link
Contributor Author

Gae24 commented Apr 10, 2025

on Windows canonicalize will convert to a UNC path, do we want this behavior?

@complexspaces
Copy link
Member

Sorry, missed your comment. I learn towards not wanting to introduce UNC paths because since we're very reliant on other applications reading the pasteboard to know how to handle them correctly.

@complexspaces complexspaces added waiting on author Further information is requested O-UNIX Work related to X11 or Wayland on UNIX platforms O-Windows Work related to the clipboard on Windows. O-Apple Work related to the macOS or iOS clipboard enhancement New feature or request labels Jun 18, 2025
@Gae24
Copy link
Contributor Author

Gae24 commented Jul 28, 2025

Sorry for taking this long.
The way it is done is the same implementation of canonicalize in the standard library, but instead of returning a Pathbuf, PathCchStripPrefix is used to remove the prefix and return the buffer as it is avoiding calling MultiByteToWideChar.

@complexspaces complexspaces added waiting on review The change is currently waiting on an arboard maintainer for a review or larger-scale update and removed waiting on author Further information is requested labels Aug 2, 2025
@complexspaces complexspaces added waiting on author Further information is requested and removed waiting on review The change is currently waiting on an arboard maintainer for a review or larger-scale update labels Aug 2, 2025
@complexspaces
Copy link
Member

The new set of changes look good to me. I'm happy to approve this and merge once the last thread is resolved.

@complexspaces complexspaces merged commit 17ef05c into 1Password:master Aug 13, 2025
11 checks passed
@complexspaces
Copy link
Member

This is now released in v3.6.1 on crates.io.

Thanks again for the new feature! I hope servo continues to make good progress :)

@Gae24 Gae24 deleted the set-file-list branch September 19, 2025 10:39
kernelkind pushed a commit to kernelkind/arboard that referenced this pull request Nov 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request O-Apple Work related to the macOS or iOS clipboard O-UNIX Work related to X11 or Wayland on UNIX platforms O-Windows Work related to the clipboard on Windows. waiting on author Further information is requested

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants