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

Update clipboard-win to v5 and replace winapi with windows-sys #123

Merged
merged 14 commits into from
Dec 4, 2023

Conversation

rhysd
Copy link
Contributor

@rhysd rhysd commented Nov 24, 2023

This PR does two things for Windows support:

  1. Update clipboard-win to v5.0.0
    • I checked changes in v5.0.0 and the changes didn't affect any implementation in this crate
  2. Replace winapi crate dependency with windows-rs
    • winapi is unmaintained. windows-rs is well-maintained by Microsoft officially
    • By this change, winapi can be removed completely from this crate
    • The replacement is somewhat mechanical translation from winapi to windows-rs. I read MSDN documents but my understanding of the Win32 APIs is not perfect. I'd like to have a detailed code review
    • Some constants used by this crate are not defined in windows-rs. I defined them directly in platform/windows.rs

I confirmed writing/reading clipboard with both text and image contents using hello_world and set_image/get_image examples on my Windows 10 machine.

With this PR, cargo dependencies will change to:

> cargo tree --edges no-dev
arboard v3.3.0
├── clipboard-win v5.0.0
│   └── error-code v3.0.0
├── log v0.4.20
├── thiserror v1.0.35
│   └── thiserror-impl v1.0.35 (proc-macro)
│       ├── proc-macro2 v1.0.43
│       │   └── unicode-ident v1.0.4
│       ├── quote v1.0.21
│       │   └── proc-macro2 v1.0.43 (*)
│       └── syn v1.0.100
│           ├── proc-macro2 v1.0.43 (*)
│           ├── quote v1.0.21 (*)
│           └── unicode-ident v1.0.4
└── windows v0.52.0
    ├── windows-core v0.52.0
    │   └── windows-targets v0.52.0
    │       └── windows_x86_64_msvc v0.52.0
    └── windows-targets v0.52.0 (*)

@complexspaces
Copy link
Collaborator

Thanks for the PR, I appreciate the effort. Would you consider modifying this to use windows-sys instead of the windows crate? The former has much less version churn and I'm not sure how much value arboard gets out of using windows because we don't utilize any COM or WinRT logic.

I would definitely be in favor of implementing a helper function to wrap some of the Global* calls to handle checking for errors internally though.

@rhysd
Copy link
Contributor Author

rhysd commented Nov 28, 2023

It makes sense. I'll try windows-sys.

@rhysd
Copy link
Contributor Author

rhysd commented Nov 28, 2023

I would definitely be in favor of implementing a helper function to wrap some of the Global* calls to handle checking for errors internally though.

I focused on replacing winapi with windows-rs with as small diff as possible. But I'll refactor the code more idiomtic.

@complexspaces
Copy link
Collaborator

No worries either way, I could refactor it later if you are busy. I generally don't mind a little bit of scope creep in PRs that touches the same area as long as its independently reviewable (its own commit, etc).

@rhysd
Copy link
Contributor Author

rhysd commented Nov 28, 2023

No problem. I like refactorying so I'll try it by myself :)

CreateDIBitmap, DeleteObject, GetDIBits, LCS_sRGB, BITMAPINFO, BITMAPINFOHEADER,
BITMAPV5HEADER, BI_RGB, CBM_INIT, DIB_RGB_COLORS, LCS_GM_IMAGES, PROFILE_EMBEDDED,
PROFILE_LINKED, RGBQUAD,
mod image_data {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Separating this module as a new file may be a good idea.

@rhysd
Copy link
Contributor Author

rhysd commented Nov 28, 2023

I added some refactorings:

  • a833222 : Add Error::unknown factory function to avoid Error::Unknown { description: expression.into() } boilerplate
  • ec5e1d8 : Separate the implementation details of image data handling into image_data module to avoid many repeats of #[cfg(feature = "image-data")] gates. Moving the module to a new file may be a good idea (I don't have a strong opinion here).
  • ba5575b : Wrap Win32 functions which can return errors to leverage error handling with ? operator. This made add_cf_dibv5 and read_cf_dibv5 simpler
  • ba9e7e8 : Move tests for image data handling to image_data module and added more test cases

@rhysd rhysd changed the title Update clipboard-win to v5 and replace winapi with windows-rs Update clipboard-win to v5 and replace winapi with windows-sys Nov 28, 2023
@rhysd
Copy link
Contributor Author

rhysd commented Nov 28, 2023

The dependency tree is now:

> cargo tree --edges no-dev
arboard v3.3.0
├── clipboard-win v5.0.0
│   └── error-code v3.0.0
├── log v0.4.20
├── thiserror v1.0.35
│   └── thiserror-impl v1.0.35 (proc-macro)
│       ├── proc-macro2 v1.0.43
│       │   └── unicode-ident v1.0.4
│       ├── quote v1.0.21
│       │   └── proc-macro2 v1.0.43 (*)
│       └── syn v1.0.100
│           ├── proc-macro2 v1.0.43 (*)
│           ├── quote v1.0.21 (*)
│           └── unicode-ident v1.0.4
└── windows-sys v0.52.0
    └── windows-targets v0.52.0
        └── windows_x86_64_msvc v0.52.0

Cargo.toml Outdated Show resolved Hide resolved
src/platform/windows.rs Outdated Show resolved Hide resolved
src/platform/windows.rs Outdated Show resolved Hide resolved
src/platform/windows.rs Show resolved Hide resolved
src/platform/windows.rs Show resolved Hide resolved
src/platform/windows.rs Outdated Show resolved Hide resolved
src/platform/windows.rs Outdated Show resolved Hide resolved
src/platform/windows.rs Outdated Show resolved Hide resolved
@rhysd
Copy link
Contributor Author

rhysd commented Dec 1, 2023

Thank you for your reviews. I'll have a look at them tonight.

@rhysd
Copy link
Contributor Author

rhysd commented Dec 1, 2023

@complexspaces I addressed all review comments. Would you take a look again?

Copy link
Collaborator

@complexspaces complexspaces left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your work on this, everything looks good now!

@complexspaces complexspaces merged commit a100f2d into 1Password:master Dec 4, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants