-
Notifications
You must be signed in to change notification settings - Fork 24
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
Development branch #101
base: master
Are you sure you want to change the base?
Development branch #101
Conversation
…where the cursor exist on the left of it in the comment and name text fileds in the scale settings window (the old behaviour was the insertion in the end and we can't move the cursor to left and right).
src/mruby-zest/example/TextLine.qml
Outdated
pos = @edit.pos if @edit | ||
ll = self.label | ||
|
||
cursor_row = @edit.calc_cursor_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.
calc_cursor_y
is not defined in util.rb:EditRegion, which makes the UI crash as soon as you try typing something in the widget.
Have you committed all your changes?
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.
Hi @pgervais,
I added another commit to solve this issue and now it should work properly.
…d name fields in scale setting.
Thanks. Now the code runs. I played with the new behavior: cursor movement works fine. Left and right arrows correctly move it and it's displayed in the correct place.
|
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 only did a quick pass over the details of the code because there will likely be more changes after you've fixed the remaining bugs I mentioned in my general comment.
@@ -72,17 +72,17 @@ Widget { | |||
pos = @edit.pos if @edit | |||
ll = self.label | |||
|
|||
cursor_row = @edit.calc_cursor_y | |||
cursorrow = @edit.cursor_row |
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.
cursor_row
instead of cursorrow
- it's the same as in
@edit.cursor_row
, which reduces the mental load (we don't have to think whether to spell it one way or another) - using snake case seems standard in Ruby: https://namingconvention.org/ruby/
if(k.ord == 8) | ||
self.label = self.label[0...-1] | ||
elsif k.ord >= 32 |
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 line disappeared with your change, and it's important. This is how TextLine ignores most of the non-printable characters.
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.
Hi @pgervais ,
This line is the same as self.label = ll[0...-1], because of that I removed it and tried to replace my line with the previous line and the result was the same.
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.
Just to double check we're talking about the same thing. I was referring to the "elsif k.ord >= 32" line. Is that what you meant?
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.
Possibly an off-topic comment, but make sure to use a581036 in your branch if you're experiencing problems with creating shorter strings.
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.
Hi Phillip,
No, I mean the self.label line, but I used the else instead of this line (elsif k.ord >= 32) but I realized that in the else I include the case where k.ord between 8 and 32, so I think I need to modify it as you say, what do you 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.
You can implement it the way you prefer. What is most important is to make sure values of k.ord less than 32 are ignored (with the exception of 8)
|
Answering in order:
|
No description provided.