Widgets/text area #4995

wiktor-obrebski · 2024-10-08T18:27:29Z

This MR introduce TextArea widget.
This widgets is already in use in gui/journal and coming gui/notes tools.
Its provide much better support for text manipultation than TextField do.

Scripts journal has been migrated here:
DFHack/scripts#1327

library/lua/gui/widgets/text_area.lua

robob27 · 2024-10-09T08:31:34Z

I really appreciate your work on all of this stuff! You really went all out with this widget and the result is fantastic. Journal is sooo good and it's awesome that every other script will have such a great textarea to work with. I have been looking forward to this PR 😄

dhthwy · 2024-11-01T20:23:32Z

This PR looks good to me.

My understanding is that this type of stuff (especially lua, ui) is primarily myk's area of responsibility to sign-off on, so it may take some time to be merged.

myk002

first pass review done (docs only)

docs/dev/Lua API.rst

myk002 · 2024-11-15T00:06:11Z

docs/dev/Lua API.rst

+* ``on_text_change``: Callback function called whenever the text changes.
+  The function signature should be ``on_text_change(new_text)``.
+
+* ``on_cursor_change``: Callback function called whenever the cursor
+  position changes. The function signature should be ``on_cursor_change(new_cursor_pos)``.


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.

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.

Done for on_text_change, also tests add.

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.

You are right, as such - I am going to implement the old_text param.

Done and tests added.

docs/dev/Lua API.rst

myk002 · 2024-11-15T00:08:51Z

docs/dev/Lua API.rst

+  position changes. The function signature should be ``on_cursor_change(new_cursor_pos)``.
+
+* ``one_line_mode``: Boolean attribute that, when set to ``true``,
+  disables multi-line text features and restricts the text area to a single line.


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.

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.

\n is ◙ in cp437 - It has a graphical representation.

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 use one_line_mode TextArea widgets to replace EditField, we'd probably want to support the whole CP437 character set.

Its not something I feel comfortable to do here, but I am open to do it in a follow up PR. Is it fine?

docs/dev/Lua API.rst

myk002 · 2024-11-15T00:14:27Z

docs/dev/Lua API.rst

+
+    Scrolls the text area view to ensure that the current cursor position is visible.
+    This is useful for automatically scrolling when the user moves the cursor
+    beyond the visible region of the text area.


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?

If we had that "keep cursor visible when moving cursor" default functionality, are there any other use cases for this function?

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.

Ok, I think I understand. How about this?

Scrolls the text area view to ensure that the current cursor position is visible. - This is useful for automatically scrolling when the user moves the cursor - beyond the visible region of the text area. + This happens automatically when the user interactively moves the cursor or + pastes text into the widget, but may need to be called when ``setCursor`` is + called programmatically.

myk002 · 2024-11-15T00:17:10Z

docs/dev/Lua API.rst

+- Text Selection: Select text with the mouse, with support for replacing or
+    removing selected text.


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

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?

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

Improved, what do u think about current version?

myk002 · 2024-11-15T00:17:38Z

docs/dev/Lua API.rst

+- Jump to Beginning/End: Quickly move the cursor to the beginning or end of the
+    text using :kbd:`Ctrl` + :kbd:`Home` and :kbd:`Ctrl` + :kbd:`End`.
+- Select Word/Line: Use double click to select current word, or triple click to
+    select current line


missing final period (also true for sentences below)

Also add tests for TextArea `on_cursor_change` callback

myk002

Please add a line to the Lua section of the changelog.

To allow me to see the diff between reviews, could you keep the commits without force pushes? Force pushing causes GitHub to lose the review context.

myk002 · 2024-11-21T12:37:06Z

docs/dev/Lua API.rst

+The cursor in the ``TextArea`` class is index-based, starting from 1,
+consistent with Lua's text indexing conventions.
+
+Each character, including new lines (``string.char(10)``),


Suggested change

Each character, including new lines (``string.char(10)``),

Each character, including newlines (``string.char(10)``),

myk002 · 2024-11-21T12:40:18Z

docs/dev/Lua API.rst

+* ``init_text``: The initial text content for the text area.
+
+* ``init_cursor``: The initial cursor position within the text content.
+    If not specified, defaults to end of the text (length of ``init_text``).


length of init_text plus one, right? (according to the Cursor Behavior text above)

myk002 · 2024-11-21T12:41:12Z

docs/dev/Lua API.rst

+* ``text_pen``: Optional pen used to draw the text.
+
+* ``select_pen``: Optional pen used for text selection.


Please document default pens

myk002 · 2024-11-21T12:48:06Z

docs/dev/Lua API.rst

+    If any "\n" (``string.char(10)``) are available in the text,
+    they will be removed from the text


