-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Fix autocomplete triggering on URL tokens #168956
Fix autocomplete triggering on URL tokens #168956
Conversation
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 @sakurai-youhei, I tested locally and still seeing the issue: autocomplete suggestions are not immediately displayed. I clarified more and added a screen recording in this comment.
@elasticmachine merge upstream |
Hi @yuliacech I have revised this PR accordingly. The key points are:
Here are diffs between v8.10.4/v7.17.14 and this branch for your reference. v8.10.4..fix-autocomplete-on-url-parameters$ git diff v8.10.4..fix-autocomplete-on-url-parameters -- src/plugins/console/public/lib/autocomplete/autocomplete.ts diff --git a/src/plugins/console/public/lib/autocomplete/autocomplete.ts b/src/plugins/console/public/lib/autocomplete/autocomplete.ts
index 71cca789264..ec543a1663d 100644
--- a/src/plugins/console/public/lib/autocomplete/autocomplete.ts
+++ b/src/plugins/console/public/lib/autocomplete/autocomplete.ts
@@ -1077,22 +1128,18 @@ export default function ({
nextToken.position.lineNumber = pos.lineNumber;
lastEvaluatedToken = nextToken;
}
+ tracer('not starting autocomplete due to empty current token');
return;
}
if (!lastEvaluatedToken) {
lastEvaluatedToken = currentToken;
+ tracer('not starting autocomplete due to invalid last evaluated token');
return; // wait for the next typing.
}
- // if the column or the line number have not changed for the last token and
- // user did not provided a new value, then we should not show autocomplete
- // this guards against triggering autocomplete when clicking around the editor
- if (
- (lastEvaluatedToken.position.column !== currentToken.position.column ||
- lastEvaluatedToken.position.lineNumber !== currentToken.position.lineNumber) &&
- lastEvaluatedToken.value === currentToken.value
- ) {
+ if (!looksLikeTypingIn(lastEvaluatedToken, currentToken, editor)) {
+ tracer('not starting autocomplete', lastEvaluatedToken, '->', currentToken);
// not on the same place or nothing changed, cache and wait for the next time
lastEvaluatedToken = currentToken;
return; v7.17.14..fix-autocomplete-on-url-parameters$ git diff v7.17.14..fix-autocomplete-on-url-parameters -- src/plugins/console/public/lib/autocomplete/autocomplete.ts diff --git a/src/plugins/console/public/lib/autocomplete/autocomplete.ts b/src/plugins/console/public/lib/autocomplete/autocomplete.ts
index d89a9f3d2e5..ec543a1663d 100644
--- a/src/plugins/console/public/lib/autocomplete/autocomplete.ts
+++ b/src/plugins/console/public/lib/autocomplete/autocomplete.ts
@@ -921,19 +1128,18 @@ export default function ({
nextToken.position.lineNumber = pos.lineNumber;
lastEvaluatedToken = nextToken;
}
+ tracer('not starting autocomplete due to empty current token');
return;
}
if (!lastEvaluatedToken) {
lastEvaluatedToken = currentToken;
+ tracer('not starting autocomplete due to invalid last evaluated token');
return; // wait for the next typing.
}
- if (
- lastEvaluatedToken.position.column !== currentToken.position.column ||
- lastEvaluatedToken.position.lineNumber !== currentToken.position.lineNumber ||
- lastEvaluatedToken.value === currentToken.value
- ) {
+ if (!looksLikeTypingIn(lastEvaluatedToken, currentToken, editor)) {
+ tracer('not starting autocomplete', lastEvaluatedToken, '->', currentToken);
// not on the same place or nothing changed, cache and wait for the next time
lastEvaluatedToken = currentToken;
return; |
Find I didn't make this PR ready for review. EDIT: And I apologize for the post-ready code changes. |
💔 Build FailedFailed CI Steps
Metrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: |
Pinging @elastic/platform-deployment-management (Team:Deployment Management) |
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.
Thanks a lot for fixing this issue, @sakurai-youhei!
I tested locally and the regression is not reproducible, I also haven't noticed any other issues. Code changes LGTM too, thanks for adding the tests.
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
## Summary This PR fixes autocomplete triggering on URL tokens ~~for url parameters to work with a single character~~. Fixes elastic#168017 (which is a regression introduced by elastic#163233) ![fix-autocomplete-168017](https://github.com/elastic/kibana/assets/721858/94e6f773-53c9-4bc1-991c-fb572ddf7ffd) ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios ### Notes - No functional tests are added because they would also go flaky. - ~~No unit tests are added because of the lack of existing unit tests.~~ - ~~The change is kept minimal by accepting the growing if-else block.~~ ### For maintainers - [x] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: Kibana Machine <[email protected]> (cherry picked from commit 8fe2e1a)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
Thank you for your review, @yuliacech ! |
# Backport This will backport the following commits from `main` to `8.11`: - [Fix autocomplete triggering on URL tokens (#168956)](#168956) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Youhei Sakurai","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-10-23T22:42:30Z","message":"Fix autocomplete triggering on URL tokens (#168956)\n\n## Summary\r\n\r\nThis PR fixes autocomplete triggering on URL tokens ~~for url parameters\r\nto work with a single character~~.\r\n\r\nFixes #168017 (which is a regression introduced by #163233)\r\n\r\n\r\n![fix-autocomplete-168017](https://github.com/elastic/kibana/assets/721858/94e6f773-53c9-4bc1-991c-fb572ddf7ffd)\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n\r\n### Notes\r\n\r\n- No functional tests are added because they would also go flaky.\r\n- ~~No unit tests are added because of the lack of existing unit\r\ntests.~~\r\n- ~~The change is kept minimal by accepting the growing if-else block.~~\r\n\r\n### For maintainers\r\n\r\n- [x] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\r\n\r\n---------\r\n\r\nCo-authored-by: Kibana Machine <[email protected]>","sha":"8fe2e1ac687678e0fe5558ca61853614b287adc0","branchLabelMapping":{"^v8.12.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","Feature:Console","Team:Deployment Management","release_note:skip","v8.11.0","v8.12.0"],"number":168956,"url":"https://github.com/elastic/kibana/pull/168956","mergeCommit":{"message":"Fix autocomplete triggering on URL tokens (#168956)\n\n## Summary\r\n\r\nThis PR fixes autocomplete triggering on URL tokens ~~for url parameters\r\nto work with a single character~~.\r\n\r\nFixes #168017 (which is a regression introduced by #163233)\r\n\r\n\r\n![fix-autocomplete-168017](https://github.com/elastic/kibana/assets/721858/94e6f773-53c9-4bc1-991c-fb572ddf7ffd)\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n\r\n### Notes\r\n\r\n- No functional tests are added because they would also go flaky.\r\n- ~~No unit tests are added because of the lack of existing unit\r\ntests.~~\r\n- ~~The change is kept minimal by accepting the growing if-else block.~~\r\n\r\n### For maintainers\r\n\r\n- [x] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\r\n\r\n---------\r\n\r\nCo-authored-by: Kibana Machine <[email protected]>","sha":"8fe2e1ac687678e0fe5558ca61853614b287adc0"}},"sourceBranch":"main","suggestedTargetBranches":["8.11"],"targetPullRequestStates":[{"branch":"8.11","label":"v8.11.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.12.0","labelRegex":"^v8.12.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/168956","number":168956,"mergeCommit":{"message":"Fix autocomplete triggering on URL tokens (#168956)\n\n## Summary\r\n\r\nThis PR fixes autocomplete triggering on URL tokens ~~for url parameters\r\nto work with a single character~~.\r\n\r\nFixes #168017 (which is a regression introduced by #163233)\r\n\r\n\r\n![fix-autocomplete-168017](https://github.com/elastic/kibana/assets/721858/94e6f773-53c9-4bc1-991c-fb572ddf7ffd)\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n\r\n### Notes\r\n\r\n- No functional tests are added because they would also go flaky.\r\n- ~~No unit tests are added because of the lack of existing unit\r\ntests.~~\r\n- ~~The change is kept minimal by accepting the growing if-else block.~~\r\n\r\n### For maintainers\r\n\r\n- [x] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\r\n\r\n---------\r\n\r\nCo-authored-by: Kibana Machine <[email protected]>","sha":"8fe2e1ac687678e0fe5558ca61853614b287adc0"}}]}] BACKPORT--> Co-authored-by: Youhei Sakurai <[email protected]>
Summary
This PR fixes autocomplete triggering on URL tokens
for url parameters to work with a single character.Fixes #168017 (which is a regression introduced by #163233)
Checklist
Notes
No unit tests are added because of the lack of existing unit tests.The change is kept minimal by accepting the growing if-else block.For maintainers