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

Fix extra newline on windows without decremented width. #601

Merged
merged 2 commits into from
Jan 2, 2018

Conversation

leak4mk0
Copy link
Contributor

Hello,
On Windows, extra newline is inserted if a newline is inserted at the right end of the terminal.
Terminal width is decremented by 1 to avoid this problem now.
I would like to propose a method by moving the cursor.
Thank you.

@SBoudrias
Copy link
Owner

Hey @leak4mk0, I've been putting aside reviewing this PR for a little while now. I read the code and I don't understand the bug you're having.

Can you post screenshots of the behavior before and after? And maybe an example to reproduce the issue you're having? Without this information, I can't really make an informed decision as to merge this PR and if this really is the proper fix for the issue. Thanks

@leak4mk0
Copy link
Contributor Author

leak4mk0 commented Nov 4, 2017

Thank you for your reading my PR.

First, I want to fix slipped cursor on win32.
This problem is occurred by decremented width.
master

So, I tried to remove decrement of console width on win32 (File: screen-manager.js, Line: 113).
But new problem is occurred and I understood why console width is decremented..
On windows, if there is newline at right end of console, extra newline appears.
fix-width

Therefore, I got an idea.
I tried to insert cursor up ("\x1b[1A") after newline, if there is newline at right end.
fix-line-control

@SBoudrias
Copy link
Owner

SBoudrias commented Nov 12, 2017

@leak4mk0 thanks for the explanation. Can you try to make the code a bit more understandable?

Right now this is a bit too confusing and quite unclear how it fix the issue.

I'd suggest assigning more descriptive variable names, stop using or at least removing nested ternary operators, and maybe split the core logics pieces into many functions.

Also, would the bug be fixed on windows if we simply do not insert \n at the line end inside breakLines? (I'd test it myself, but I don't have a windows computer/VM handy...)

@leak4mk0
Copy link
Contributor Author

leak4mk0 commented Nov 12, 2017

Thank you for your reply.

As you say, this code is not easy for reading, sorry.
I'll rewrite this code more simply.

I tried your way ("do not insert \n at the line end") before I tried cursor-up way.
But, duplicate lines appeared.
image

Tried code: (PoC code, all newlines are removed.)

diff --git a/lib/utils/screen-manager.js b/lib/utils/screen-manager.js
index e5d60fd..dbb70a0 100644
--- a/lib/utils/screen-manager.js
+++ b/lib/utils/screen-manager.js
@@ -110,7 +110,7 @@ ScreenManager.prototype.normalizedCliWidth = function () {
     output: this.rl.output
   });
   if (process.platform === 'win32') {
-    return width - 1;
+    // return width - 1;
   }
   return width;
 };
@@ -131,5 +131,5 @@ function breakLines(lines, width) {
 }
 
 function forceLineReturn(content, width) {
-  return _.flatten(breakLines(content.split('\n'), width)).join('\n');
+  return _.flatten(breakLines(content.split('\n'), width)).join('');
 }

@SBoudrias
Copy link
Owner

@leak4mk0 thanks! BTW I've made some major code upgrade in the repo today, might have created a few conflicts on your side.

# Conflicts:
#	lib/utils/screen-manager.js
@leak4mk0
Copy link
Contributor Author

I'm sorry for being late.
Most of this problem seems to be fixed in new screen manager.
So, I fixed only two points for Windows console.

  1. Remove extra right space on Windows. (screen-manager.js: 114)
  2. Prevent move cursor if cols equals to 0. (screen-manager.js: 76)
    Because behavior of cursorForward(0) looks like one of cursorForward(1) on Windows cmd.exe.

@SBoudrias SBoudrias merged commit 5f83804 into SBoudrias:master Jan 2, 2018
@SBoudrias
Copy link
Owner

Thanks a lot :)

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