search

bczsalba / pytermgui

Python TUI framework with mouse support, modular widget system, customizable and rapid terminal markup language and more!
https://ptg.bczsalba.com
MIT License
2.23k stars 54 forks source link

Add `_Terminal.no_echo` context manager #20

Closed USLTD closed 2 years ago

USLTD commented 2 years ago

modified: pytermgui/ansi_interface.py

modified: tests/yamlload.py

USLTD commented 2 years ago

I tested code and it looks like I needed to import one more module.

USLTD commented 2 years ago

I tested code and it didn't worked. Looks like it needs more complex code. I have C++ code for echo_on and echo_off and I am trying to translate it into Python. If push comes to shove I will convert C++ code into Python module via extension. Looks like this PR will be on hold for a few days. 😔

bczsalba commented 2 years ago

Hey,

This is a bunch of huge changes that I once again feel are out of scope. I appreciate the hustle, but this is a rewrite of a lot of the underlying code for not much reason. For example, as far as I know the getch implementations in the library are perfectly stable, and the keys.py file isn't really needed, as we already have the _Keys class. I appreciate the whole driver architecture that projects like Textual use, but it's not what we have and changing it requires massive work for not much benefit.

bczsalba commented 2 years ago

I do however like the @export decorator, so I might add that at some point.

USLTD commented 2 years ago

Huh. Looks like I accidentally pulled request for commit branch. That was my private branch where I was refactoring, upgrading and optimizing pytermgui. Sorry for that. Although @export decorator is easy and you can vendor it by just copy-paste. Also your getch implementation is not optimized and needs improvement since it has unnecessary things added and consumes more memory.

Also I wanted to comment one of your comments where you said to prefer print than sys.stdout due it being unbuffered but print is just wrapper function and buffer/unbuffered part is determined by file argument (which is by default sys.stdout), although it can be forced with flush=True. So this means print("text") is the same as sys.stdout.write("text\n") which means that print is not buffered

Also it looks like a longer research will be needed before undrafting this PR. For now I will close this PR and reopen when finish to-push branch.

Also let's chat from now on GitHub discussions or Twitter (which I prefer) so we won't overload this PR with our messages. My Twitter username is UltraStudioLTD

bczsalba commented 2 years ago

Also your getch implementation is not optimized and needs improvement since it has unnecessary things added and consumes more memory.

This is kind of a big statement to make with nothing to back it up.

you said to prefer print than sys.stdout due it being unbuffered

That is not something I ever said. What I said was:

Writing to stdout will make it buffered, which is not what these functions are meant to be.

Minute difference I admit, but still misrepresents what I said. Also, your ideas in the changes I commented this on did not include a newline, which meant that they were functionally incompatible.

Also let's chat from now on GitHub discussions or Twitter (which I prefer) so we won't overload this PR with our messages. My Twitter username is UltraStudioLTD

I appreciate the message, but any major update to the library should be seen by everyone who might be interested, as such Twitter is not an appropriate place for it. There would not be this many messages if your PR stuck to the goal you yourself defined in the title.