yjftsjthsd-g / keynav

retire your mouse.
Other
18 stars 3 forks source link

A few fixes, add `toggle-start`, merge other's contributions #9

Closed mgsloan closed 7 years ago

mgsloan commented 7 years ago

Also opened up this PR here: https://github.com/jordansissel/keynav/pull/23

I based my changes off of your fork, thanks for maintaining it! Figure it's best to reduce the overall divergence of forks, so opening this up to see if you would like to merge it in. It would also be good to get your review, I haven't written any C in ages.

yjftsjthsd-g commented 7 years ago

I'm glad that you've found my efforts useful:) My goal is mostly to be a unified maintained version of keynav so we don't all get split up, so nice to know it's working.

To the actual PR: At a glance I like everything in this PR, but 2 things need verification.

First, I'd like to make sure that this will still build with gcc and clang on glibc/GNU/Linux, muslc/GNU/Linux, and FreeBSD (portability is a particular goal of mine); I'd be surprised if there are issues, but in particular the asprintf fix should be checked.

Second, I need to take more time to look at the toggle-start bits; it looks fine at a glance, but I also don't read enough C regularly to say that without thinking about it for a while.

mgsloan commented 7 years ago

First, I'd like to make sure that this will still build with gcc and clang on glibc/GNU/Linux, muslc/GNU/Linux, and FreeBSD (portability is a particular goal of mine); I'd be surprised if there are issues, but in particular the asprintf fix should be checked.

Good point, I hadn't considered the portability of that change, or really what it does. There probably wouldn't be issues, since the #define could effectively be ignored unless it's a compiler that recognizes it. I looked into what this actually does, and now thinking it's not a good idea - https://stackoverflow.com/a/5583764/1164871 . Doesn't seem worth it considering that in this case all it fixes is a warning, but completely changes the definition of things, adds more things, etc.

I've rebased out that change.

yjftsjthsd-g commented 7 years ago

Yeah, the asprintf thing is something I've poked around before: https://github.com/yjftsjthsd-g/keynav/issues/6

Would you please add a section for toggle-startto keynav.pod? It's easier if we keep the manpage updated as we go. I'll test on various platforms to make sure everything works, then we should be good to go.

yjftsjthsd-g commented 7 years ago

I've verified that your version builds on a few of my systems:

System gcc clang
glibc/GNU/Linux works works
musl/GNU/Linux works works
FreeBSD untested works

I didn't have gcc installed on my FreeBSD system, but I'm willing to bet that it's fine.

mgsloan commented 7 years ago

@yjftsjthsd-g Added the docs!

yjftsjthsd-g commented 7 years ago

Looks good. Thanks for working on this:)