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

Migrating to windows-sys #26

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

WanderLanz
Copy link

@WanderLanz WanderLanz commented Oct 3, 2022

Attempt at moving to windows-sys for win32 api

Please do as you see fit, winapi still seems to be working fine for crossterm's use at the moment.

@WanderLanz WanderLanz requested a review from TimonPost as a code owner October 3, 2022 22:11
@WanderLanz WanderLanz marked this pull request as draft October 3, 2022 22:12
@WanderLanz WanderLanz closed this Nov 9, 2022
@WanderLanz
Copy link
Author

@WanderLanz WanderLanz reopened this Nov 11, 2022
@WanderLanz WanderLanz marked this pull request as ready for review November 11, 2022 02:56
joshka added a commit to joshka/crossterm-winapi that referenced this pull request Jun 22, 2024
This replaces crossterm-rs#26
Using the windows crate instead of the windows-sys crate simplifies the
code and avoids having to maintain code that handles errors, and various
other simplifications provided by the higher level crate.

From the windows crate documentation:

> The windows-sys crate is a zero-overhead fallback for the most
> demanding situations and primarily where the absolute best compile
> time is essential. It only includes function declarations (externs),
> structs, and constants. No convenience helpers, traits, or wrappers
> are provided.
@joshka
Copy link
Contributor

joshka commented Jun 22, 2024

I rebased this and changed to use the windows crate instead of windows-sys in #29. This simplifies some of the result handling and conversions between types. I haven't tested it yet as I use a mac. It compiles, and runs the CI tests I added in #28. The APIs seem as correct as I can make them without actual testing on a Windows machine.

@WanderLanz
Copy link
Author

WanderLanz commented Jun 23, 2024

@joshka Thank you for letting me know. I haven't used the higher-level windows crate in a long time, awesome to see it getting along.

It would be interesting to see if it helps the main crossterm crate as well. Timon seems pretty apathetic to migrating from winapi unless a issue arises, so any benefit is awesome.

@joshka
Copy link
Contributor

joshka commented Jun 24, 2024

@joshka Thank you for letting me know. I haven't used the higher-level windows crate in a long time, awesome to see it getting along.

It would be interesting to see if it helps the main crossterm crate as well. Timon seems pretty apathetic to migrating from winapi unless a issue arises, so any benefit is awesome.

Reading the other thread (and assuming positive intent), Timon's view seems more like a "don't fix what ain't broke" perspective combined with an acknowledgment that they don't have time to spend thoroughly testing this. Fundamentally changing a library like this that underpins one that has such a broad blast radius if it breaks is risky if done with limited time availability.

@WanderLanz
Copy link
Author

Oh definitely, nothing negative or anything. It's just one of those things that will always be low priority, just like msvc to ucrt, with Win32 ABI backwards compatibility guarantees and all that.

I was semi-expecting more ioctl changes in Win32 with the new APIs, but there doesn't seem to be better/faster ways yet, so any extra benefit helps motivate the migration that's all I was really saying.

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.

2 participants