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

Aggregated contributions #23

Merged
merged 67 commits into from
Aug 21, 2018
Merged

Aggregated contributions #23

merged 67 commits into from
Aug 21, 2018

Conversation

mgsloan
Copy link
Contributor

@mgsloan mgsloan commented Aug 26, 2017

I wanted to make some changes to keynav, so I took a look at what forks existed. I merged the changes that looked quality to me, and then added some of my own. My changes:

  • Add toggle-start command, a combination of start and end based on if keynav is active

  • Fix a bug where commands are run if there prefixes match. E.g. c runs a bunch of commands and doesn't report an error.

  • Fix a bug where keys are not ungrabbed on clear, if there are multiple commands which begin with start

I'd offer to maintain, but I do not frequently write code in C, my preferred language is Haskell.

aszlig and others added 30 commits February 24, 2012 18:03
* make grid black and white with (almost) pixel-perfect align;
* do not show grid if area too small (< 4x4);
This way when one is not found, it doesn't stop the inclusion of the
rest.
The old path still works.
bug fix: failed when the last char in the config is not a newline

Merge remote-tracking branch 'shouya/master'
Merge marekjm's fork (mostly adding install target to Makefile)
Some support for XDG Base Directory.
Copy link
Owner

@jordansissel jordansissel left a comment

Choose a reason for hiding this comment

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

Neat! I wasn't aware there was so many patches/forks floating around.

I added some questions inline in the PR. I haven't tested this yet.

Thank you for putting this together.

keynav.c Outdated
* Let's remove those, as they cause breakage */
e->state &= ~(Button1Mask | Button2Mask | Button3Mask | Button4Mask | Button5Mask);

/* Ignore LockMask (Numlock, etc) and Mod2Mask (shift, etc) */
e->state &= ~(LockMask | Mod2Mask);

/* Ignore different keyboard layouts (e.g. russian) */
e->state &= ~(1<<13);
Copy link
Owner

Choose a reason for hiding this comment

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

Is there a constant definition for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point!
I think it makes more sense to have a whitelist of modifiers rather than a blacklist, made that change here: mgsloan@2128fd2

Makefile Outdated
CFLAGS+=$(shell pkg-config --cflags cairo-xlib xinerama glib-2.0 xext x11 xtst 2> /dev/null || echo -I/usr/X11R6/include -I/usr/local/include)
LDFLAGS+=$(shell pkg-config --libs cairo-xlib xinerama glib-2.0 xext x11 xtst 2> /dev/null || echo -L/usr/X11R6/lib -L/usr/local/lib -lX11 -lXtst -lXinerama -lXext -lglib)
LDFLAGS+=$(shell pkg-config --libs glib-2.0)
CFLAGS+=$(shell pkg-config --cflags cairo-xlib 2> /dev/null)
Copy link
Owner

Choose a reason for hiding this comment

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

Why split these up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure, that wasn't my change. It's been a long time since I've done much makefiles and C, so not sure what makes sense here. It works fine for me either way.

Makefile Outdated

keynav.o: keynav_version.h
keynav_version.h: version.sh

keynav: LDFLAGS+=-Xlinker -rpath=/usr/local/lib
keynav: keynav.o
$(CC) keynav.o -o $@ $(LDFLAGS) -lxdo; \
$(CC) keynav.o -o keynav $(CFLAGS) $(LDFLAGS) -lxdo
Copy link
Owner

Choose a reason for hiding this comment

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

why change $@ to keynav ? $@ is the target name, which is keynav in this case. I'm a little confused; can you help me understand?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also not my change, pinging @yjftsjthsd-g

Copy link
Contributor

Choose a reason for hiding this comment

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

