ssokolow / quicktile

Adds window-tiling hotkeys to any X11 desktop. (An analogue to WinSplit Revolution for people who don't want to use Compiz Grid)
https://ssokolow.com/quicktile/
GNU General Public License v2.0
874 stars 79 forks source link

crashes on particular hotkeys like " Mod1 + Mod4 + , " #18

Closed wmax closed 11 years ago

wmax commented 11 years ago

The Error I get after pressing the hotkey:

Xlib.protocol.request.QueryExtension Traceback (most recent call last): File "/usr/bin/quicktile", line 577, in handle_xevent self.wm.doCommand(self.keys[keycode]) KeyError: 59

My current environtment:

Linux bodhi32-ThinkPad-X61 3.5.0-11-generic #11 SMP Wed Aug 22 14:45:14 CDT 2012 i686 i686 i386 GNU/Linux

I'm using e17, feel free to ask me for further information

ssokolow commented 11 years ago

I'll be busy for the next few days but I'll get on this as soon as I can.

My first impression is that, somehow, it's set up a situation where it's bound an X event but didn't put the keycode for it into self.keys. (Since KeyError would be referring to the self.keys[keycode] dictionary lookup)

wmax commented 11 years ago

hi, today i tried to provoke the same issue on my other computer which is running ubuntu 11.04 (32bit) this time the script was not crashing but ignoring the shortcuts "Control + Mod1 + , " and " Control + Mod1 + . " I tried out several other modmasks like "Control + Shift + Mod1" or "Mod1 Mod4" or "Control Mod4"

but the script's output stayed the same no matter which modmask I am using aslong the buttons "," and "." are concerned:

./quicktile.py -d Xlib.protocol.request.QueryExtension WARNING: One or more requested keybindings were already in use and could not be bound.

wmax commented 11 years ago

FIXED IT, at least for my case :)

i modified this line: self.keys = dict([(self.xdisp.keysym_to_keycode(string_to_keysym(x)), self._keys[x]) for x in self._keys])

to: self.keys = dict([(self.xdisp.keysym_to_keycode(ord(x)), self._keys[x]) for x in self._keys])

You can also switch the hashbang to:

!/bin/sh

"""": python2 -c "" 2>/dev/null && exec python2 $0 ${1+"$@"} python -c "" 2>/dev/null && exec python $0 ${1+"$@"} echo "Could not find a python interpreter." exit 1 """

with this hashbang your script gonna run even on older systems like mine ( ubuntu 11.04 ) because mine was linking python2.7 to python and not python2

found this cool trick at: http://stackoverflow.com/questions/12606333/scripting-hashbang-how-to-get-the-python-2-interpreter

wmax commented 11 years ago

pushed those two changes into my fork of quicktiles :)

wmax commented 11 years ago

i realized a couple of seconds ago that my resolution creates another issue. now the script crashes if the shortkey consists of more than one character.

example:

, = bottom (works now) KP_1 = bottom (does not work anymore)

reason: the function ord(x) expects exactly one character

gonna try to fix this now :)

wmax commented 11 years ago

i think i fixed the issue now, just check out my branch of your app ;) The default hotkey config works again, no trouble with keys like KP_1. Hotkeys like "Mod4 + ," also work now, which makes me happy :)

The root of this issue lies in the function string_to_keysym(x) which is called in QuickTileApp._init_xlib(). It seems like string_to_keysym(x) is buggy because it returns zero for the following cases: 0 = string_to_keysym(',') 0 = string_to_keysym('.') this is why self.xdisp.keysym_to_keycode() ignores some hotkeys

so what I did is using ord() instead of string_to_keysym() for hotkeys which consist of one char only. since ord() cant handle strings like 'KP_1' the function string_to_keysym() will be used further on in those cases

ssokolow commented 11 years ago

Thanks for the hashbang trick. I didn't know that one. I'm still a bit too busy to really look into this, but I'll try to get my prior obligations done as quickly as possible.

As for string_to_keysym, it's not that it's buggy. It's just that the X11 key symbol tables are counter-intuitive on this point.

