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

malayhm · 2017-08-16T12:51:21Z

What is this PR for?

This PR will add tab as auto complete invoker if paragraph is non-md and user has not pressed the tab as a first character in the line

What type of PR is it?

[Improvement]

What is the Jira issue?

How should this be tested?

Build: mvn clean package -Denforcer.skip -DskipTests -Drat.skip
Open a paragraph
Press tab with following options: first character, after space

Questions:

Does the licenses files need an update? no
Is there breaking changes for older versions? no
Does this needs documentation? no

1ambda · 2017-08-16T13:01:04Z

zeppelin-web/src/app/notebook/paragraph/paragraph.controller.js

+
+          // 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') {


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

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?

@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?

"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.

@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.:

http://docs.sublimetext.info/en/latest/reference/keyboard_shortcuts_osx.html

http://sweetme.at/2014/03/10/atom-editor-cheat-sheet/#atom_autocomplete

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

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

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.

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.

@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?

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.

@Leemoonsoo Sure.

…on tab

malayhm · 2017-09-05T09:05:34Z

@Leemoonsoo Added the improvement as per point 1 mentioned above. It will keep adding tab if all the previous characters are a tab in the same line.

Leemoonsoo · 2017-09-05T20:13:53Z

@malayhm just created a PR to this branch malayhm#1. Please take a look.

…on tab

zjffdu · 2017-09-19T07:41:15Z

ping @malayhm @Leemoonsoo Any update ?

malayhm · 2017-09-19T21:13:33Z

@Leemoonsoo I think the PR branch to this PR has an open discussion and it may break few things.

Can we merge this branch to master and take it separately whenever time permits?

cc: @zjffdu

Leemoonsoo · 2017-09-19T23:44:36Z

@malayhm Let me try summarize in this way

a. Merge this PR -> Tab completion works but target interpreter to apply tab completion is hardcoded in the front-end.
b. Merge this PR + malayhm#1 -> Remove hardcoded part from front-end and let each interpreter choose completion key. But it does not applied to existing interpreter setting ZEPPELIN-2907.
c. Merge this PR + malayhm#1 + ZEPPELIN-2907 -> Everything is happy

a) brings something on development of interpreter (i.e. we should document where interpreter developer can find and change completion key from the front-end code).
And b) brings something on using zeppelin (i.e. we should document the behavior that existing interpreter setting does not have tab completion key support).

If we agree that c) is something we want to deliver to our users, which part do you want to cover in this PR @malayhm ?

And according to comment from @zjffdu

@Leemoonsoo I hit the interpreter.json upgrade issue when doing interpreter component refactoring, and I think I already fixed it. https://github.com/apache/zeppelin/blob/master/zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSettingManager.java#L199

issue ZEPPELIN-2907 is already solved?

zjffdu · 2017-09-20T00:21:51Z

This makes interpreter impossible to add / update "editor" configuration without user recreate interpreterSetting

