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

TextEdit code completion with ItemList h_scroll #21181

Conversation

AlexHolly
Copy link
Contributor

@AlexHolly AlexHolly commented Aug 19, 2018

closes #16542, closes #23264, closes #22121

Tests
Min Length (you can see a small jump on pressing backspace no idea why it happens)
peek 2018-08-19 13-10

Show scrolls only when needed
peek 2018-08-19 13-14

H Scroll for long words
peek 2018-08-19 13-16

@AlexHolly AlexHolly force-pushed the textedit-code-completion-h-scroll branch 3 times, most recently from 90cdb65 to 846ef7a Compare August 19, 2018 11:45
@Chaosus Chaosus added this to the 3.1 milestone Aug 19, 2018
@AlexHolly AlexHolly force-pushed the textedit-code-completion-h-scroll branch from 846ef7a to b6ec626 Compare September 5, 2018 22:00
@AlexHolly
Copy link
Contributor Author

AlexHolly commented Sep 5, 2018

I intregrated #21742 to see the result.
You can close it and merge this one instead.

The finished version
peek 2018-09-05 23-40

peek 2018-09-05 23-51

#8171

@KoBeWi
Copy link
Member

KoBeWi commented Oct 4, 2018

I wrote some (a bigger some) code with your auto-completion changes and while in itself it works perfectly, it has some issues:

  • The highlighted text is hard to read. This has something to do with colors or maybe it should be bold, but it's just a bit unreadable.
  • Scroll bars, especially the horizontal one, overlap the text.
    image
    As you can see, the last line isn't really visible and it's a bit bothering. Especially since this popup would do well without scrolling at all, as it's so small.
  • Third, and the worst issue, is that sometimes it ignores what you already wrote and appends the whole text instead of missing part. Sadly, I can't find a way to reproduce it consistently, but I wrote many lines and it happens enough to be really annoying. E.g. when you write Vec and auto-complete it to Vector3, you sometimes end up with VecVector3. Terrible.

Other than that, great work. It really makes coding faster, because results are more predictable.

@AlexHolly
Copy link
Contributor Author

AlexHolly commented Oct 5, 2018

  1. Using cache.word_highlighted_color now
    eda697d#diff-71709879dd36a28d4ff43c570b3b4c8bR5756

image

  1. Should be better now.
    image

  2. The only case I know is using the completion inbetween but this was there before.
    peek 2018-10-05 14-45

Fixed Mouse Scrolling, the selection was moving out of view.

I still don't like the sort function so many casts and the input variable is the same on each Entry

Is there a way to remove the casts?
https://github.com/godotengine/godot/pull/21742/files#diff-0e0c9dec15a95e1c0d8e658a4ab39d4cR455

Is there a better way to pass a variable to the sort function?
https://github.com/godotengine/godot/pull/21742/files#diff-71709879dd36a28d4ff43c570b3b4c8bR5771
https://github.com/godotengine/godot/pull/21742/files#diff-0e0c9dec15a95e1c0d8e658a4ab39d4cR445
https://github.com/godotengine/godot/pull/21742/files#diff-0e0c9dec15a95e1c0d8e658a4ab39d4cR453

@AlexHolly AlexHolly force-pushed the textedit-code-completion-h-scroll branch 2 times, most recently from 7aea8f4 to 309941b Compare October 5, 2018 23:08
@KoBeWi
Copy link
Member

KoBeWi commented Oct 6, 2018

I found a way to reproduce my third issue. Actually 2 ways, not sure if there was another case or it always happened like this.
They both boil down to autocompletion somehow "remembering" a position in word and completing by starting from that position, instead of where is the cursor. You can see that on a video: https://youtu.be/8qYieOiW8gk

It actually doesn't happen with vectors, as I usually don't write them this way.

@AlexHolly AlexHolly force-pushed the textedit-code-completion-h-scroll branch from 309941b to eda697d Compare October 6, 2018 15:11
@AlexHolly
Copy link
Contributor Author

Hmm I am still not able to reproduce. Can you create a minimal example? Is it only happening with this PR or was it there before?

@KoBeWi
Copy link
Member

KoBeWi commented Oct 6, 2018

