search

jwlodek / py_cui

A python library for intuitively creating CUI/TUI interfaces with widgets, inspired by gocui.
https://jwlodek.github.io/py_cui-docs
BSD 3-Clause "New" or "Revised" License
759 stars 42 forks source link

Replace KEY_BACKSPACE by a list #154

Closed PabloLec closed 3 years ago

PabloLec commented 3 years ago

This PR tries to solve #74. Currently the KEY_BACKSPACE var is set either explicitly if win32 or darwin, or else by curses. The thing is curses might get a wrong backspace key on Linux depending on $TERM environment variable value and actual terminal conf.

To solve this issue, we might just catch as a backspace all possible values. Either 8 (ASCII BS), 127 (ASCII DEL) and any other value returned by curses.KEY_BACKSPACE.
This shouldn't cause any false positive as 8 and 127 are well defined, except if a user explicitly tweaked his terminal configuration for 8 and 127 which would cause trouble with curses anyway.

What this pull request changes

Issues fixed with this pull request

Optionally, what changes should join this PR

If this solution is accepted, I will need to edit docs reference of KEY_BACKSPACE.

PabloLec commented 3 years ago

Sure! I didn't think about that. Also, I made the name plural, KEYS_BACKSPACE as I saw other plural occurences of the word "key". But maybe it will not be really clear for the users as to why use a plural name for a single key. Also it will break backward compatibility. I guess I should either just rename it or make an alias, keep the KEYS to make it clear it's a list for maintainers but also refer it as KEY for users. (?)

jwlodek commented 3 years ago

Ah good thing you mentioned that as I didn't even notice :sweat_smile:

I think keeping it at KEY_BACKSPACE without the plural would be best - this way it's more transparent to the user, and won't break BC. Maybe something like _KEYS_BACKSPACE as an internal alias would be a good idea as well so it makes sense internally, but for someone using the library, KEY singular is more clear imo.

Also, this same add_key_command logic needs to be implemented in the Widget class, since the key commands can be applied to widgets as well

PabloLec commented 3 years ago

Yeah I'll just drop the "S". I'm not on my usual dev environment right now, I'll wait until tomorrow to make the changes (I don't want to miss any occurence). Just saying in case you want to implement this change in 0.1.4.

And will do for Widget too 😉

jwlodek commented 3 years ago

I actually just released 0.1.4, so this will have to wait for the next one. Hope the next release won't take as long, since I have more less ambitious features I want to add, and some bug fixes - no huge logging overhauls or mouse support additions

PabloLec commented 3 years ago

Ok, I think this one is ready for merge.