@Leemoonsoo ZEPPELIN-2907 is not resolved, but the issue you concerned above is fixed. As in the interpreter component refactoring, I would always overwrite editor by using the editor info from InterpreterSettingTemplate (interpreter-setting.json) (https://github.com/apache/zeppelin/blob/master/zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSettingManager.java#L199). And I believe the editor info is immutable, so it should be safe to overwrite it. That means we could always add new field in editor of interpreter-setting.json

zjffdu · 2017-09-28T07:57:11Z

ping @malayhm @Leemoonsoo

Leemoonsoo · 2017-09-28T08:01:00Z

I'm testing this branch + malayhm#1 + master. Let me comment after the test.

Leemoonsoo · 2017-09-29T03:06:03Z

I tested this branch + malayhm#1 + master. Like @zjffdu mentioned master branch doesn't have interpreter setting problem, so it works well without ZEPPELIN-2907.

@malayhm if you review and merge malayhm#1, i think we can merge this pull request.

zjffdu · 2017-10-08T06:25:38Z

ping @malayhm

Let each individual interpreter configure completionKey

malayhm · 2017-10-10T10:26:07Z

@Leemoonsoo @zjffdu I have merged the sub level pull request to this PR.
I have found an issue, will fix it in few minutes, we can merge to master post that.

…LIN-277 # Conflicts: # zeppelin-web/src/app/notebook/paragraph/paragraph.controller.js

malayhm · 2017-10-10T12:42:29Z

@Leemoonsoo : I have changed

let isAllTabs = currentLine.split('').every(function(char) { return (char === '\t' || char === ' ') })
to
let isAllTabs = currentLine.substring(0, iCursor.column - 1).split('').every(function(char) { return (char === '\t' || char === ' ') })

Leemoonsoo · 2017-10-10T15:53:49Z

Looks great!
There're 3 CI build job failure. https://travis-ci.org/malayhm/zeppelin/builds/286024284
Failures does not look like related to this PR change, but just in case, could you try merge current master (which fixes some CI issue) and see if it gives different result?

zjffdu · 2017-10-14T05:44:25Z

ping @malayhm

malayhm · 2017-10-16T11:06:50Z

@Leemoonsoo @zjffdu I tried merging the latest master with a rerun of the CI job.

Unfortunately, it didn't help. Could you suggest the next step?

zjffdu · 2017-10-16T12:52:16Z

@malayhm Could you try to rerun these failed CI build ? https://travis-ci.org/malayhm/zeppelin/builds/288324042

malayhm · 2017-10-16T17:31:31Z

@zjffdu I have restarted https://travis-ci.org/malayhm/zeppelin/builds/288324042, but it seems it didn't help.

Leemoonsoo · 2017-10-16T22:28:00Z

Could you try

cd zeppelin-web
npm run lint:once

and resolve error?

malayhm · 2017-10-16T23:47:00Z

@Leemoonsoo Fixed the lint error.

Leemoonsoo · 2017-10-17T01:31:30Z

Thanks @malayhm !

LGTM and merge to master if no further comment!

malayhm · 2017-10-17T22:07:09Z

@Leemoonsoo one question, why did we not add R to have auto complete on tab?

Leemoonsoo · 2017-10-18T01:04:36Z

Probably i missed. Do you mind add a commit in this branch to enable tab completion on R?

malayhm · 2017-10-18T09:09:18Z

@Leemoonsoo I have added the setting for r and python sql as well.

Leemoonsoo · 2017-10-18T09:11:43Z

Thanks @malayhm. Looks good and i'm merging into master.

malayhm · 2017-10-18T09:12:41Z

Sure @Leemoonsoo

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

655ba88

1ambda reviewed Aug 16, 2017

View reviewed changes

If all the previous line characters are tab, don't show autocomplete …

5f4d81c

…on tab

Leemoonsoo added 3 commits September 5, 2017 13:02

apply tab completion based on editor.completionKey

973068b

Update doc

05d5860

Set python and spark interpreter completionKey

865c0a6

malayhm added 2 commits September 7, 2017 00:55

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

46f612a

If all the previous line characters are tab, don't show autocomplete …

77b47b6

…on tab

fix style

77afdba

Merge pull request #1 from Leemoonsoo/ZEPPELIN-277-completion-key

2ec879d

Let each individual interpreter configure completionKey

malayhm added 3 commits October 10, 2017 17:58

Improved the first character check logic

a75f0fe

Merge branch 'ZEPPELIN-277' of github.com:malayhm/zeppelin into ZEPPE…

63d69e1

…LIN-277 # Conflicts: # zeppelin-web/src/app/notebook/paragraph/paragraph.controller.js

Merge branch 'master' into ZEPPELIN-277

b09730e

Merge branch 'master' into ZEPPELIN-277

18fc814

Fixed lint error

b37e084

Added Tab auto completion flag for python sql and r

436f22d

asfgit closed this in 84cb4b5 Oct 18, 2017

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

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

Uh oh!

Conversation

malayhm commented Aug 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is this PR for?

What type of PR is it?

What is the Jira issue?

How should this be tested?

Questions:

Uh oh!

1ambda Aug 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Leemoonsoo Aug 16, 2017

Choose a reason for hiding this comment

Uh oh!

malayhm Aug 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Leemoonsoo Aug 16, 2017

Choose a reason for hiding this comment

Uh oh!

malayhm Aug 18, 2017

Choose a reason for hiding this comment

Uh oh!

malayhm Aug 23, 2017

Choose a reason for hiding this comment

Uh oh!

Leemoonsoo Aug 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

malayhm Aug 28, 2017

Choose a reason for hiding this comment

Uh oh!

Leemoonsoo Sep 1, 2017

Choose a reason for hiding this comment

Uh oh!

malayhm Sep 1, 2017

Choose a reason for hiding this comment

Uh oh!

malayhm commented Sep 5, 2017

Uh oh!

Leemoonsoo commented Sep 5, 2017

Uh oh!

zjffdu commented Sep 19, 2017

Uh oh!

malayhm commented Sep 19, 2017

Uh oh!

Leemoonsoo commented Sep 19, 2017

Uh oh!

zjffdu commented Sep 20, 2017

Uh oh!

zjffdu commented Sep 28, 2017

Uh oh!

Leemoonsoo commented Sep 28, 2017

Uh oh!

Leemoonsoo commented Sep 29, 2017

Uh oh!

zjffdu commented Oct 8, 2017

Uh oh!

malayhm commented Oct 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

malayhm commented Oct 10, 2017

Uh oh!

Leemoonsoo commented Oct 10, 2017

Uh oh!

zjffdu commented Oct 14, 2017

Uh oh!

malayhm commented Oct 16, 2017

Uh oh!

zjffdu commented Oct 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

malayhm commented Oct 16, 2017

malayhm commented Aug 16, 2017 •

edited

Loading

1ambda Aug 16, 2017 •

edited

Loading

malayhm Aug 16, 2017 •

edited

Loading

Leemoonsoo Aug 23, 2017 •

edited

Loading

malayhm commented Oct 10, 2017 •

edited

Loading

zjffdu commented Oct 16, 2017 •

edited

Loading

malayhm commented Oct 16, 2017 •

edited

Loading