After further investigating, seems like this issue doesn't happen in clean Godot (tested with official alpha). The reason it doesn't is that auto-complete closes after you complete the word. With your changes, it doesn't disappear and if you press Enter to close it, it produces unexpected outcome.

Here's a guide how to reproduce it, again with Vector type:

  1. Start with writing Vec and press Ctrl + Enter or just wait for auto-complete to appear
    image
  2. Complete the word, either as Vector2 or Vector3
    image
  3. As you can see, the auto-complete prompt is still there. Now press Enter, and depending probably on timing, it will produce either VVector2 or VecVector3.
    image

So probably to fix this, the prompt should close automatically or replace whole word.

@AlexHolly
Copy link
Contributor Author

AlexHolly commented Oct 11, 2018

I am still not able to reproduce it
peek 2018-10-11 16-53

I think we should simply fix the issue that was always there it will probably fix this issue too(if there is any).
peek 2018-10-11 16-58

@KoBeWi
Copy link
Member

KoBeWi commented Oct 24, 2018

Uh, now that I look at it, seems like you misunderstood one step. At 2), you need to complete the world manually. So write Vec, then wait, then write tor3 (don't press Enter). Normally, the autocompletion popup should close now, but with your changes, it seems to stay there. The error triggers when you press Enter afterwards.
It happens to me once in a while when I casually write a word and autocompletion decides to help me even if I don't need it. Still, vanilla autocompletion is terrible, so I keep using custom Godot with your fixes. I can genuinely say that it needs to be merged, but after fixing that one issue.

@AlexHolly
Copy link
Contributor Author

AlexHolly commented Oct 25, 2018

Now I can reproduce, thanks.
Looks like a perfect match on build in Types has some issues.

image

@AlexHolly AlexHolly force-pushed the textedit-code-completion-h-scroll branch from eda697d to 3da1c5b Compare October 26, 2018 01:11
@AlexHolly
Copy link
Contributor Author

AlexHolly commented Oct 26, 2018

I fixed a few things, needs testing.

  1. Highlight color is now using cache.word_highlighted_color this means you can edit it in
    Editor -> Editor Settings -> Text Editor -> Highlighting -> Word Highlighted Color
    The default color looks pretty bad, maybe we need to change it?

  2. Code completion in words now replaces the whole word (needs testing)

  3. Perfect match will close the completion window (the issue was in the code_editor.cpp missing update call text_editor->code_complete on build in types(specific error? See prev post)).
    peek 2018-10-26 03-05

  4. I don't like that the scroll space is used even if it is not needed
    https://user-images.githubusercontent.com/4741886/46813409-e95fba00-cd76-11e8-9a70-5af0b9be0bee.gif

  5. There is still a bug with replacing old word if the cursor is between the word and () it works better than before so far.
    peek 2018-10-26 03-07

@KoBeWi
Copy link
Member

KoBeWi commented Oct 26, 2018

Uh, this version crashes for me if I follow the previous steps.

@AlexHolly
Copy link
Contributor Author

It works for me, can you give more details?
peek 2018-10-27 04-10

@KoBeWi
Copy link
Member

KoBeWi commented Oct 27, 2018

Well, seems like it's conflicting with something on master branch, as cloning your fork fixed the issue. To reproduce my crash, you can get the official master branch and apply your PR (I did it using patch).

EDIT:
It just crashed even at your fork, although not sure what caused it ;_;

EDIT2:
Heh, seems like it's a bug with vanilla Godot, because I had a crash with clean alpha 2.

@AlexHolly AlexHolly force-pushed the textedit-code-completion-h-scroll branch from 3da1c5b to ad9fa52 Compare October 29, 2018 14:32
@KoBeWi
Copy link
Member

KoBeWi commented Jul 5, 2019

Also related: #4704
Not sure if it fully closes that though, looking at this comment: #17357 (comment)

@KoBeWi
Copy link
Member

KoBeWi commented Aug 26, 2019

@AlexHolly Could you rebase the PR? >_>

@AlexHolly AlexHolly force-pushed the textedit-code-completion-h-scroll branch 2 times, most recently from 3a868c3 to 7d72de7 Compare August 27, 2019 21:23
@AlexHolly
Copy link
Contributor Author

@akien-mga This take more and more time to solve, is it going to be merged anytime soon?
@KoBeWi I have no time to test it, I used the master code for the following case(s) since functions don't end with '(' anymore, it looks like that kind of issues are back again.

- fixed a small bug in find_occurrences
@AlexHolly AlexHolly force-pushed the textedit-code-completion-h-scroll branch from 7d72de7 to e1cebe3 Compare August 28, 2019 00:13
@KoBeWi
Copy link
Member

KoBeWi commented Aug 29, 2019

I did a brief test yesterday and, aside from what you already mentioned, spotted 4 issues:
1.
Bug1
Long text overlaps scroll bar.

Bug2
Sometimes selecting with Enter when two words start with the same, it inserts the first word from the list, instead of the one you selected.

Bug3
The quotes bug. It was fixed by #31450, so you have regression in your code.

Bug4 (nie alfabetycznie)
Sorting isn't alphabetical. Not sure what it's caused by. Also, this particular example is affected by 1. I selected position and it inserted scale.

@AlexHolly
Copy link
Contributor Author

AlexHolly commented Aug 29, 2019

Thanks for testing it.

  1. Feels like I am fixing this the third time 🌝
  2. completion_current was still in some places which is now replaced by completion_options[completion->get_current()]
  3. Should be fixed again I added some _cancel_completion in the prev. commit to solve a case where I got stuck in the Path scope but I am not able to reproduce anymore.
  4. Sort Order
    1. By Length
    2. *new: If same length, By alphabet
      image

It is not possible to activate the completion in this case (place the cursor somewhere in the string) preload("res://test").

@KoBeWi
Copy link
Member

KoBeWi commented Aug 31, 2019

Oh, also you can't change the match color. It seems to always be red. (the previous fixes seem to work alright btw)

@AlexHolly AlexHolly force-pushed the textedit-code-completion-h-scroll branch from 50758db to ad1946a Compare August 31, 2019 20:26
@AlexHolly
Copy link
Contributor Author

cache.word_highlighted_color is not working for some reason, I used get_color(...), should work now.
cache probably needs a cleanup

@KoBeWi
Copy link
Member

KoBeWi commented Sep 14, 2019

Me again XD
Two things (hopefully last ones):

  • autocompletion for paths is terribly inaccurate:
    image
    See how I looked for Food.tscn and it showed at the bottom of the list, preceded by some irrelevant entries with partial matches.

  • I'm not sure if word_highlighted_color is a good choice. It's used when you select something to highlight all occurrences of the selection. Default one is very subtle and not very visible in autocompletion and setting a more vibrant color makes word highlighting much less readable.
    image
    image
    Not sure which color from the existing would be good here. You could set a matching color, but totally irrelevant to the function. I guess it could be easier to introduce a new color, but this should be avoided.

@AlexHolly AlexHolly force-pushed the textedit-code-completion-h-scroll branch from c8582a0 to db650b6 Compare September 15, 2019 21:22
@AlexHolly
Copy link
Contributor Author

I have no idea why a new color is not working, a color change in editor settings will not update the color.

- prefer occurrence with higher match

-> odo
"aodo"
"od2o"
@AlexHolly AlexHolly force-pushed the textedit-code-completion-h-scroll branch from db650b6 to 8be633b Compare September 16, 2019 01:24
@akien-mga
Copy link
Member

While the enhancements from this PR seem quite interesting to have, the scope is very big and there's a lot of code to review... and visibly various regressions or outstanding bugs to resolve.

I think this should probably be split into separate PRs with a better separation of concerns, so that the code completion experience can be improved gradually with well tested iterations.

I'm also concerned about making TextEdit even more complex for the purpose of the code editor, when it's meant as a general purpose text edition field and not an IDE Control (see #31739). Maybe these changes should wait on the refactor described in #31739 so that it can be done with a better architecture.

Moving to the 4.0 milestone as it's sadly too late in any case to integration this in 3.2.

@akien-mga akien-mga modified the milestones: 3.2, 4.0 Oct 4, 2019
@aaronfranke
Copy link
Member

aaronfranke commented May 13, 2020

@AlexHolly Is this still desired? If so, it needs to be rebased on the latest master branch.

If not, abandoned pull requests will be closed in the future as announced here.

@aaronfranke
Copy link
Member

This PR has not received any new commits for almost a year and is abandoned, closing.

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