My memory fails me, unfortunately. I may have changed it just because $@ was unfamiliar to me; looking now, it appears to be reasonably portable (the current version of the Makefile only works in GNU make, and I'd like to fix that), so I agree that it should be reverted. The only other minor concern would be use in ex. the debug target, but building a differently named debug binary isn't exactly a bad thing.

@@ -216,7 +217,7 @@ dispatch_t dispatch[] = {

// Mouse activity
"warp", cmd_warp,
"click", cmd_click,
"click", cmd_click,
Copy link
Owner

Choose a reason for hiding this comment

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

The trailing whitespace changes are making the diff harder to read for me, so I am likely to miss some things in review.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry about that, my emacs config automatically trims it.

Copy link

Choose a reason for hiding this comment

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

Maybe put all traling whitespace changes into a single commit at the very end of the list of patches.

@@ -225,7 +226,8 @@ dispatch_t dispatch[] = {
"daemonize", cmd_daemonize,
"sh", cmd_shell,
"start", cmd_start,
"end", cmd_end,
"end", cmd_end,
"toggle-start", cmd_toggle_start,
Copy link
Owner

Choose a reason for hiding this comment

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

++ this is a nice feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

char *user_config_file;

if (config_home &&
asprintf(&user_config_file, "%s/keynav/keynavrc", config_home) != -1) {
Copy link
Owner

Choose a reason for hiding this comment

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

asprintf is not available on Solaris (by default), but I suspect this is probably not a problem.

+1 to supporting XDG config pathing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool!

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the example of where asprintf will fail; I noted that it wasn't portable but I've only been testing on GNU and BSD systems. I've updated yjftsjthsd-g#6 to note that it'll break on Solaris.

keynav.c Outdated
if (*args == '\0')
args = "";
else
args++;

found = 1;
dispatch[i].func(args);

break;
Copy link
Owner

Choose a reason for hiding this comment

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

this is ok, probably easier to read if the for loop condition is changed dispatch[i].command && !found instead of breaking here. (should have the same effect, though)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! Made that change in mgsloan@0331365

A few fixes, add `toggle-start`, merge other's contributions
@mgsloan
Copy link
Contributor Author

mgsloan commented Sep 8, 2017

@jordansissel Hey! Thanks for the review. I will try to address this stuff next week, been busy. Most of those things are in code that other's wrote. My changes also got merged to https://github.com/yjftsjthsd-g/keynav

@mgsloan
Copy link
Contributor Author

mgsloan commented Sep 24, 2017

@jordansissel Hey! Sorry for the delay on this. I've added a couple commits addressing comments. Can't do much about the Makefile changes. Perhaps merge and add a commit putting the Makefile back, if you feel strongly about it?

@mgsloan
Copy link
Contributor Author

mgsloan commented Sep 24, 2017

Also just added a couple fixes that have been bugging me for years, particularly the xrandr thing. No longer will I have to kill and restart keynav when plugging in a display! Being a bit more familiar with the codebase helps.

@jordansissel
Copy link
Owner

jordansissel commented Sep 24, 2017 via email

yjftsjthsd-g and others added 3 commits September 30, 2017 20:19
This is a bit inconsistent with other functions, and apparently (I was surprised, too) breaks on the version of gcc shipped with CentOS, which will only accept it if you explicitly declare -std=c99.
Without this change, when a new display is added or an existing display changes
size, keynav doesn't use correct starting bounds for its rectangle.

I also sometimes get "BadWindow (invalid Window parameter)" from a call of
X_GrabKeyboard, consistent with using an invalid root window.
With xmonad (tiling window manager), I can make it so that no window is
selected, just by making an empty workspace active.  Running a keynav command
involving "windowzoom" in this context results in:

X Error of failed request:  BadDrawable (invalid Pixmap or Window parameter)
  Major opcode of failed request:  14 (X_GetGeometry)

This fixes that such that keynav no longer crashes in this case, but it does
output an error message:

XGetWindowProperty[_NET_ACTIVE_WINDOW] failed (code=1)

Looks like this is coming from xdotool -
https://github.com/jordansissel/xdotool/blob/a70547cf14ab31b3a2900f8bd1e8648ad3633b38/xdo.c#L712

I don't really mind the extra output.
@sojusnik
Copy link

@jordansissel when do you plan to finally merge those fixes? Can't wait to use the up to date and feature rich version of keynav!

This seems to fix the following X11 error

Error of failed request:  BadWindow (invalid Window parameter)
  Major opcode of failed request:  129 (SHAPE)
  Minor opcode of failed request:  1 (X_ShapeRectangles)
  Resource id in failed request:  0x0
@mgsloan
Copy link
Contributor Author

mgsloan commented Aug 20, 2018 via email

@jordansissel
Copy link
Owner

I haven't had much time for this project lately, so I appreciate your patience. The code overall looks good. I'll test it quickly and merge if it compiles and runs. :)

@jordansissel
Copy link
Owner

Alrighty I did a quick test and things seems to be working ok.

I think this is a backwards compatible change (Everything I tested worked with my existing keynavrc). Merging!

@jordansissel jordansissel merged commit 78f9e07 into jordansissel:master Aug 21, 2018
@jordansissel
Copy link
Owner

Thanks everyone for the combined efforts on these patches.

@mgsloan
Copy link
Contributor Author

mgsloan commented Aug 21, 2018

No worries, I know how it goes. Thanks for making keynav, I use it all the time. I got a logitech mx ergo wireless mouse 4 months ago and have never needed to recharge the batteries. Hah!

AutoHotKey (Windows), and AppleScript (macOS). If you find something that works,
let me know and I'll consider adding it to this list.

Q: Can I use keynav to scroll?
Copy link

Choose a reason for hiding this comment

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

I'm sorry if this information is easy to find, but is there a way to do control-click from keynav?
I tried putting "ctrl+1 click 1" in my .keynavrc but it had no effect - no click happens when i ctrl+1
(i also tried with ctrl-q and there's still no effect)
I just want to be able to control-click on something...am i missing something? thank you

@jordansissel
Copy link
Owner

jordansissel commented Oct 31, 2018 via email

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.