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

term: obtain the cursor position via termios.h #11372

Merged
merged 16 commits into from Sep 6, 2021
Merged

term: obtain the cursor position via termios.h #11372

merged 16 commits into from Sep 6, 2021

Conversation

onerbs
Copy link
Contributor

@onerbs onerbs commented Sep 3, 2021

This PR solves a TODO:
Use termios.h, C.tcgetattr and C.tcsetattr directly, instead of using stty

...But I have a couple of doubts:

  1. The code inside a defer block is called on panic?
    'cuz this implementation assumes a yes, a no might cause unwanted results
    (i.e. not restoring the initial terminal state)
  2. Would it be better to use C.printf('\e[6n'.str) instead of print('\e[6n')? (L63)
    according to my tests it doesn't seem to make a difference, but I am not an expert.
    ('m thinking about performance here, but I think no one will call this function so many times to notice 🤷🏻)

Cheers !
😺

@spytheman
Copy link
Member

It would be better to make the function return an ?Coord and use return error('description') instead of the panic() calls.

@spytheman
Copy link
Member

The code inside a defer block is called on panic?

Currently it is not.

@spytheman
Copy link
Member

Would it be better to use C.printf('\e[6n'.str) instead of print('\e[6n')? (L63)

Pure V is cleaner imho.

@onerbs
Copy link
Contributor Author

onerbs commented Sep 3, 2021

So, by making the get_cursor_position return an optional for *NIX it leaves the same function for Windows with a different signature. A possible solution would be:

pub fn get_cursor_position() ?Coord {
	if os.is_atty(1) <= 0 && os.getenv('TERM') != 'dumb' {
		return Coord{}
	}
	info := C.CONSOLE_SCREEN_BUFFER_INFO{}
	if !C.GetConsoleScreenBufferInfo(C.GetStdHandle(C.STD_OUTPUT_HANDLE), &info) {
		return error(os.get_error_msg(int(C.GetLastError())))
	}
	return Coord{
		x: info.dwCursorPosition.X
		y: info.dwCursorPosition.Y
	}
}

But there are more functions that use the GetConsoleScreenBufferInfo function. When making the change above, should all other functions be updated as well? (I'm thinking of a no)

Also,

In term.ui there's a function called get_cursor_position. I understand that the module was developed independently and then got merged... I think term.ui should import term, but that's another topic.

@spytheman spytheman self-assigned this Sep 6, 2021
@spytheman spytheman merged commit af28d09 into vlang:master Sep 6, 2021
@awmorgan
Copy link

awmorgan commented Sep 6, 2021

cc -std=gnu99 -w -o ./v ./vc/v.c -lm -lpthread
./vc/v.c:30104:84: error: field designator 'c_line' does not refer to any field in type 'struct termios'
old_state = (struct termios){.c_iflag = 0,.c_oflag = 0,.c_cflag = 0,.c_lflag = 0,.c_line = 0,.c_cc = {0},};
^
./vc/v.c:30109:95: error: field designator 'c_line' does not refer to any field in type 'struct termios'
struct termios state = (struct termios){.c_iflag = 0,.c_oflag = 0,.c_cflag = 0,.c_lflag = 0,.c_line = 0,.c_cc = {0},};
^

medvednikov added a commit that referenced this pull request Sep 6, 2021
spytheman added a commit to spytheman/v that referenced this pull request Sep 6, 2021
@onerbs onerbs deleted the cursor_position branch September 16, 2021 01:35
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.

None yet

3 participants