The reason it doesn't accept "," or "." is because it treats punctuation like the keys you really can't type (eg. arrow keys) and expects the actual words "comma" and "period" instead. (As evidenced by the XK_comma and XK_period lines in /usr/share/pyshared/Xlib/keysymdef/latin1.py.

(Because the key definition files are basically variable=value mappings, it looks up keys by prepending "XK_" to the string you gave, and you can't use punctuation in variable names.)

If using ord() works, then you just got lucky and the X11 devs got lazy because there's no guarantee that the X11 key symbol tables will use the ASCII value of every key for its keycode.

Unfortunately, I can't accept your fork in its current state for the following reasons:

  1. Each commit contains multiple unrelated changes. (eg. Changing the shabang, which I want, and adding ord(), which I don't want.)
  2. I can't accept a change using ord() because it'll break for non-ASCII symbols on non-US keyboards like the é on French keyboards.

However, I would accept changes which add a lookup table to translate "," to "comma" and so on before using string_to_keysym. You can use git gui to easily pick apart changes so each commit only contains the lines relevant to a given feature.

I've also been planning to experiment with using gtk.accelerator_parse since that'd further simplify things and allow me to reduce my dependency on Xlib to just the code for actually XGrabKey-ing a key. (The GTK+ developers are using the fact that Windows has nothing that behaves exactly like XGrabKey as an excuse to not include a cross-platform global keybinding wrapper)

ssokolow commented 11 years ago

If you'd like to experiment with them:

Using these should make it trivial to implement both the ModMask and individual keybindings using the same parsing function and to allow the individual keybindings to contain extra modifiers so you could do something like this for Ctrl+Alt+Win+C = move-to-center:

[general]
ModMask = <Control><Alt>

[keys]
<Super>C = move-to-center

Just do | (bitwise OR) on the modifiers from the two calls to gtk.accelerator_parse before binding the keys. I don't have time to do the testing right now or I'd already have implemented it.

wmax commented 11 years ago

Thanks for the kind information, especially the thing about "comma" and "period" was an eye opener. Gonna try that out and tidy up my fork so you can pull the hashbang.

I have to say that I totally appreciate this project because now I am independent of compiz since compiz's grid feature was the only one i realy cared about :)

But I cant promise you to tinker around on this project beyond my needs for now although its very tempting. The thing is that I must work hard on my CS courses to finish my bachelor thesis next semester.

ssokolow commented 11 years ago

No problem. Once I've got my current obligations out of the way, I'll probably be able to make time to fix whatever you didn't get around to.

wmax commented 11 years ago

hi, i've just pushed a new version to my fork of quicktiles

status:

ssokolow commented 11 years ago

Better, but you didn't rewrite the git history to give me nice, clean commits to merge. That's understandable (it's a more advanced skill and breaks things if anyone has already pulled a copy of the old version) but it does mean I can't just merge it right this instant.

If you don't mind waiting until around New Years Day, I can take them as they are and adjust as necessary.

If you'd like to experiment with a more advanced git feature (that should only be used if you're sure nobody else has pulled your public repo), you can rewrite previous commits using these commands:

git rebase -i HEAD~5
git push -f

The first lets you rewrite the five most recent commits while the second bypasses the "rewriting history will cause merry hell for anyone who has git pulled your public repo" error.

Also, it's something I really have no problem doing myself, but KEYLOOKUP needs work:

wmax commented 11 years ago
  1. i think i fixed my commits now
  2. by 'plus' i meant the button left to the enter button on a german keyboard which does not belong to the keypad this button works as intended on my mashine
  3. gonna extend the kemap later on :)
ssokolow commented 11 years ago

Almost. The unwanted bits are gone but it's now all collapsed into a single commit.

Just use git gui's "amend" mode to unstage the non-shebang stuff from the most recent commit, then commit it separately and use git push -f again.

As for 'plus', I stand corrected. Bit of an irony since I was using non-U.S. keyboards as an example of why it was necessary. :P

wmax commented 11 years ago

with the help of http://stackoverflow.com/questions/1440050/how-to-split-last-commit-into-two-in-git i was able to restage my last commit and split it into two.

I sure learned some cool stuff about git in the process, which I started to use the day before yesterday because of your project.

Btw, my lucky guess would be that you are using a russian keyboard ;)

ssokolow commented 11 years ago

That works too. I prefer git gui because I can just right-click on a line and choose to stage/unstage just that line or diff hunk.

I'm about to go to bed (late) but I'll try out your changes tomorrow and merge them if everything looks OK.

As for my keyboard, I'm actually using a US 104-key keyboard with extra keys where I've remapped the right Alt and Control keys to AltGr and Compose for typing French accents and symbols like ¢ and ½. (Specifically, the original Logitech G15 ...though the closest I could find a picture of on short notice was AZERTY layout rather than QWERTY)

I'm an English-speaking Canadian in the process of learning French. Good guess on the origin of my family name though.

ssokolow commented 11 years ago

Oh, and, ideally, try to keep the first line of your commit messages under 80 characters in length so tools don't have to cut it short when displaying it.

You can always add more lines for supporting details but, ideally:

wmax commented 11 years ago

cool thanks for the hint, never thought about formalisms on commit messages :)

ssokolow commented 11 years ago

The really proper thing to do is limit the first line to 60 characters so that metadata can be prepended without exceeding 80 characters but, given that not even git gui warns about that, I consider it a little unrealistic.

Vim does, but I never write my commit messages in vim anymore.

(You caught me when I hadn't quite finished preparing for bed)

ssokolow commented 11 years ago

Your commits have been merged but I'm having trouble reproducing the original crash so I just wrapped the line from your traceback in an if/else check which gracefully displays an error message.