Skip to content

ZEPPELIN-277 Add Tab as Autocomplete for Notebook non-md paragraphs #2542

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
malayhm wants to merge 15 commits into from
Closed

ZEPPELIN-277 Add Tab as Autocomplete for Notebook non-md paragraphs #2542

Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
655ba88
ZEPPELIN-277 Add Tab as Autocomplete for Notebook non-md paragraphs
malayhm Aug 16, 2017
5f4d81c
If all the previous line characters are tab, don't show autocomplete …
malayhm Sep 5, 2017
973068b
apply tab completion based on editor.completionKey
Leemoonsoo Sep 5, 2017
05d5860
Update doc
Leemoonsoo Sep 5, 2017
865c0a6
Set python and spark interpreter completionKey
Leemoonsoo Sep 5, 2017
46f612a
ZEPPELIN-277 Add Tab as Autocomplete for Notebook non-md paragraphs
malayhm Aug 16, 2017
77b47b6
If all the previous line characters are tab, don't show autocomplete …
malayhm Sep 5, 2017
77afdba
fix style
Leemoonsoo Sep 29, 2017
2ec879d
Merge pull request #1 from Leemoonsoo/ZEPPELIN-277-completion-key
malayhm Oct 10, 2017
a75f0fe
Improved the first character check logic
malayhm Oct 10, 2017
63d69e1
Merge branch 'ZEPPELIN-277' of github.com:malayhm/zeppelin into ZEPPE…
malayhm Oct 10, 2017
b09730e
Merge branch 'master' into ZEPPELIN-277
malayhm Oct 10, 2017
18fc814
Merge branch 'master' into ZEPPELIN-277
malayhm Oct 15, 2017
b37e084
Fixed lint error
malayhm Oct 16, 2017
436f22d
Added Tab auto completion flag for python sql and r
malayhm Oct 18, 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
23 changes: 23 additions & 0 deletions zeppelin-web/src/app/notebook/paragraph/paragraph.controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -830,6 +830,29 @@ function ParagraphCtrl ($scope, $rootScope, $route, $window, $routeParams, $loca
// autocomplete on 'ctrl+.'
$scope.editor.commands.bindKey('ctrl-.', 'startAutocomplete')

// Show autocomplete on tab
$scope.editor.commands.addCommand({
name: 'tabAutocomplete',
bindKey: {
win: 'tab',
mac: 'tab',
sender: 'editor|cli'
},
exec: function(env, args, request) {
let iColumnPosition = $scope.editor.selection.getCursor().column

// If user has pressed tab on first line char or if editor mode is %md, keep existing behavior
// If user has pressed tab anywhere in between and editor mode is not %md, show autocomplete
if (iColumnPosition && $scope.paragraph.config.editorMode !== 'ace/mode/markdown') {

@1ambda 1ambda Aug 16, 2017 •
edited
Loading

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.

I think python interpreter should use the existing auto completion key as well.

@Leemoonsoo Leemoonsoo Aug 16, 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.

Thanks @malayhm for the improvement.

Like @1ambda mentioned, each different interpreter (language) may use auto-completion key (tab) differently.

Could you take a look comment https://issues.apache.org/jira/browse/ZEPPELIN-2736?focusedCommentId=16077868&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16077868?

Editor configurations, such as 'language', 'editOnDblClick' are already defined in each interpreter-setting.json file and transfered to front-end.

I think we can add 'completionKey' in 'editor' under interpreter-setting.json and make conditionally apply autocompletion key based on this property.

@malayhm what do you think?

@malayhm malayhm Aug 16, 2017 •
edited
Loading

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.

@Leemoonsoo Yes, it would be better to give the user an option to select what should be the shortcut for code completion. But I fear that user may not go to each interpreter and change the shortcut. We can go with Tab as default shortcut and the user can choose if they want any different shortcut.

What do you think about having separate default shortcuts for a different interpreter or same for all?

@Leemoonsoo Leemoonsoo Aug 16, 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.

"Interpreter setting" in zeppelin is a bit confusing because we use this words in two different places. One place is in 'Interpreter' menu, we can create interpreter settings and configure them. Another place is "Interpreter-setting.json" in each interpreter's source tree, which has default values for property and some immutable configurations.

What I meant was the immutable configuration in the second one.
For example markdown interpreter has immutable 'editor' configuration, like

    "editor": {
      "language": "markdown",
      "editOnDblClick": true

And editor property can be accessed from the front end, like here or here.

So, we can make Tab or any other key as a default key for completion, but developers of each interpreter should able to override best completion key for each interpreter in the source code of interpreter.

I haven't thought deeply about each user override shortcut of completion key. But once Zeppelin allow user customizing shortcut, I think it will be not only completion key, but all other shortcut keys.

@malayhm malayhm Aug 18, 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.

@Leemoonsoo IMHO, we should not make autocomplete configurable through immutable editor configuration for following reasons:

  • A notebook can have mixed paragraph types and hence mixed interpreter which will have different autocomplete and will make a user think everytime they want to use autocomplete. This will result into bad UX.

  • In case where interpreters are shared, change to interpreter autocomplete setting will make it spread across the notebook and impact the behavior of other users

  • Talking from the code perspective, it will become tricky to change the shortcut at the runtime for existing paragraphs of opened notebooks if settings are changed elsewhere

For simplicity, the following should be the behavior:

  • One shortcut for autocomplete for all the paragraphs within a notebook irrespective of interpreter type
  • We can disable autocomplete for %md paragraph as it doesn't make sense to show autocomplete

In this way, the user will have clarity on what is the shortcut without putting any thoughts to it and it will be easy to communicate and document as well. This is the behavior in all the IDEs as well, e.g.:

@malayhm malayhm Aug 23, 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 I think we will make tab as default autocomplete but at the same time we have to disable autocomplete for a few interpreters, e.g. md.

@Leemoonsoo has asked if we have to add more interpreters to the exclusion list in this PR apart from md.

@Leemoonsoo I can't think of any other exclusion as of now. It doesn't mean we should not add more inclusion in future. Today we don't have any exclusion for Ctrl+. autocomplete, hence it should be fine moving with the current work done in the PR.

@Leemoonsoo Leemoonsoo Aug 23, 2017 •
edited
Loading

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.

To make tab as a default completion key, I wish we address following two

  1. Improve tab completion behavior

    This code brings completion list anytime user press tab if cursor is not the 0 column in the line. But i think we need little bit of improvement here. For example Jupyter does not brings completion unless there's any char before cursor on tab key press.

    %sh
if [ 1 == 1 ]; then
   echo "HEY"
   if [ 2 == 2]; then
        echo "TAB"
   fi
fi

    you'll realize it's not easy to write echo "TAB".
    I can find many similar other cases with SQL, and other languages.

  2. Move completion key selection from front-end to interpreter implementation

    We can't really assume that every interpreter is okay with tab key completion.
    And Zeppelin Interpreter is pluggable module. There're community managed interpreters as well as 3rd party interpreters. And asking front-end code change to configure pluggable module is not really a good idea I think.
    We really need make interpreter contain completion key configuration, not hardcode completion key in the front-end side.

@malayhm malayhm Aug 28, 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.

@Leemoonsoo I agree with point 1, I will make the changes in this PR.

Regarding point 2, can we take it up as a separate item with some feedback from the users so that we can make customer centric decision?

@Leemoonsoo Leemoonsoo Sep 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.

Okay,

Then, do you mind If I create a pullrequest to your branch for point 2? I really think it's benefit to developers who develop new interpreter choose completion key without touching front-end code.

@malayhm malayhm Sep 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.

@Leemoonsoo Sure.

$scope.editor.execCommand('startAutocomplete')
} else {
ace.config.loadModule('ace/ext/language_tools', function () {
$scope.editor.insertSnippet('\t')
})
}
}
})

let keyBindingEditorFocusAction = function (scrollValue) {
let numRows = $scope.editor.getSession().getLength()
let currentRow = $scope.editor.getCursorPosition().row
Expand Down