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

Repl: rows implementation and fixes #1156

Closed
wants to merge 2 commits into from
Closed

Repl: rows implementation and fixes #1156

wants to merge 2 commits into from

Conversation

Bilal2453
Copy link
Contributor

@Bilal2453 Bilal2453 commented Sep 6, 2021

Fixes #1107
Fixes #1157
Fixes #1158

This PR tries to not change how readline worked in the past, while introducing tty rows to the system, this is achieved by refreshing the whole line with all of its rows (by making sure to always start writing from the left of the very first row). Unlike the way NodeJS's refreshLine clear the screen, this will only clear said line, which means we must handle each position change differently and separately; that might be unclean, but it is the best way available to us without having to rewrite the whole thing. I will walk you through each of those changes step by step.

  1. Editor.getCursorOffset: This is the way I am using to measure where exactly the cursor is, we could've used ESC[6n instead, or used the NodeJS of manually counting each character width, but both will require a good portion of code while the first requiring a bit of rewrite of how onKey works. This seems to be accurate as long as UTF-8 unicodes are not presented (more about that later). On the other hand, there is no need for NodeJS's way of doing it since Luvit repl does not allow tabs characters in a row entered by the user. Although do note that this makes use of Editor.columns which is NOT updated on resize events, meaning all you need to break this is resizing the terminal; I do plan on implementing this later on.

  2. refreshLine's command: Couple required changes were made here, first of them forcing a new line to fix # , making sure the cursor goes to a new line when needed. Second of them is using ESC[0J which is not required but it makes more sense, plus it simplifies handling the deletion of first character of a row.

  3. refreshLine's row refreshing: This will basically force the cursor to go up first initial line, to start the rewrite from there. As you can see because of the way we are handling this we will have to handle each case separately, having to handle a row up (moveLeft) and a row down (moveRight).

  4. Editor.insertAbove: Since the user might trigger this when they are at first row while having more rows, we need to handle this a bit differently than used to. Neither this nor previous implementation handled more than a single row insertion.

  5. Editor.insert: Making sure the cursor is always visible, trigger a refreshLine when a new row is required.

  6. The rest of small stdout:write (ones as in here) were required to handle each case separately. Maybe there is a cleaner way of doing it?

  7. readline's finish: Make sure results are displayed right when the user hit enter at a row above the last row.

  8. Editor.pervRow: Required to track the movement of the cursor, this is the same as NodeJS's implementation.

@Bilal2453
Copy link
Contributor Author

Bilal2453 commented Sep 6, 2021

Lastly, I need to mention when this fix will not work.

  1. If a Unicode is used in the line, this seems to be true for both this fix and previous Luvit implementation. Not sure why but it seems like Luvit messes the count on it or something. It does not cause big problems although. This seems to be handled correctly on NodeJS.
    With this commit's behavior
    Latest Luvit release behavior (Notice how it uses the second row even though it can fit one more character)

  2. When terminal is resized, this totally breaks everything. We must handle onResize and update Editor.columns.
    Footage of it breaking on resize

  3. When the line is spanned upon multiple pages, that's, no enough space to show all rows at once. The biggest deal seems to happen when insert-between is triggered. NodeJS suffer from the same bug.
    Inserting at the end of a second page
    Inserting at the middle of a second page
    NodeJS's behavior

  4. In some rare cases Editor.getHistory seems to eat a row, not sure why, was not able to replicate it a second time.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant