Fixed backspace and arrow support on Windows

[jhonatan-lopes]

Fixing #57, and two issues from Inquirer (#117 and #116).

For #57 and Inquirer's #116, I fixed the wrong code for the backspace, which should be "\x08" (the 'extra' bytes that appear on the string mentioned in the Inquirer's issue). This needs testing on other Windows machines to confirm, although it matches the ASCII/Extended codes on Microsoft's guide.

For the missing arrow support #117, the scan codes on readchar.py were wrong. I fixed them according to Microsoft's scan codes.

[Cube707]

DON'T merge this!

This PR introduces inconsistency making the Problem on Windows even worse. The lookup table will now fail for some key in every case. See #66 for details.

[jhonatan-lopes]

Hi @Cube707, could you please elaborate on how this PR introduces inconsistencies and makes the problems on Windows even worse? My team and I are using my fork (which led to this PR) for over three weeks now on multiple Windows machines and haven't seen any issues. Both python-reachar and inquirer are running well on Windows and Linux (although the PR didn't touch anything on the Linux side).

Please let me know if the PR introduced new bugs or broke backspace/arrow support on your machine.

As far as I'm aware, the PR fixes what it was set out to fix. If the issue is that the code base needs refactoring, that should be, in my opinion, a different PR. I followed the current code style on the repository on purpose, to keep changes to a minimum while still fixing the problem.

Now, if the other characters in the lookup table are wrong and you think that the code needs refactoring, I would suggest opening a new PR for that.

[Cube707]

the variable a can have two values that lead to the "special key" part of the code triggering, 0 (\x00) or 224 (\xe0).

The lookup table only containes values for a=224 and failes for a=0, which is what windows 10 seems to be using right now. (No idea where the 224 comes from). This is a Problem and needs fixing, see #66 for more about that.

Your PR now chages some of the values to work with a=0, but leaves the rest at the old values. No all version of windows don't work fully.

You are right that the arrowkeys now work as expected on current windows 10, but leaving the libary partly broken is a bad idea...

[jhonatan-lopes]

If that's the case, I reiterate my point that there is no reason to not approve this PR. As you said, support for the arrow key has been corrected for you and it is working on Windows 10 now.

However, if issues remain other than the support for backspace and arrow keys, in my opinion, they are beyond the scope of this PR and should be raised as a separate issue (as you have done), and corrected separately.

The PR didn't raise inconsistencies nor did it make the problem worse. It simply corrected the bugs.

[mlabuda2]

Have you tested it on the Unix platform?

[jhonatan-lopes]

Yes, I use my fork both on a Windows 10 and a Ubuntu 20.04 LTS machines with no issues. Although the code is modular and the PR didn't touch anything on the Linux/Mac files.

[jmkd3v]

Can this be merged? I am completely unable to use the library in its current state because of this. @magmax

[cafkah]

Still confused as to why this hasn't been merged. Is it really difficult to fix or what?

[jens-coding]

Same here, its urgently required for other python libraries to work, please create a temporary fix for this.

[magmax]

Sorry very much. I will try to release this as soon as possible.

[vanschelven]

Have you tested it on the Unix platform?

I would think "not really"... since backspace was "\x7f" previously, and worked, I have no idea how simply replacing it could ever work without some kind of if statement to distinguish between the 2 platforms.

[jhonatan-lopes]

Hi @vanschelven. If you check the code -- particularly the portion I have altered -- you will see that it is broken down by platforms. The portion where the scan codes come in is only used on Windows, so there is no possibility that the changes affected Linux or Darwin platforms. Note that key file (where the backspace code was altered) is only imported on win32 and cygwin platforms (file readchar.py):

elif sys.platform in ("win32", "cygwin"):
    import msvcrt

    from . import key
    from .readchar_windows import readchar

On the same file, also note that the xlate_dict is contained inside an if statement checking only for win32 and cygwin platforms.

However, just to be on the safe side, as I mentioned on a comment before, I did test it on a Ubuntu 20.04 LTS and it is working fine.

[vanschelven]

@jhonatan-lopes I see what you mean now...

I suppose whether this change is breaking depends on what you think valid assumptions about the privateness/publicness of the key.py module as an interface are. I had been pointed to this file "by someone on the internet" as a source of truth about special keys' character values. There was nothing in the file to indicate that it was a Windows-only file, nor was it made private in some way (in fact it is explicitly put in __all__).

Given those conditions, I think assuming "platform independent public interface" was not too crazy. But looking at the code as a whole it indeed seems to be windows-specific. Perhaps this could at least be clarified at the top of the file? The more explicit solution would be a rename to reflect the windows-ness of the file, but that would also break more people's code.

[Cube707]

I belive @vanschelven has a valid point here. As far as I am aware key.py is intentionaly exported to be used as a reference when using the libary. Multiple examples show usecases like this:

a = readchar()
if a == key.BACKSPACE:
    # do stuff

This usecase might break if the byte returned by readchar is not what the platform uses. So it is probably importand to find out why the old code was "\x7f" and if their are plaforms that require it.

[jhonatan-lopes]

@vanschelven and @Cube707 thank you for the clarifications, your points are absolutely valid and correct. I totally get it now.

I honestly hadn't considered the key.py module as a public interface to getting platform agnostic key codes (I suppose because of the narrow scope of my bug fix), but as @Cube707 has highlighted it makes total sense. In that case, yes, I believe that this PR might have fixed that functionality for Windows and broken it for UNIX.

With that in mind, I don't think it is possible to have a single mapping of all characters that is platform agnostic, such as key.py tries to do. The backspace example shows that. Maybe it is necessary to have specific codes for specific platforms. Please correct me if I'm wrong.

Also, if you have any suggestions, let me know.

[Cube707]

I actually started to going down that rabbithole after I read the comments here. I wanted to try if its posible to properly seperate all the windows/Linux stuff and only have the __init__.py decide what needs to be exported for the current system.

Its not the moste pretty thing right now, but you are welcome to have a look: https://github.com/Cube707/python-readchar/tree/seperate-platform