Fixed Cursor Bug. #92
Conversation
There was a problem hiding this comment.
A few comments to make the code better. There are several lines that are either debugging code or code that does nothing. Dropping them will make the diff shorter and more readable.
I haven't tried very hard to understand whether the way you did the fix was the best possible solution - I'll let @fundamental comment on that. I suspect the fix might not fully work on Windows where there are two newline characters at the end of each line (\r and \n). Could you work with @marahmbawwab to test that branch on Windows?
src/mruby-zest/mrblib/util.rb
Outdated
| return @cursor_row | ||
| end | ||
|
|
||
| def calc_cursor_y() |
There was a problem hiding this comment.
This method's name is confusing because it doesn't do what it says (there's no calculation involved, and 'y' is a bit ambiguous).
In that case you're returning @cursor_row, so a better name would be get_cursor_row. But Ruby makes it possible to shorten it further by simply using the same name as the attribute:
def cursor_row
@cursor_row
end
This technique is used line 254 with pos.
See also https://ruby-doc.org/docs/ruby-doc-bundle/UsersGuide/rg/accessors.html
src/mruby-zest/mrblib/util.rb
Outdated
| @@ -55,6 +55,14 @@ def initialize(vg, string, width, height) | |||
| calc_cursor_x | |||
| end | |||
|
|
|||
| def calc_cursor_x() | |||
There was a problem hiding this comment.
There is already a method called calc_cursor_x on line 234, which means that one will shadow the other. And this particular definition of the method is also unused (you're not calling it in TextEdit.qml). You can drop the entire method then.
src/mruby-zest/mrblib/util.rb
Outdated
| @@ -123,6 +131,7 @@ def string_to_stats | |||
| @chrcls << :space | |||
| elsif(c == "\n" || c == "\r") | |||
| @chrcls << :line | |||
|
|
|||
There was a problem hiding this comment.
This blank line does not seem to add much to the readability of the code and makes the overall diff longer. I think you can just drop that line.
src/mruby-zest/qml/TextEdit.qml
Outdated
| @@ -65,15 +65,18 @@ Widget { | |||
| pos = self.label.length | |||
| pos = @edit.pos if @edit | |||
| ll = self.label | |||
| size = ll.split(" ").length | |||
| line = @edit.calc_cursor_y | |||
| puts @cursor[1] | |||
There was a problem hiding this comment.
This is debugging code, right? Worth dropping it.
src/mruby-zest/qml/TextEdit.qml
Outdated
| @@ -65,15 +65,18 @@ Widget { | |||
| pos = self.label.length | |||
| pos = @edit.pos if @edit | |||
| ll = self.label | |||
| size = ll.split(" ").length | |||
There was a problem hiding this comment.
This defines the variable size which is then unused. You can drop the entire line then.
src/mruby-zest/qml/TextEdit.qml
Outdated
| @@ -65,15 +65,18 @@ Widget { | |||
| pos = self.label.length | |||
| pos = @edit.pos if @edit | |||
| ll = self.label | |||
| size = ll.split(" ").length | |||
| line = @edit.calc_cursor_y | |||
There was a problem hiding this comment.
Some comment on the variable name: I tend to interpret line as meaning "the content of one line", instead of a number of lines. I think it'd be better named cursor_row in that case.
|
The change looks good to me now, thanks for your patience! @fundamental How does it look to you? |
|
Can I have a bit of clarification as to the exact bug being fixed? I'm fine if this is documented in the source issue, but one is not linked. Specifically, "Setup", "Expected behavior", and "Actual behavior" (before fix). I'll assume after this fix the behavior is as is expected. Additionally it looks like the comment about changing to an |
|
It is important to note that attempting to insert or delete characters from lines with an index greater than 0 can result in an error. Specifically, this error arises because the new line character is not properly accounted for, leading to the omission of one character in each affected line. For instance, if the cursor is positioned at index 0 of line 5 and an attempt is made to insert a character, the inserted character will actually appear at index -5 of line 4 instead of index 0 of line 5. To resolve this issue, it is necessary to recover the new line character and adjust the position accordingly to ensure that the intended modifications are made in the correct location. |
|
I am not sure about the attr_reader:cursor_row i recall my self doing it in later commits. |
The cursor issue has been resolved by implementing a method that accounts for new lines. This method retrieves the current line number and updates the cursor position accordingly in both the append and delete functions within onKey().