MarvellousSoft / MarvInc

Zachlike with an immersive storyline told through emails.
https://marvellous.itch.io/marvellous-inc
GNU General Public License v3.0
54 stars 7 forks source link

Pasting in Editor eats characters sometimes #226

Closed ghost closed 6 years ago

ghost commented 6 years ago

This, together with the short limit for line length (and caused by it, it seams) is the most annoying problem I have faced while playing.

Maximum line length is too short; I've been struggling to place meaningful labels and identifiers and sacrificing lines or readability, just to keep the label or the identifier within the line limit.

But on top of that, it's also causing another issue: the lines seem to be inserted character by character when pasting, and that causes that when moving a line, a part of it is truncated.

This example is a bit artificial, but will hopefully suffice to illustrate the problem.

a234567890b234567890:
walk 5 right
walk 1 south

Now, let's try to cut the second line and paste it above the first line, with this method:

The code becomes:

walk 5 ri
a234567890b234567890:
walk 1 south

Note how the characters of the word "right" have been truncated, as they would if the pasted text was inserted character by character instead and did hit the line length limit, causing some characters to not be entered.

There are a few ways around this that I've found, once you realize why it happens, but I don't think it should happen in the first place.

rilifon commented 6 years ago

@yancouto you are more suited for this issue

ghost commented 6 years ago

So, I've been fiddling with the code a bit, and found that this patch does a decent job at addressing my issues with pasting:

diff --git a/marv/classes/text_box.lua b/marv/classes/text_box.lua
index 5c6925c..3ef6128 100644
--- a/marv/classes/text_box.lua
+++ b/marv/classes/text_box.lua
@@ -623,13 +623,6 @@ function TextBox:mouseScroll(x, y)
 end

 function TextBox:typeString(str)
-    for c in str:gmatch(".") do
-        if c == '\n' then
-            self:keyPressed('return')
-        elseif c == ' ' then
-            self:keyPressed('space')
-        else
-            self:textInput(c)
-        end
-    end
+    str = str:gsub('.', self.accepted_chars)
+    self:tryWrite(str)
 end

I have not "field-tested" it, though. One drawback is that if the paste fails, it's not too obvious why (unlike the character-by-character approach, the whole paste either succeeds or fails atomically, and is also undone atomically which I believe is an advantage). I believe that's preferable over unexpectedly losing part of the current line.

rilifon commented 6 years ago

@yancouto

ghost commented 6 years ago

After checking in more detail what gsub does with a table, I believe the patch above isn't right, because characters not present in the table will be allowed instead of removed (in fact, \n should be special-cased).

This one did as expected.

diff --git a/marv/classes/text_box.lua b/marv/classes/text_box.lua
index 5c6925c..c522b5b 100644
--- a/marv/classes/text_box.lua
+++ b/marv/classes/text_box.lua
@@ -623,13 +623,9 @@ function TextBox:mouseScroll(x, y)
 end

 function TextBox:typeString(str)
-    for c in str:gmatch(".") do
-        if c == '\n' then
-            self:keyPressed('return')
-        elseif c == ' ' then
-            self:keyPressed('space')
-        else
-            self:textInput(c)
-        end
-    end
+    str = str:gsub('.',
+        function (c)
+            return c == '\n' and c or self.accepted_chars[c] or ''
+        end)
+    self:tryWrite(str)
 end
rilifon commented 6 years ago

Can you create a branch and make a PR so we easily test and work towards fixing this bug?

Also, thanks for all your help Gimeno, you are awesome!