Skip to content
This repository has been archived by the owner on Apr 19, 2024. It is now read-only.

Fix PreviousLine and NextLine on linux #309

Merged
merged 1 commit into from
Dec 11, 2020

Conversation

System-Glitch
Copy link
Contributor

@System-Glitch System-Glitch commented Nov 6, 2020

References:

#300 #302 #307 #308

I think this change will be enough to make it work on both Windows and Linux. I have tested in cmd.exe on windows, and on Ubuntu Konsole and VSCode integrated terminal. I cannot test on MacOS though because I don't own a Mac.

Logic behind this change:

In cursor_windows.go, PreviousLine() calls Down() and HorizontalAbsolute(), so I just did the same for linux but without HorizontalAbsolute() because it is apparently not needed (Test it on MacOS and tell me if it works. If it doesn't, then I think adding a call to HorizontalAbsolute() would do the trick). This allows us to keep PreviousLine() and NextLine() and don't have a conflict in naming between the windows and other versions.

@AlecAivazis
Copy link
Owner

Thanks for getting this out so quickly. I am a bit confused tho, your original PR made the switch from NextLine to Down - doesn't this change have the same effect as before?

@System-Glitch
Copy link
Contributor Author

@AlecAivazis My previous PR replaced all occurrences of function calls for PreviousLine() and NextLine() with Up() and Down(). For Linux terminals, the result is the same. However, an unforeseen effect of that was that is broke the lib for windows because on windows you need HorizontalAbsolute(), which was not called anymore because of the replacement.

@AlecAivazis
Copy link
Owner

@System-Glitch - finally got my hands on a mac laptop. I will pull this down now and give it a shot

@System-Glitch
Copy link
Contributor Author

@AlecAivazis I got a friend of mine test it for me on mac and everything appears to be working correctly.

@AlecAivazis
Copy link
Owner

I was also able to confirm that the behavior is consistent across mac and windows. Thanks for being patient 👍

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

Successfully merging this pull request may close these issues.

2 participants