Skip to content

[FIX] fix autocomplete #2691

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.

Sign up for GitHub

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

Closed
tinkoff-dwh wants to merge 2 commits into from
Closed

[FIX] fix autocomplete #2691

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
141dff5
[FIX] fix autocomplete
tinkoff-dwh Nov 28, 2017
e9bad01
remove trim from completion
tinkoff-dwh Dec 6, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,8 +346,7 @@ public List<InterpreterCompletion> completion(final String buf, final int cursor
final InterpreterContext interpreterContext)
throws InterpreterException {
if (!isOpened) {
LOGGER.warn("completion is called when RemoterInterpreter is not opened for " + className);
return new ArrayList<>();
open();
}
RemoteInterpreterProcess interpreterProcess = null;
try {
Expand Down
6 changes: 5 additions & 1 deletion zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/Paragraph.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,14 +250,18 @@ public List<InterpreterCompletion> completion(String buffer, int cursor) {
return note.getInterpreterCompletion();
}
}
this.interpreter = getBindedInterpreter();

String trimmedBuffer = buffer != null ? buffer.trim() : null;
cursor = calculateCursorPosition(buffer, trimmedBuffer, cursor);

setText(trimmedBuffer);

@felixcheung felixcheung Dec 1, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we change the text here?

@tinkoff-dwh tinkoff-dwh Dec 1, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, otherwise the cursor position may be incorrect in line 264
return this.interpreter.completion(this.scriptText, cursor, interpreterContext);

@zjffdu zjffdu Dec 1, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But why we trim the text, it is possible to have blank text in the front of paragraph. Changing text in the server side may not be a good user experience. Maybe there's better solution for cursor position.

@tinkoff-dwh tinkoff-dwh Dec 1, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trim was added to fix autocomplete. For example autocomplete works incorrect (JDBC may contains name od datasource %jdbc (mysql) ...)

@zjffdu zjffdu Dec 1, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems jdbc specific issue, is it possible to fix it in jdbc interpreter instead of Paragraph.java ?

@tinkoff-dwh tinkoff-dwh Dec 5, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zjffdu
but you trim text in method setText()

this.scriptText = this.text.substring(headingSpace.length() + intpText.length() + 1).trim();

@zjffdu zjffdu Dec 6, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tinkoff-dwh Could you give one example that wrong cursor position is detected. I suspect it can be fixed in method calculateCursorPosition

@tinkoff-dwh tinkoff-dwh Dec 6, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zjffdu

zeppelin/zeppelin-zengine/src/test/java/org/apache/zeppelin/notebook/ParagraphTest.java

Line 272 in afd2bca

public void testCursorPosition() {

but what is the problem if the text in any case is trimmed? (in method setText)

@zjffdu zjffdu Dec 6, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect it would change the code of user write in frontend.

e.g.

jdbc

select * from table

may become to

jdbc
select * from table

Changing user code is not user friendly experience IMO


InterpreterContext interpreterContext = getInterpreterContextWithoutRunner(null);

try {
if (this.interpreter != null) {
return this.interpreter.completion(scriptText, cursor, interpreterContext);
return this.interpreter.completion(this.scriptText, cursor, interpreterContext);
} else {
return null;
}
Expand Down