Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor and fix multiline text bug #355

Merged

Conversation

guysalt
Copy link

@guysalt guysalt commented Feb 13, 2023

commit 1 - ConsoleRender: remove redundant parameter from _print_status_bar:

  • unused parameter

commit 2 - ConsoleRender: remove redundant code in _print_header:

  • the print_header printed every time new line and then move up once. i don't know what the point of working like this. i tried to do some different uses and it's seems to be pretty redundant.

commit 3 - ConsoleRender: remove redundant round brackets:

  • just aesthetics commit. as for the blessed documentation, it's look like the right way to call ANSI clearing functionality is without round brackets

commit 4 - ConsoleRender: remove function _force_initial_column:

  • the _force_initial_column in the end of the event loop seems to be duplicate as after it we call the _relocate function who call the _force_initial_column again.
  • then, remove function _force_initial_column as it's just print \r and used once on _relocate_ function. and add in _relocatefunction print to\r`.

commit 5 - ConsoleRender: adding handling of lines bigger than terminal width:

  • add clearing eos (end of screen) in relocate.
  • remove clear_eos on start before event loop (as it happens right after).
  • adding calculating of the position in the print_str method

commit 6 - ConsoleRender: remove redundant terminal.clear_eol from print_line:

  • again, seems to be redundant as if i'm understanding it right all the time the line will be clear, because we are doing clear_eos on the start, and every line is a new line... (eol = end of line)

commit 7 - ConsoleRender: make render_factory static:

  • unused parameter self

@guysalt guysalt changed the base branch from main to dev/renderer-update February 13, 2023 22:56
@Cube707
Copy link
Collaborator

Cube707 commented Feb 15, 2023

@guysalt what is the target of this PR? the mutliline renderer doesn't seem to work?

@Cube707
Copy link
Collaborator

Cube707 commented Feb 15, 2023

oh, I see now. You only added support for to long messages, not messages containing manual linebreaks

@guysalt
Copy link
Author

guysalt commented Feb 15, 2023

@Cube707

yes...

there is too many issues about this problem, and as you can see blessed had all the tools i need to do this.

the major challenge was to understanding what's going on there, and tbh there is a lot of mess there. that's why the next pr is mostly about refactor the code.

what do you say? are you approving it?

@guysalt
Copy link
Author

guysalt commented Feb 15, 2023

#162 #115 #26

@Cube707
Copy link
Collaborator

Cube707 commented Feb 16, 2023

Yes I know the issues, but it wouod be great if you added some more context to your commits, either in the commit messages or in the PR introducing theme. Its really hard for me to see why you introduced these changes (why is stuff redundant? why did you make a methode static? Etc.). I didn't write the original code and its really hard for me to gage your reasoning and its impact. So please edit either the commit messages or this PRs info.

As for the fix: its only a partial fix that only applies to lines longer that terminal.width and not to lines containing manual \n (see #162). This doesn't need to be fixed in this PR, but needs to be clearly stated to make the limitations of the current state clear. This defnetly requires a adjustment of the commit message.

Thanks for your hard work on this 👍. You are absolutely right in that its quite the mess. Thats why I never dared to touch it 😅. Defnetly feel free to rework it completly... don't forget tests though

@guysalt
Copy link
Author

guysalt commented Feb 17, 2023

After i dived in this code, i definitely can add supporting of line containing manual line feed (\n). it's not something i encounter with like the supporting of lines longer than terminal.width, but i can do it later. it's just i have already 2 more PR waiting to be open 1 by 1 so i want to finish with them first (the first one just to organize the code and make it more comprehensible, and second one for adding trim options)

Regarding the pull request commits, the code appears to be messy and disorganized, so to be honest i didn't understand what the purpose of some of the lines. Anyway the primary goal of these changes is to modify the logic related to controlling the screen and managing the re-render process. And to accomplish this, i removed any unnecessary code after i changed the logic.

i would try now to edit the commit to be more verbal for your CR :)

@guysalt guysalt force-pushed the refactor-and-fix-multiline-text-bug branch from a066700 to 63af9ac Compare February 17, 2023 23:14
@guysalt
Copy link
Author

guysalt commented Feb 17, 2023

ok so i update the PR first comment to describe each commit reason. i did it instead of editing the commit's messages because i think the commit's messages need just to describe the change and not the reason.

also, i edit the commit message from ConsoleRender: adding supporting of multiline text to ConsoleRender: adding handling of lines bigger than terminal width as you wish

thanks for your time :)

@Cube707
Copy link
Collaborator

Cube707 commented Feb 18, 2023

Note: I beleve the idea of using clear_eol for every line printed was to ensure it would properly work even in the case print_line() is used spereatly from the "normal" reprinting steps. (the function-name implies that a line gets printet, so it should ensure a clean result on itself...)

I believe it would make sense to rework it to only use clear_eol and not use clear_eos at the beginning. That way we could (with this update or later) rework the renderer to support single, per line updates, which would be cool.

Copy link
Collaborator

@Cube707 Cube707 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Cube707 Cube707 merged commit bf4d495 into magmax:dev/renderer-update Feb 18, 2023
@guysalt
Copy link
Author

guysalt commented Feb 18, 2023

great!
let's go to the next PR :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants