Control cursor position and visibility via Frame

[Minoru]

This implements the idea I outlined in #91 (comment):

can't Frame have a field like Option<(u16, u16)>, which is the cursor's desired position after drawing the frame? None means the cursor is hidden, Some with a position means the cursor is shown and is positioned at the given cell.

The implementation deviates from the idea in two ways.

First of all, Frame is moved into the drawing closure — there is no way for Terminal::draw() to examine frame's fields afterwards. This could be resolved in two ways:

1. make Terminal::draw() require FnOnce(&mut Frame<B>). This is very clean, but it's a breaking change, so I decided against that.

2. store that Option<(u16, u16)> in the Terminal itself, and make Frame just pass the calls through. That's a bit of a hack, because I don't think it's Terminal's responsibility to store this data; it's Frame's job to describe what has to be drawn and how.

The second option is a non-breaking change, so I went with that.

That decision led to the second deviation. Terminal already has an API for controlling the cursor: hide_cursor(), show_cursor(), set_cursor(). These calls have immediate effect on the terminal (modulo stdout buffering). This conflicts with the proposed API, and could be resolved in two ways:

1. make the old API non-public. This doesn't seem right to me, since Terminal is supposed to be a low-level wrapper around the backend.

2. make the new API opt-in. Up until the first call to Frame::set_desired_cursor_position(), calls to draw() don't affect cursor visibility, so the user is free to use Terminal::hide_cursor() and Terminal::show_cursor(). After the first call to the new API, however, the cursor position and visibility is always enforced after each Terminal::draw() call.

The first option is not viable, so I went with the second one.

I think that in the long term, tui-rs should change the signature of Terminal::draw(), to move all of this to Frame, and make Terminal a completely low-level thing again. If that's the plan, it might make sense to make the new API on Terminal non-public now, so people don't depend on what is going to be removed. @fdehau, I'm also okay with re-doing the PR to change the signature now, if you're ok with a breaking change.

[teohhanhui]

store that Option<(u16, u16)> in the Terminal itself, and make Frame just pass the calls through. That's a bit of a hack, because I don't think it's Terminal's responsibility to store this data; it's Frame's job to describe what has to be drawn and how.

Actually the cursor is not (and should not be) part of what is being drawn.

[teohhanhui]

I think a better way would be to make it possible to "query" the widgets, so that we are able to calculate the position of the cursor after draw. Something of the sort would be needed for #166 anyway...

Taking inspiration from QtQuick, perhaps we could switch to a scene graph-based approach? (It's still immediate mode, if I understand correctly.)

[Minoru]

Actually the cursor is not (and should not be) part of what is being drawn.

I think that's debatable. When the cursor is visible, it becomes a part of the "picture", which is Frame's responsibility (as I understand it).

I think a better way would be to make it possible to "query" the widgets, so that we are able to calculate the position of the cursor after draw.

Widgets in tui-rs are destroyed after each draw, so I don't see how they can be queried after the fact. Perhaps their state could be queried?

I agree that cursor positioning can be decoupled from drawing, but I wonder if we should. Currently main loop might look like loop { terminal.draw(…); handle_input(); }. Do we really want users to add terminal.reposition_cursor(…) after the draw()? Why not integrate that into the draw()?

[fdehau]

the crate usually does not attempt to fix silently users' errors. In this case if the cursor is outside of the viewport then it should simply not be shown.

[Minoru]

Should I explicitly check for that and hide the cursor, or is it okay to just pass the coordinates down to Terminal::set_cursor, and let the backend deal with it?

[Minoru]

I went ahead and changed it so it just passes the coordinates down into Terminal::set_cursor(); see 65db02a.

[fdehau]

@Minoru I would advocate for a breaking change if that's make the api and the internal logic better. Moreover, I fail to see how this would be painful for end users other than changing some functions signatures. I would also not change the cursor visibility based on a value changed in the draw function. It was not required until now and I would prefer that we just make the previous behavior more approachable that trying to do too much for the user.

@teohhanhui I tend to agree with @Minoru on this. The cursor may be visible so it can and is considered by some users as part of the UI. This change should end up adding a single utility function that make this cursor position easier to manipulate. The cursor is not actually moved during the draw call but you register its desired position after it. If you have a strong argument against this I'm more than happy to hear it.

[Minoru]

I would also not change the cursor visibility based on a value changed in the draw function.

But then we're back to square one: the user can set the position from their draw() function, but to actually show the cursor, they have to call Terminal::show_cursor(), which is not accessible from Frame. I'm okay with splitting that into two functions on Frame (one for position, another for visibility), but there must be a way to control cursor visibility via Frame.

I would advocate for a breaking change if that's make the api and the internal logic better.

Okay! Re-doing it now.

[teohhanhui]

Actually I still don't understand what benefit this PR brings. For me as a user, I'd like to be able to say, "set the cursor in this widget, at (col, row)", for example when I'm doing a "text field".

So, for example:

let input_texts = vec![Text::raw(input_value.as_str())];
let input_paragraph = Paragraph::new(input_texts.iter())
    .cursor(input_value.chars().count(), 0);

Currently I have to store the area Rect for input_paragraph, and calculate the cursor position manually based on that (taking into account margins and borders).

[Minoru]

Let's run with your example. Paragraph::cursor() just stores the position in a private field in input_paragraph, right? It clearly can't do anything else, since it doesn't have access to the Terminal. Then you'd call Frame::render_widget(), which only has access to Frame and Rect. It logically follows that one of those should provide some API to control the cursor. This PR adds such an API.

It might make sense to add cursor() methods on all widgets for ergonomic reasons, but that's a job for another PR.

Currently I have to store the area Rect for input_paragraph, and calculate the cursor position manually based on that (taking into account margins and borders).

That a different problem. You can solve it by creating a new struct InputField, which stores the text, and implement Widget for it. Then you'd have access to Rect from inside the render() method. You'll still have to take care of margins and borders, though.

This PR resolves different problem: you can't change cursor's position from the function that renders your UI. You have to either capture a reference to Terminal (which is ugly), or store the cursor position somewhere and write code elsewhere to extract the position and apply it to Terminal. That second solution is ugly too, because your rendering is now split between two places in code for no good reason (I consider visible cursor a part of the render). Does that make sense, @teohhanhui?

[teohhanhui]

Hmm, yeah, you're right. This does open up the possibility of implementing the cursor method at the widget level.

You can solve it by creating a new struct InputField, which stores the text, and implement Widget for it. Then you'd have access to Rect from inside the render() method. You'll still have to take care of margins and borders, though.

I wouldn't do that because it's only a half solution, but I don't want to go off-topic too much, and it's already a solved problem in my app (by setting the cursor after draw).

[Minoru]

Upon re-reading the code I found that Frame is already passing calls through to the Terminal. So the implementation here wasn't a hack; it's in line with what was already there. Thus, I didn't re-do the API as I promised earlier.

Apart from addressing the feedback, I fixed two things:

• 3813606 corrects the docs to note that cursor is repositioned after Terminal::flush(), not Terminal::draw(). This might make a difference to people who are trying to do some low-level manipulations;
• b9b7680 fixes a bug where Terminal::flush() cleared the screen but didn't reset the cursor state. That's at odds with "re-render everything each time" approach, so I fixed that.

I did those as fixup commits to make it easier to review, but I'd like a chance to squash them before you merge.

[fdehau]

@Minoru I continue to believe that storing the cursor on the Frame instead of the Terminal would makes the internal logic a lot cleaner as it would avoid all the state machine. Given how other widgets work, we could say that if you call Frame::set_cursor(u16, u16) (we don't even need the Option here) in terminal.draw the cursor is shown at the given position otherwise it is hidden. The cursor position is set to None before the call to the drawing function, Frame::set_cursor registers a position as Some(u16, u16). When the drawing is done, if the position is Some we show the cursor and move it to the given position otherwise we hide it and do not issue any goto calls.

I'm also starting to wonder whether we could deprecate Terminal::show_cusor and Terminal::hide_cursor in favor of this new API as I fail to see a case where one would prefer to use those methods over a single call and the Terminal managing the visibility automatically.

[Minoru]

we could say that if you call Frame::set_cursor(u16, u16) (we don't even need the Option here) in terminal.draw the cursor is shown at the given position otherwise it is hidden

That sounds much better than what I have envisioned! I updated the code to match.

I'm also starting to wonder whether we could deprecate Terminal::show_cusor and Terminal::hide_cursor in favor of this new API

Well, since the new API is a breaking change, I removed the old API instead of deprecating it. It's a separate commit, so can be easily dropped if need be.

[Minoru]

crossterm_demo doesn't compile if I remove Terminal::show_cursor. I'll drop the commit where I removed those methods.

[fdehau]

You should not change the version, I'll take care of that when I release.

[fdehau]

Suggested change

	resulting_cursor_position: Option<(u16, u16)>,
	cursor_position: Option<(u16, u16)>,

given that there is no other cursor position, I think that we can drop the adjective.

[Minoru]

I wanted to emphasize that this position will become effective only after the frame is drawn, but I guess stating that in the docstring is enough.

[fdehau]

I've left some very last comments but I see nothing blocking the merge. Thank you for keeping with the long review process and sharing your thoughtful view on this subject. I think it will be a neat addition to this crate.

[Minoru]

Both comments are now addressed. Thanks for the review!