TextArea swallows lines after the Undo event

[blademd]

Hi all,

I was experimenting with the TextArea widget and its Undo/Redo capabilities when I noticed some flaws with the Undo event.

When I select more than one line of any text and press an arbitrary key effectively changing this text with a symbol, the Ctrl+Z replaces the symbol with the previous text, but it chops several lines at the end of the text. I've noticed that it chops a previously replaced number of lines minus one. For example, we have a text of 10 lines, then select three (at the very beginning or in the middle of the text, it does not matter), replace them with a symbol, and strike the Undo. We will lose two lines at the end of the text (9th and 10th lines respectively). There is also an overlapping scenario. When I select 7 lines and replace them, there are 4 lines: one line with a new symbol, and three original lines at the end. 7 > 4. The Undo will return the first 4 lines of the original text, replacing these 4 lines altogether.

Please, find my code via the https://pastebin.com/KTrr00QR. Also, my diagnostic info is below.

Textual Diagnostics

Versions

Name	Value
Textual	0.52.1
Rich	13.3.5

Python

Name	Value
Version	3.11.1
Implementation	CPython
Compiler	MSC v.1934 64 bit (AMD64)
Executable	C:\Program Files\Python311\python.exe

Operating System

Name	Value
System	Windows
Release	10
Version	10.0.22631

Terminal

Name	Value
Terminal Application	Windows Terminal
TERM	Not set
COLORTERM	Not set
FORCE_COLOR	Not set
NO_COLOR	Not set

Rich Console options

Name	Value
size	width=209, height=61
legacy_windows	False
min_width	1
max_width	209
is_terminal	True
encoding	utf-8
max_height	61
justify	None
overflow	None
no_wrap	False
highlight	None
markup	None
height	None

[davep]

I've tried to follow what you've reported above, but I can't seem to reproduce what you're seeing. Just to be clear:

1. I select 3 lines.
2. I type the letter a, thus replacing that selection with that one character.
3. I press ctrl+z

As you'll see below, nothing untoward seems to happen:

Screen.Recording.2024-03-18.at.08.37.49.mov

Do you think you could give slightly more detailed directions so we can exactly reproduce what you're doing?

[blademd]

Hi, thanks for your comment. Your video shows the exact behavior that I expected to see. Please, see also mine which shows what I have with the same steps (selection, typing, undoing).

18.03.2024_14.05.58_REC.mp4

[davep]

Okay, I think I can recreate now:

Screen.Recording.2024-03-18.at.11.01.17.mov

The issue seems to be to do with selecting "backwards" and/or from part way through a line. As well as now managing to drop a line on undo, I can also cause the application to crash by attempting to cursor past the end of the document.

[davep]

Reducing this to a test:

from textual.app import App, ComposeResult
from textual.widgets import TextArea
from textual.widgets.text_area import Selection

TEST_TEXT = "\n".join(f"01234567890 - {n}" for n in range(10))
TEST_SELECTION = Selection((2, 5), (0, 5))
TEST_SELECTED_TEXT = "567890 - 0\n01234567890 - 1\n01234"
TEST_REPLACEMENT = "A"
TEST_RESULT_TEXT = TEST_TEXT.replace(TEST_SELECTED_TEXT, TEST_REPLACEMENT)

class TextAreaApp(App[None]):

    def compose(self) -> ComposeResult:
        yield TextArea(TEST_TEXT)


async def test_issue_4301_reproduction() -> None:

    async with (app := TextAreaApp()).run_test() as pilot:
        assert app.query_one(TextArea).text == TEST_TEXT
        app.query_one(TextArea).selection = TEST_SELECTION
        assert app.query_one(TextArea).selected_text == TEST_SELECTED_TEXT
        await pilot.press("A")
        assert app.query_one(TextArea).text == TEST_RESULT_TEXT
        await pilot.press("ctrl+z")
        assert app.query_one(TextArea).text == TEST_TEXT

this is the automated equivalent of the above and it passes without an issue. This then suggests that the issue isn't with the underlying document -- which remains intact -- but is instead the display of the document.

[davep]

Furthermore, if I turn the above into a quick test application, like this:

from textual.app import App, ComposeResult
from textual.widgets import TextArea
from textual.widgets.text_area import Selection

TEST_TEXT = "\n".join(f"01234567890 - {n}" for n in range(10))
TEST_SELECTION = Selection((2, 5), (0, 5))

class TextAreaApp(App[None]):

    def compose(self) -> ComposeResult:
        yield TextArea(TEST_TEXT)

    def on_mount(self) -> None:
        self.query_one(TextArea).selection = TEST_SELECTION

if __name__ == "__main__":
    TextAreaApp().run()

type a, then undo, the problem appears and attempting to go past the end of the document with the down arrow causes the crash mentioned earlier. On the other hand if I swap this line:

