magmax / python-inquirer

A collection of common interactive command line user interfaces, based on Inquirer.js (https://github.com/SBoudrias/Inquirer.js/)
MIT License
1.02k stars 97 forks source link

Discussion: solving multiline text render issues #348

Open guysalt opened 1 year ago

guysalt commented 1 year ago

hi there,

i see there is a lot of issues about solving the render bugs that appear when a line is longer then the terminal width. i really want to help this repo to be better, and i think i have enough time to do so. so i'm wanting to try to fix these bugs and making pr's for this. and i wanted to ask in advance if it's ok before i'm starting to put a lot of hours in these. and also how can i communicate with you if i have any questions about anything in the code?

thank you :)

Cube707 commented 1 year ago

Hey, we would be happy to accept PRs on this (and its a really big issue and fixing would be a great advancement)

However keep in mind that this will probably more work, as it might require rewriting the renderer pretty much completely.


As for questions, feel free to ask questions you have here and if you have a working example, open a PR and we can discuss it there. I can also open a development branch for this feature, which you can target with your PRs (if you plan on multiple PRs)

My knowledge on the codebase is not super deep though. I mostly know about the handeling of input and the interactions with readchar (which I maintain). Staticdev is leaving the project right now and the original author is very rarly available.

guysalt commented 1 year ago

great!

yea i understand this will require a lot of work and rewriting and that why i ask before starting to work.

and i think it will be great to open a development branch that i can target!

thank you!

guysalt commented 1 year ago

https://github.com/magmax/python-inquirer/blob/7b0fe3248f2ba4968fbe796c5e1f02bc64343203/src/inquirer/render/console/__init__.py#L80-L82

https://github.com/magmax/python-inquirer/blob/7b0fe3248f2ba4968fbe796c5e1f02bc64343203/src/inquirer/render/console/__init__.py#L160-L165

hey,

there is a specific reason why the print_str formatting instead of just get the message and print it?

i think it will be better if print_str function just print the output and handle the position attribute. and everything that relate to the formatting will be outside the function...

Cube707 commented 1 year ago

I dont know why it was done this way.

I assume there where plans on making this more cabale of different output devices (for example, to files or non-tty consoles or similar). This way the theming would be present in all higher functions and only the last step would be specific to a rendering device and implement how to apply the theming.

But I think its safe to assume a TTY output and work from there (readchar also requires it for the most part)

guysalt commented 1 year ago

hey, do you want to open a development branch?

i have already some changes you can look at and i think it will be great to go over features/bug fixes one by one for convenience

i can do all the changes in one branch and open a pr to main, but it will be include more than one issue... so it's for your decision :)

Cube707 commented 1 year ago

No, i will open a dev branch. But I am currently on holiday and didn't bother to to it on mobile. Will do it in the weekend

Cube707 commented 1 year ago

@guysalt

new branch is at dev/renderer-update