Terminal.Gui icon indicating copy to clipboard operation
Terminal.Gui copied to clipboard

Add example of auto scrolling TextView

Open tznind opened this issue 2 years ago • 9 comments

I've opened this PR to explore the best way to enable an 'auto scrolling' log style view. The two problems I encountered implementing the scenario were:

  • When updating the Text the cursor position is reset to 0
  • There doesn't seem to be a way to scroll to easily reposition the cursor (e.g. to the first Column of the last Row)

chat

I am a little nervous about my changes to TextView so am creating this as a draft. I did think that instead of adding void SetText(ustring value, bool resetCursorPosition=true) we could have a bool flag e.g. PreserveSelectionWhenChangingText. Also I'm not sure if this could result in unstable behaviour if clearing text etc (could result in invalid selections).

Or if there is a way to achieve this same effect without making any changes to TextView that would be ideal.

tznind avatar Jun 26 '22 09:06 tznind

Or if there is a way to achieve this same effect without making any changes to TextView that would be ideal.

Anyway there has to be any changes in TextView. Perhaps another option is to reposition the cursor after ResetPosition, but I think your solution is preferable. Like always, good stuff :-)

BDisp avatar Jun 26 '22 20:06 BDisp

Ok cool, I will stick with this approach then but leave as Draft for now as I want to add some unit tests to make sure theres no errors crept in where e.g. SetText and preserve selection results in illegal selection. Also expect preserving the selection but prepending the text (as opposed to adding after the current selection point) results in the selection changing but we can probably live with that.

tznind avatar Jun 27 '22 08:06 tznind

Ok cool, I will stick with this approach then but leave as Draft for now as I want to add some unit tests to make sure theres no errors crept in where e.g. SetText and preserve selection results in illegal selection.

I'm convinced that the selected text preceding the text added are preserved, but more some unit tests are always better.

Also expect preserving the selection but prepending the text (as opposed to adding after the current selection point) results in the selection changing but we can probably live with that.

As the text are always added at the end, probably the selected text also will be preserved, but if not I thinking we can live with that :-)

BDisp avatar Jun 27 '22 10:06 BDisp

So I originally made wrote this with SetText as public. This lets the user preserve the selection regardless of how they are changing the Text. However when writing tests I realized two things:

  • Preserving the selection when prepending doesn't make much sense as although the selected indexes are the same the text underneath has changed
  • When setting the text to something shorter or "" it is quite hard to work out if the selected indexes are still valid (since they track row/col/length not indexes into the string directly.

So I have instead made SetText private and added a new method AppendText which has the option to preserve selection. This still lets AutoScrollingText scenario work and avoids all of the above issues.

We can always revisit this later and even if we do make SetText public later on it is still nice enough to have the AppendText method there in the public API.

tznind avatar Jun 28 '22 15:06 tznind

I have made MoveStartOfLine public in this PR too. Should we also make other methods like MoveToEndOfLine public for consistency?

Or another option would be to make use of the keybinding system with a method on View like:

/// <summary>
/// Performs the requested command.  Returns true if
/// the command is of a Type supported by the view
/// and the command was successful
/// </summary>
public bool DoCommand(Command cmd)
{
	if (!CommandImplementations.ContainsKey(cmd))
	{
		return false;
	}

	return CommandImplementations[cmd]() ?? false;
}

This would give the user quite a bit more power to automate stuff in the views?

tznind avatar Jun 28 '22 15:06 tznind

I have made MoveStartOfLine public in this PR too. Should we also make other methods like MoveToEndOfLine public for consistency?

I think so, makes sense to allow use all the move methods.

This would give the user quite a bit more power to automate stuff in the views?

Good idea, that give more options to use all the key binding by a command.

BDisp avatar Jun 28 '22 16:06 BDisp

Also, no matter what I do, I can't get selection to work. The selection is cleared automatically.

https://i.imgur.com/i8Pq1Dq.gif

This was already fixed by the PR #1840. Maybe @tznind could merge the fix into this PR.

BDisp avatar Jun 30 '22 19:06 BDisp

Also, no matter what I do, I can't get selection to work. The selection is cleared automatically. https://i.imgur.com/i8Pq1Dq.gif

This was already fixed by the PR #1840. Maybe @tznind could merge the fix into this PR.

My bad for not merging main before testing his PR. I've just verified you are correct.

tig avatar Jun 30 '22 19:06 tig

Thanks for the review, theres quite a lot to respond to. I will try to find some time in the next week or two to address the issues you raised.

tznind avatar Jul 01 '22 16:07 tznind

@tznind I'm marking this as Draft so I don't keep checking it ;-).

tig avatar Oct 20 '22 14:10 tig