I find this phrasing a unclear for the semantics I expect. This is the behavior I'd like to see described:

If multiline text is pasted into the widget, newlines are removed (that is, the non-multiline SDL clipboard functions are used)

The Enter key is not handled by the widget as if it were included in ignore_keys

myk002 · 2024-11-21T12:52:24Z

docs/dev/Lua API.rst

+
+    Scrolls the text area view to ensure that the current cursor position is visible.
+    This is useful for automatically scrolling when the user moves the cursor
+    beyond the visible region of the text area.


Ok, I think I understand. How about this?

Scrolls the text area view to ensure that the current cursor position is visible. - This is useful for automatically scrolling when the user moves the cursor - beyond the visible region of the text area. + This happens automatically when the user interactively moves the cursor or + pastes text into the widget, but may need to be called when ``setCursor`` is + called programmatically.

myk002 · 2024-11-21T14:01:14Z

library/lua/gui/widgets/text_area/text_area_content.lua

+TextAreaContent.ATTRS{
+    text = '',
+    text_pen = COLOR_LIGHTCYAN,
+    ignore_keys = {'STRING_A096'},


same comment as above -- I think we can change the default to an empty list due to INTERCEPT_HANDLED_HOTKEYS

myk002 · 2024-11-21T14:06:48Z

library/lua/gui/widgets/text_area/text_area_content.lua

+    enable_cursor_blink = true,
+    debug = false,
+    one_line_mode = false,
+    history_size = 10,


the internet does not seem to have a good consensus of what a 'good' default history size is, but 10 seems low to me. How about 20 (or leave the attribute as DEFAULT_NIL and use the HistoryStore default of 25)?

myk002 · 2024-11-21T14:11:38Z

library/lua/gui/widgets/text_area/text_area_content.lua

+function TextAreaContent:getPreferredFocusState()
+    return true
+end


it doesn't make sense for both TextArea and TextAreaContent to want focus since TextAreaContent is a child of TextArea. This function should be removed since it is TextArea that really needs the focus.

myk002 · 2024-11-21T14:20:03Z

library/lua/gui/widgets/text_area/wrapped_text.lua

+
+function WrappedText:update(text, wrap_width)
+    self.lines = text:wrap(
+        wrap_width,


if wrap_width is nil, then the text will be wrapped at 72 characters. Is this appropriate? Should wrap_width instead default to math.huge?

myk002 · 2024-11-21T14:22:35Z

test/library/gui/widgets.TextArea.lua

+
+config.target = 'core'
+
+local df_major_version = tonumber(dfhack.getCompiledDFVersion():match('%d+'))


now that 50.14 has been released (which supports the new cursor navigation keybindings), you can take this out and run the tests unconditionally

dhthwy · 2024-11-21T23:30:43Z

You force pushed him again. lol.

myk002 · 2024-11-22T02:05:28Z

I should clarify that force pushing is ok to amend a just-committed commit. The review complications happen when force pushing such that already-reviewed commits get a new hash.

myk002 · 2024-11-22T02:14:01Z

docs/dev/Lua API.rst

-    they will be removed from the text
+* ``one_line_mode``: If set to ``true``, disables multi-line text features.
+    In this mode the :kbd:`Enter` key is not handled by the widget
+    as if it were included in ``ignore_keys``. 


Suggested change

as if it were included in ``ignore_keys``.

as if it were included in ``ignore_keys``.

myk002 · 2024-11-22T02:18:10Z

docs/dev/Lua API.rst


-Functionality:
+Functionality
+-------------


welcome to the wonderful world of restructured text header underline rules. Currently, this section is interpreted like this:

that is, Functionality is a peer of TextArea class and Cursor Behavior is a higher level header than TextArea class.

You need to scan through the existing headers and discover what underline style is used for what level of header. It's different for every file, and is determined by whatever the file uses first for that particular header depth.

The indentation for list continuation lines is also apparently different from what sphinx expects:

You can see how this PR's text is currently rendered here: https://dfhack--4995.org.readthedocs.build/en/4995/docs/dev/Lua%20API.html#textarea-class

You can get to that link by clicking on "details" for the ReadTheDocs build action:

Thank for the tip, I did not know that I can preview docs in build results!
I corrected the docs now.

myk002 · 2024-11-22T02:23:21Z

docs/dev/Lua API.rst

 - New Lines: Easily insert new lines using the :kbd:`Enter` key, supporting
    multiline text input.
 - Text Wrapping: Text automatically wraps within the editor, ensuring lines fit
    within the display without manual adjustments.
+- Scrolling behaviour for long text build-in.


Suggested change

- Scrolling behaviour for long text build-in.

- Scrolling for long text entries.

myk002 · 2024-11-22T02:24:33Z

library/lua/gui/widgets/text_area.lua

    end
 end

 function TextArea:getPreferredFocusState()
-    return self.parent_view.focus
+    return self.parent_view and self.parent_view.focus or true


why not just true?

I read the docs, but I was always confused by this "preferred focus" feature.
Changed to true

wiktor-obrebski added 2 commits October 8, 2024 19:02

Create TextArea widget, based on gui/journal widget

8bc9da2

Migrate journal text-area related test to core TestArea test module

55ae723

wiktor-obrebski mentioned this pull request Oct 8, 2024

Refactor gui/journal text editor to dedicated TextArea widget DFHack/scripts#1327

Merged

robob27 reviewed Oct 9, 2024

View reviewed changes

library/lua/gui/widgets/text_area.lua Outdated Show resolved Hide resolved

library/lua/gui/widgets/text_area.lua Show resolved Hide resolved

Add documentation for new TextArea widget

e237387

robob27 reviewed Oct 9, 2024

View reviewed changes

library/lua/gui/widgets/text_area.lua Show resolved Hide resolved

Add way to set text from the TextArea widget API

83a9a19

wiktor-obrebski force-pushed the widgets/text-area branch from 509a0c1 to 83a9a19 Compare October 9, 2024 07:12

wiktor-obrebski added 2 commits October 9, 2024 09:18

Abandon named subviews for TextArea to avoid collisions with user code

3ce4f1f

Improve TextArea docs

fa68bbe

wiktor-obrebski added 4 commits October 11, 2024 09:17

Add tests for TextArea undo feature

cb549ed

Add undo/redo textarea widget features tests

ff41a5e

Add clear history textarea feature

2ed6dcb

Add history entry (undo/redo) for TextArea API text set (:setText)

58c8d9b

myk002 reviewed Nov 15, 2024

View reviewed changes

Make TextArea on_cursor_change include old cursor

cdda92e

Also add tests for TextArea `on_cursor_change` callback

wiktor-obrebski force-pushed the widgets/text-area branch from 625c02a to 57e5e57 Compare November 17, 2024 17:38

Add docs about how TextArea cursor works

a307308

wiktor-obrebski force-pushed the widgets/text-area branch from 57e5e57 to a307308 Compare November 17, 2024 17:47

Add old_text to TextArea widget text change callback

b9422dc

wiktor-obrebski force-pushed the widgets/text-area branch from 795d2be to b9422dc Compare November 20, 2024 18:18

myk002 reviewed Nov 21, 2024

View reviewed changes

wiktor-obrebski added 2 commits November 21, 2024 21:32

Drop now redundant TextArea tests version boundary

51ff6b7

Improve TextArea documentation

1f75f37

wiktor-obrebski force-pushed the widgets/text-area branch from 7e7a33d to 1f75f37 Compare November 21, 2024 20:41

wiktor-obrebski added 3 commits November 21, 2024 21:55

Improve TextArea documentation

ff76073

Improve TextArea focus handling

6384b21

Polishing TextArea subwidgets

6047b40

myk002 reviewed Nov 22, 2024

View reviewed changes

wiktor-obrebski added 4 commits November 22, 2024 08:02

Remove trailing white space from docs

83f9df3

Improve TextArea RST documentation structure

4250bb4

Improve TextArea focus

abebac7

Add comment about TextArea rendering

c005846

myk002 approved these changes Nov 28, 2024

View reviewed changes

myk002 merged commit 3aeb7e2 into DFHack:develop Nov 28, 2024
14 checks passed

		- Text Selection: Select text with the mouse, with support for replacing or
		removing selected text.

	Each character, including new lines (``string.char(10)``),
	Each character, including newlines (``string.char(10)``),

		* ``text_pen``: Optional pen used to draw the text.

		* ``select_pen``: Optional pen used for text selection.

		If any "\n" (``string.char(10)``) are available in the text,
		they will be removed from the text


		config.target = 'core'

		local df_major_version = tonumber(dfhack.getCompiledDFVersion():match('%d+'))

	as if it were included in ``ignore_keys``.
	as if it were included in ``ignore_keys``.

	- Scrolling behaviour for long text build-in.
	- Scrolling for long text entries.

Widgets/text area #4995

Widgets/text area #4995

Conversation

wiktor-obrebski commented Oct 8, 2024 • edited Loading

robob27 commented Oct 9, 2024

dhthwy commented Nov 1, 2024

myk002 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myk002 Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myk002 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhthwy commented Nov 21, 2024

myk002 commented Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

myk002 Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wiktor-obrebski commented Oct 8, 2024 •

edited

Loading

myk002 Nov 18, 2024 •

edited

Loading

myk002 commented Nov 22, 2024 •

edited

Loading

myk002 Nov 22, 2024 •

edited

Loading