magmax / python-readchar

Python library to read characters and key strokes
MIT License
143 stars 45 forks source link

Fixed most issues on Windows/Python 3.5 + Moved the linux loop in the linux code file #13

Closed guiweber closed 2 years ago

guiweber commented 8 years ago

This pull request is an improvement over pull request #9, which hasn't moved in some time.

Almost all the special characters are now working (including the arrows). I moved the Linux character read loop to the Linux file, since it's not needed on Windows, cleaned up some code sections to better align with PEP8 and prevented a crash for characters that cannot be decoded.

This doesn't resolve the fact that two checks need to be made for cross-platform scripts (e.g. if (inp == key.DOWN or inp == "down") )

However, I would suggest making the Linux code work like the Windows code instead of the opposite. The way it's done for Windows, it is easy to both compare a key, and print it:

if inp == "down":
    print('you pressed' + inp)

but this is not so easy on Linux since the special characters are not translated to a readable string. Let me know what you think.

guiweber commented 8 years ago

P.S. I'm not able to install/run the test suite on my pc, the setup just crashes... and looking at the Travis build, I can't really tell what went wrong. So I'd be glad to fix the build errors, but I'll need a hand in identifying them.

magmax commented 8 years ago

Hello,

I've just fixed the dependency problem, so travis should work right now.

Could you kindly merge with master?

thank you.

guiweber commented 8 years ago

Hi, I just rebased as requested. Travis now works but all build fail even though the code works well in practice both under linux and windows. I suspect this is because:

Again, I would suggest making the Linux code work like the new Windows code instead of the opposite. The way it's done for Windows, it is easy to both compare a key, and print it:

if inp == "down":
    print('you pressed' + inp)

but this is not so easy on Linux since the special characters are not translated to a readable string.

suresttexas00 commented 7 years ago

I like this PR but it also has the un-needed "return buffer" statement addressed in my PR #21. However, it is now in a different file on this PR (the un-needed return is found at the end of readchar_linux.py.

May I also suggest not using 'buffer' as as variable name and use charbuffer (or similar) instead? Buffer shadows built-in name 'buffer'.

I am closing my PR; I hope this one gets pulled back to the branch...

guiweber commented 7 years ago

@suresttexas00 I just pushed a new commit with the improvements you mentioned. Would you mind testing it to make sure I didn't break anything? My linux machine is not working at the moment.

suresttexas00 commented 7 years ago

In this:

windows_keys = {
    # Single keys:
    '\\x1b': 'escape',
    ' ': 'space',
    '\\x85': 'f11',
    '\\x86': 'f12',
    '\\x08': 'backspace',

is it really necessary to have a "space" item? Shouldn't these items simply be replacement for non-ASCII, multicharacter keypresses?

That could play havok on someone's program if it did not check for a space explicity... a user might type "hello world" and the program sees "hellospaceworld". This was not an issue for the linux handler because SPACE was just \x20 (which is still just a space).

suresttexas00 commented 7 years ago

At the end of keys.py:


linux_keys = {
    # Single keys:
    CR: 'cr',
    ENTER: 'enter',
    BACKSPACE: 'backspace',
    ESC: 'escape',
    UP: 'up',
    RIGHT: 'right',
    DOWN: 'down',
    LEFT: 'left',
    PAGE_UP: 'page_up',
    PAGE_DOWN: 'page_down',
    F1: 'f1',
    F2: 'f2',
    F3: 'f3',
    F4: 'f4',
    F5: 'f5',
    F6: 'f6',
    F7: 'f7',
    F8: 'f8',
    F9: 'f9',
    F10: 'f10',
    DELETE: 'delete',
    CTRL_A: 'ctrl_a',
    CTRL_B: 'ctrl_b',
    CTRL_C: 'ctrl_c',
    CTRL_D: 'ctrl_d',
    CTRL_E: 'ctrl_e',
    CTRL_F: 'ctrl_f',
    CTRL_Z: 'ctrl_z',
    INSERT: 'insert',
    SUPR: 'supr',
    CTRL_ALT_A: 'ctrl_alt_a'
}

Most ctrl key and shift combos don't work on my Linux, so that seems to be all of them. Space is not there for which reasons I indicated.

I also do not think the special key is getting intercepted on Linux.

magmax commented 7 years ago

Hello.

I've reverted some changes that were problematic in failed release 1.1.1, so I released 0.8.

Please, fix the conflicts and travis and I will merge this. I'm afraid you used 1.1.1 as base and it was wrong, so maybe you have to change something.

Thank you

suresttexas00 commented 7 years ago

Just FYI, the travis problems are due to this code:

def readkey(getchar_fn=None, blocking=True):
    getchar = getchar_fn or readchar
    buffer = getchar(blocking)
    return buffer

The Travis test code substitutes a 'mock" get_char function in place of your (or mine) readchar()... so it does not every return more than a single character and it never uses the logic for parsing escape keys because we moved that to the readchar (or in my PR, a new get_char function).

I wasted much time trying to "fix" my code before I realized the test was not testing our code properly.

suresttexas00 commented 7 years ago

1) To make this truly cross platform, you WILL need to implement character constants that are unified between Unix and WIndows. Using hard coded things like '\x1b\x01` are not going to produce the same result between Linux and Windows because they use completely different codes. I think @guiweber has the right idea here and the readchar_linux.py needs to follow suit with Windows..

2) The Travis testing is not meaningful anymore: a) It is just feeding the character sequences to the readchar function, which in these pull requests is not where the parsing occurs. _Therefore, the test is not even using our code in reacharlinux/windows.py. b) The code is only testing Linux keycodings. c) also, some of the code testing for the Python2.7 tests are overly constricting. Here is an example where an unused import (which was on purpose for Linux) failed the check: image To get it to pass, I had to create an un-needed wrapper to "use" the import. So now we are designing the code to make our Travis happy, often at the expense of more bloated code.

I have, for the time being, adopted my own single module version that goes back primarily to the original Danny Yoo thread version with some of these elements we have experimented with here thrown in. I have included Danny Yoo's thread and your Readchar in the attributions/credits:

https://github.com/suresttexas00/readkey

I think most everything there has been tried here too at one point or another, but I borrowed a nice Windows 10 unit and updated a lot of key mappings; please have a look and see if this can be fixed. I would really like to see the non-blocking versions that can read ESC come back!

suresttexas00 commented 7 years ago

You are not going to be able to get it to work. The testing suite is not actually testing "real-world" conditions... look carefully at the travis test code and how the factory functions are doing the testing. It is too much for me to explain, but I have tried elsewhere in various comments.

guiweber commented 7 years ago

@suresttexas00 Indeed, I did see your other comments and I haven't tried to make it pass the Travis build. I'm hoping @magmax will eventually adapt the Travis test suite.

The issue I was referring to happens when using the package in my own code (you can run example.py to test a quick implementation). The code in example.py worked on Linux and Windows before the merge, but now it only works as intended on Windows.

suresttexas00 commented 7 years ago

The example.py is really the best way to test for now. Unfortunately, I don't have the time to test it at the moment. when I get the opportunity, I'll revisit it.