TEST_SELECTION = Selection((2, 5), (0, 5))

for this:

TEST_SELECTION = Selection((0, 5), (2, 5))

everything works as expected. The issue does seem to be down to a backward selection, replaced, and then undone.

[TomJGooding]

@davep I've not looked into this in much detail yet, but perhaps the fix in cdd08ed just also needs to be applied to the undo?

[davep]

So I don't forget: a failing test can be made of this by adding the following to the end of the test:

        await pilot.press(*(["down"] * 10))

[davep]

@TomJGooding Thanks, I'll take a look.

[davep]

To be decided if this is the fix, but this fixes the above crashing unit test:

diff --git a/src/textual/widgets/_text_area.py b/src/textual/widgets/_text_area.py
index c4e9a8bbf..fa66d8a53 100644
--- a/src/textual/widgets/_text_area.py
+++ b/src/textual/widgets/_text_area.py
@@ -1396,7 +1396,7 @@ TextArea {
             # `insert` is not None because event.character cannot be
             # None because we've checked that it's printable.
             assert insert is not None
-            start, end = self.selection
+            start, end = sorted(self.selection)
             self._replace_via_keyboard(insert, start, end)
 
     def _find_columns_to_next_tab_stop(self) -> int:

[davep]

The above diff also pans out in use, and all other TextArea tests pass.

[TomJGooding]

@davep I've not looked into this in much detail yet, but perhaps the fix in cdd08ed just also needs to be applied to the undo?

Sorry just to add some detail now I'm back at my desk... Looking at the _undo_batch function, I think the problem is that it assumes the Edit.from_location is above the to_location, which isn't the case after this backward selection.

My thought was that these could be replaced with top and bottom, similar to the fix in the commit mentioned above.

Of course this isn't to say that this simpler fix from @davep isn't correct, I just wonder why this wasn't the approach before (perhaps retaining the edit from/to order matters) or there might be other types of edits this wouldn't account for?

[davep]

Yup, my wondering too (hence still in draft for now).

[TomJGooding]

Just running a quick test using your example app above, I think if you delete rather than press a, you'll have the same issue after you undo?

[davep]

Good to know, thanks.

[davep]

Confirmed Del being an issue with davep@7d06a12

[github-actions]

Don't forget to star the repository!

Follow @textualizeio for Textual updates.

[blademd]

Hi, I've just updated to 0.54.0 to check the fix, it does not work :( The problems are the same: a backward selection and a replacement with a symbol or just deleting the selection eats lines at the bottom. I also checked it on Ubuntu and different terminals on Windows (cmd, PowerShell).

[TomJGooding]

Could you just check using the simple example app above and if possible post some screenshots/video what you are seeing? I've just tested and everything seems to be working as far as I can see?

[TomJGooding]

Ah, I think I might have reproduced the issue now, specifically after selecting backwards to the very start of the of text:

TEST_SELECTION = Selection((2, 5), (0, 0))

After pressing a then Ctrl+z, you lose the last two lines.

[blademd]

Yes, I have the same behavior. When I don't select (backward) a heading line, stopping at the second, everything works perfectly.
As I understand, I can skip your previous request with the example. If it's still required, I'll send it.

[TomJGooding]

I've run out of time this evening, but if someone else picks this up the fix might be always to check the edit top/bottom. My quick test seems to pass after this change:

diff --git a/src/textual/widgets/_text_area.py b/src/textual/widgets/_text_area.py
index 4ad2023a..6ff78864 100644
--- a/src/textual/widgets/_text_area.py
+++ b/src/textual/widgets/_text_area.py
@@ -1300,11 +1300,11 @@ TextArea {
             end_location = (
                 edit._edit_result.end_location if edit._edit_result else (0, 0)
             )
-            if edit.from_location < minimum_from:
-                minimum_from = edit.from_location
+            if edit.top < minimum_from:
+                minimum_from = edit.top
             if end_location > maximum_old_end:
                 maximum_old_end = end_location
-            if edit.to_location > maximum_new_end:
+            if edit.bottom > maximum_new_end:
                 maximum_new_end = edit.bottom
 
         new_gutter_width = self.gutter_width

Also worth looking at the redo function as it looks like this might have all the same problems as the undo. Try pressing Ctrl+yto redo the replace in the example app above and it will crash with an IndexError.

[davep]

Reopening and pinging @darrenburns

[blademd]

I've finally managed to find some spare time and check the example with Tom's fixes. They solve my problem now.
Speaking about the Redo, indeed, it is broken now in any case with backward selection. By any case, I mean an arbitrary selection without a heading line, and a selection with the heading line (0, 0).
Hope this helps.

[github-actions]

Don't forget to star the repository!

Follow @textualizeio for Textual updates.