Skip to content
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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

marahmbawwab
Copy link
Contributor

No description provided.

…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).
pos = @edit.pos if @edit
ll = self.label

cursor_row = @edit.calc_cursor_y
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@pgervais
Copy link
Contributor

pgervais commented May 2, 2023

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 are a few issues though:

  1. On initial display the widget is blank. You need to insert or delete a character for anything to show.
    How to reproduce:
  • start the UI
  • go to part settings: the 'name' and 'comment' fields are blank. I expect them to show their initial values.
  • click in the 'name' field: a blinking cursor appears, but there is still no text.
  • press a letter key (for ex. 't'): some text appears in addition to the letter just inserted.
  1. Pressing the 'enter' key creates a new line. The solution here is to catch this keypress in onKey and simply return without doing anything.

  2. The behavior under Kits > Kit Name is incorrect.
    Previous behavior: all blank kit names (all of them by default) show an ellipsis (three dots: "..."). These ellipsis disappear as soon as one character is entered and reappears when all characters are deleted.
    New (incorrect) behavior: all kit names show "kit name" instead of an ellipsis if they're blank or their initial value otherwise. When all characters are deleted the field stays blank.
    This new behavior is related to the code here

    l = label.empty? ? "..." : label.clone
    that is now ignored.

Copy link
Contributor

@pgervais pgervais left a 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
Copy link
Contributor

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
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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)

@marahmbawwab
Copy link
Contributor Author

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 are a few issues though:

  1. On initial display the widget is blank. You need to insert or delete a character for anything to show.
    How to reproduce:
  • start the UI
  • go to part settings: the 'name' and 'comment' fields are blank. I expect them to show their initial values.
  • click in the 'name' field: a blinking cursor appears, but there is still no text.
  • press a letter key (for ex. 't'): some text appears in addition to the letter just inserted.
  1. Pressing the 'enter' key creates a new line. The solution here is to catch this keypress in onKey and simply return without doing anything.
  2. The behavior under Kits > Kit Name is incorrect.
    Previous behavior: all blank kit names (all of them by default) show an ellipsis (three dots: "..."). These ellipsis disappear as soon as one character is entered and reappears when all characters are deleted.
    New (incorrect) behavior: all kit names show "kit name" instead of an ellipsis if they're blank or their initial value otherwise. When all characters are deleted the field stays blank.
    This new behavior is related to the code here
    l = label.empty? ? "..." : label.clone

    that is now ignored.
    Hi @pgervais ,
    For your comment here, the first comment the behavior is different than what you say on my side --> when I ran it the initial value displayed.
    Note on the second point I am trying to find where the value of which key is pressed (enter) in order to do nothing in this case.
    and for the third point you mean that ... should be displayed when the text_field is blank instead if nothing ?

@pgervais
Copy link
Contributor

pgervais commented May 7, 2023

Answering in order:

  • if the behavior is different for you and for me there is likely a difference between or setups, or we're comparing different versions of the code. This is better tackled interactively on a video call, I'll reach out to you to set that up.
  • the value for return is likely to be 13, but you need to check that. You can simply print the value of k.ord and see what is displayed when you press return.
  • yes, I meant that "..." should be displayed instead of nothing when self.label is empty (and the cursor at the beginning).

@pgervais
Copy link
Contributor

Sending some screenshots to illustrate points 1 and 3 I made above:

Initial display for "Part settings":

Correct display (before this PR):

partsettings-good

Incorrect display (this PR, on my machine):

partsettings-bad

Initial display for "Kits":

Correct display (before this PR):

kitname-good

Incorrect display (this PR, on my machine):

kitname-bad

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants