Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Widgets/text area #4995
Widgets/text area #4995
Changes from 10 commits
8bc9da2
55ae723
e237387
83a9a19
3ce4f1f
fa68bbe
cb549ed
ff41a5e
2ed6dcb
58c8d9b
cdda92e
a307308
b9422dc
51ff6b7
1f75f37
ff76073
6384b21
6047b40
83f9df3
4250bb4
abebac7
c005846
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please document default pens
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest also passing the old text/old cursor position so callbacks that care don't have to keep that state themselves. Callbacks that don't care don't even have to include the parameter in their function signatures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will do it for the cursor, but I have doubts for it as default behaviour of
on_text_change
.The text can be potentially long and storing and passing a copy of it on every keypress can be bad for performance and memory, especially that I assume most of the tools will not need it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done for
on_text_change
, also tests add.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lua strings are immutable (a change to a string causes a new string to be allocated). If you take a reference to the string before modification and pass it to the callback along with the current (modified) string, there is no additional memory being used. There was already a string copy operation as a result of modifying the string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, as such - I am going to implement the
old_text
param.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done and tests added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should document whether string.char(10) will be interpreted as the cp437 glyph. It would be nice if it did, but that can be implemented later if it's not already true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added.
\n
will be removed from the text, not rendered as glyph.Its not so obvious for me that they should be rendered as glyph, what do u think that?
maybe precise what do u mean by glyph, so I am sure we are think about the same thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
\n is ◙ in cp437 - It has a graphical representation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, in CP437, single-string multiline text isn't a thing. We're creating a funky hybrid here that needs careful documentation around the handling of
string.char(10)
. You can use ◙ in a name, for example. Currently you cannot use that character at all in this widget (which is a perfectly fine compromise, I think), but if we are to useone_line_mode
TextArea widgets to replace EditField, we'd probably want to support the whole CP437 character set.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its not something I feel comfortable to do here, but I am open to do it in a follow up PR. Is it fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scrolling on cursor movement doesn't happen automatically? I'd expect this to be the default so the parent isn't required to implement on_cursor_change for a good default behavior. What are the use cases for on_cursor_change callback other than to call this function? If we had that "keep cursor visible when moving cursor" default functionality, are there any other use cases for this function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is default functionality for all default manipulation of TextArea by the user.
But there is
:setCursor
API where we allow the API consumer to manipulate the cursor.This function do not automatically scroll to the cursor, to allow API consumer to better control.
e.g. imagine someone want to paste a text to end of the textarea for some reason, but do not change user cursor and scroll position.
I had a similar case with
gui/journal
Table of Contents feature, but I do not remember now what it was exactly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I think I understand. How about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clear
->Clears
to be consistent with the tense of other function descriptionsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be adjacent to the word navigation feature
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be moved above Ctrl-U so mouse users know they can skip paying attention to the deletion keyboard shortcuts. Actually, on second thought, I'd split this list into two sections:
1st section: baseline functionality (features that players would expect from any text editor: text wrapping, mouse selection, copy/paste, undo/redo). We can probably reduce the amount of documentation dedicated to well-understood features, like Home and End. You can just mention that "the usual keyboard keys are supported, like Home, End, Backspace, Delete, etc. In addition, common mouse gestures are supported, like double click to highlight a word or triple click to highlight a line."
2nd section: keyboard power user shortcuts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not convinced, IMO we should keep all option described.
its quite think line what is "baseline" features and what now, it can be different for every user.
may proposition is to include and intro, like u proposed, but below it, in dedicated section, keep all shortcuts like now.
what do u think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
at least Text Selection in particular should be moved adjacent to the Mouse Control line so similar concepts are grouped. This list is long and feels unorganized
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improved, what do u think about current version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be adjacent to line navigation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be adjacent to Text Selection
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing final period (also true for sentences below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we can take this out and default to an empty list. We seem to have fixed the issue of backtick appearing in text boxes via the
INTERCEPT_HANDLED_HOTKEYS
feature that I added a while back. Testing shows that we can even removeignore_keys={'STRING_A096'},
fromgui/launcher
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be handled in the
getPreferredFocusState
override, otherwise there will be conflicts when there are multiple TextArea widgets in a single focus groupThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is
cursor_liny_y
supposed to becursor_line_y
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the outer parentheses are unnecessary and don't add clarity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is beautiful. such a clean way to implement deduplication of successive similar events.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably ok for now, but if larger history sizes are used, we should switch to using ring buffers for
self.past
andself.future