C types #32
C types #32
Conversation
|
Can you rebase for the tests to pass ? (I added a timeout so they would at least crash. I had to crash them by hand here) |
|
I rebased so this can be merged, but it isn't yet ready - I haven't looked at the windows code. |
changes some instances of u32 to c_int when calling functions take c_int as an argument. c_int is an alias for u32 on 32-bit and 64-bit linux, so this has little practical effect, but it makes intent clear and may improve portability to other, more exoctic systems.
change keycodes type to CGKeyCode in an attempt to improve portability
decl_keycodes makes keycodes.rs smaller by replacing the 3 repeditive blocks of keycodes and conversion functions with a macro and one large block of keycodes.
decl_keycodes declared consts that were not used elsewhere. This was exactly how the non-macro version did it for readability, but the macro improves readability enough that the consts aren't needed.
Changed all instances of i32 being passed to function taking c_int to c_int, and not an i32, despite those being the same on 32 and 64-bit linux. (similar steps for all integer types). All instances of Result<T, ()> were replaced with Option<T>, and some error handling code was made more brief.
X11's "BYTE" isn't guaranteed to be the same as Rust's u8. For example, on some architectures, char is 16 bits (mostly DSPs)
Narsil
left a comment
There was a problem hiding this comment.
Some nits here and there, but it feels like much better code.
.github/workflows/rust.yml
Outdated
| headless: Xvfb :99 -screen 0 1024x768x24 > /dev/null 2>&1 & | ||
| - os: ubuntu-latest | ||
| dependencies: sudo apt install libxtst-dev | ||
| dependencies: sudo apt-get install libxtst-dev |
There was a problem hiding this comment.
I would rather we keep apt instead of apt-get.
(https://itsfoss.com/apt-vs-apt-get-difference/)
There was a problem hiding this comment.
apt-get maintains backward compatibility, whereas apt does not - it is designed for humans source. Using apt-get in scripts is a best practice. It should actually be sudo apt-get install libxtst-dev --assume-yes
There was a problem hiding this comment.
Fair enough ! I learned something.
|
I wasn't done! I'm still working on windows! |
|
Oh sorry, can you push another PR ? I thought you wanted another PR for windows. (Maybe put WIP in PR name, I usually use this to know that it is not meant to be merged) |
fixes #31
closes #35 also