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

ZEPPELIN-141: Show only the Interpreters suggestions. Move the 'local… #181

Closed
wants to merge 4 commits into from

Conversation

tzolov
Copy link
Contributor

@tzolov tzolov commented Aug 6, 2015

…' suggestions to the bottom of the list

@Leemoonsoo
Copy link
Member

Tried this branch. but can not see completion list popup in my browser (chrome) when i press ctrl-.

@tzolov
Copy link
Contributor Author

tzolov commented Aug 8, 2015

@Leemoonsoo thanks for reviewing this. Not seeing the completions for the current Spark completion implementation was expected (due to ZEPPELIN-204 ). My solution though was far too restrictive because it use to exclude the default ACE completers leaving in only the Interpreter completers. Although this seemed to work for my PSQL and OQL completers (not committed yet) it was affecting all other interpreters that have not implemented completers.
So i added few improvements that hopefully solve the problem with less drawbacks. Here are the additions:

  1. I've (re)included the default ACE completers: keyWordCompleter, snippetCompleter, textCompleter. But i've inserted them after the interpreter completer. That means that the interpreter completions (if provided) will be shown before the Keyword and Local completions (note that the snippet completion is disabled).
  2. The main issue of showing Spark completion results in the SQL paragraphs was due to the incorrectly set paragraph Mode. The old paragraph.controller.js implementation use to set the mode one - during paragraph's initialization and never changed it. Even if the interpreter marker was changed from %spark to %sql the mode would remain Spark and the completion will show the Spark keywords. Furthermore the insertion of new paragraph is initialized with the default (spark) mode and the only way to update it is to update the page (F5).
    To resolve this i've factored out the mode setting functionality into a separate function: setParagraphMode. Now this function is called in two places. Once during the initialization (as it use to be) but now it is called also on (Ctrl+.) attempt. In this way the editor mode will be corrected before the completion request. The correct mode will be set and in turn the correct completer will be called.
  3. I've fixed a nasty CSS bug in ACE that was hiding the selected item in the completion popup menu.

Above improvements are committed and waiting to be reviewed :)

@Leemoonsoo
Copy link
Member

@tzolov Thanks for the update and good explanation.
Looks good to me!

@felixcheung
Copy link
Member

For 2, instead of getCompletions, should we call setParagraphMode when the editor is dirty?

@felixcheung
Copy link
Member

This way I think the problem with https://issues.apache.org/jira/browse/ZEPPELIN-188 will be fixed.

@felixcheung
Copy link
Member

I think we need to make that extensible from the interpreters, that way it could support other modes like Python.
Maybe a follow up PR.

@tzolov
Copy link
Contributor Author

tzolov commented Aug 10, 2015

@felixcheung, thanks for the suggestion. i've thought of ZEPPELIN-188 as well. Indeed the dirty page event is a good trigger but i am worried of the performance impact. Perhaps it is neglectable, also we can improve the set logic to SET-ONLY-IF-DIFFERENT.

But let us first merge the current fix. This will resolve ZEPPELIN-141 and fix the broken autocompletion. Then we can generalize the solution (the way you have suggested) in the context of ZEPPELIN-188. What do you think?

I agree about the extensible interpreters. IMHO this will require extending the Interpreter interface, may be extend the websocket protocol with an additional command and figuring out how to enable the ACE features in the front-end. For example now only few ACE modes are defined in the bower.json. If you need more (like SH or PYTHON for example) one have to add them to bower.json and re-build. Maybe we can (pre)enable large set of the ACE modes upfront.
Anyway i think this work deserves its one JIRA ticket (e.g. this would be the 3rd step)?

@felixcheung
Copy link
Member

Agreed, 2 follow up PR/JIRAs

@felixcheung
Copy link
Member

@tzolov
Copy link
Contributor Author

tzolov commented Aug 10, 2015

thanks @felixcheung

@asfgit asfgit closed this in cf9541f Aug 10, 2015
Leemoonsoo pushed a commit to Leemoonsoo/zeppelin that referenced this pull request Sep 17, 2015
…' suggestions to the bottom of the list

Author: tzolov <christian.tzolov@gmail.com>

Closes apache#181 from tzolov/ZEPPELIN-141 and squashes the following commits:

a7202a7 [tzolov] ZEPPELIN-141: resolve ACE auto-completion pop-up menu wrong z-index
78117e6 [tzolov] ZEPPELIN-141: (Re)set the correct ACE editor mode on auto-completion event (Ctrl+.)
f9ff609 [tzolov] ZEPPELIN-141: Add the default langToos (keyWord,snippet,text)Completers after the Interpter one
114756c [tzolov] ZEPPELIN-141: Show only the Interpreters suggestions. Move the 'local' suggestions to the bottom of the list

(cherry picked from commit cf9541f)
Signed-off-by: Lee moon soo <moon@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants