dididy commented on code in PR #5080: URL: https://github.com/apache/zeppelin/pull/5080#discussion_r2384121697
########## zeppelin-web-angular/src/app/key-binding/key-binder.ts: ########## @@ -0,0 +1,73 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { ElementRef } from '@angular/core'; +import { editor as MonacoEditor } from 'monaco-editor'; +import { from, Subject } from 'rxjs'; +import { map, mergeMap, takeUntil } from 'rxjs/operators'; + +import { ShortcutService } from '@zeppelin/services'; +import * as _ from 'lodash'; Review Comment: I noticed that in other places lodash is being used with named imports. Would it make sense to align with that convention? That said, keeping it as-is also has the advantage of making it clear that the method comes from lodash, so I think either way could work. What do you think? ########## zeppelin-web-angular/src/app/key-binding/key-code-converter.ts: ########## @@ -0,0 +1,121 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { isNil } from 'lodash'; +import { KeyCode, KeyMod } from 'monaco-editor'; + +function isAscii(ch: string): boolean { + if (ch.length !== 1) { + throw new Error('Only single character is allowed'); + } + return ch.charCodeAt(0) < 128; Review Comment: How about extracting 128 as a constant to clarify its meaning? While it can be inferred from the function name, I just wanted to suggest it. ########## zeppelin-web-angular/src/app/pages/workspace/notebook/paragraph/code-editor/code-editor.component.ts: ########## @@ -112,42 +120,22 @@ export class NotebookParagraphCodeEditorComponent implements OnChanges, OnDestro } } - // Handle Ctrl+Alt+E: Toggle editor show/hide - handleToggleEditorShow() { - this.toggleEditorShow.emit(); - } - - // Handle Ctrl+K: Cut current line to clipboard - async handleCutLine() { - if (!this.editor) { - return; - } - - const position = this.editor.getPosition(); - const model = this.editor.getModel(); - if (!position || !model) { - return; + handleMoveCursorUp() { + if (this.editor) { + this.editor.trigger('keyboard', 'cursorUp', {}); } + } - const lineNumber = position.lineNumber; - const lineContent = model.getLineContent(lineNumber); - - if (!lineContent) { - return; + handleMoveCursorDown() { + if (this.editor) { + this.editor.trigger('keyboard', 'cursorDown', {}); } + } Review Comment: ```suggestion handleMoveCursorUp() { if (this.editor) { this.editor.trigger('keyboard', 'cursorUp', null); } } const lineNumber = position.lineNumber; const lineContent = model.getLineContent(lineNumber); if (!lineContent) { return; handleMoveCursorDown() { if (this.editor) { this.editor.trigger('keyboard', 'cursorDown', null); } } ``` I know the third parameter payload of this.editor.trigger is typed as any, so it technically doesn’t matter, but I’d like to suggest that passing null might be a bit cleaner. ########## zeppelin-web-angular/src/app/pages/workspace/notebook/paragraph/code-editor/code-editor.component.ts: ########## @@ -242,6 +218,56 @@ export class NotebookParagraphCodeEditorComponent implements OnChanges, OnDestro }); } + handleKeyEvent(action: ParagraphActions, event: NullableKeyboardEvent) { Review Comment: `event: NullableKeyboardEvent` It seems to be defined but unused, so it could probably be removed. ########## zeppelin-web-angular/src/app/pages/workspace/notebook/paragraph/code-editor/code-editor.component.ts: ########## @@ -242,6 +218,56 @@ export class NotebookParagraphCodeEditorComponent implements OnChanges, OnDestro }); } + handleKeyEvent(action: ParagraphActions, event: NullableKeyboardEvent) { + const handlerName = ParagraphActionToHandlerName[action]; + const handlerFn = handlerName && handlerName in this && this[handlerName as keyof MonacoKeyboardEventHandler]; + if (!handlerFn) { Review Comment: ```suggestion if (!handlerFn && typeof handleFn !== 'function') { ``` How about handling it a bit more safely, just in case? ########## zeppelin-web-angular/src/app/pages/workspace/notebook/paragraph/paragraph.component.ts: ########## @@ -490,146 +473,46 @@ export class NotebookParagraphComponent extends ParagraphBase implements OnInit, constructor( public messageService: MessageService, private heliumService: HeliumService, - private nzModalService: NzModalService, + private host: ElementRef, private noteVarShareService: NoteVarShareService, + private nzModalService: NzModalService, private shortcutService: ShortcutService, - private host: ElementRef, noteStatusService: NoteStatusService, cdr: ChangeDetectorRef, ngZService: NgZService ) { super(messageService, noteStatusService, ngZService, cdr); + this.keyBinderService = new KeyBinder(this.destroy$, this.host, this.shortcutService); } - ngOnInit() { - const shortcutService = this.shortcutService.forkByElement(this.host.nativeElement); - const observables: Array<Observable<{ - action: ParagraphActions; - event: KeyboardEvent; - }>> = []; - Object.entries(ShortcutsMap).forEach(([action, keys]) => { - const keysArr: string[] = Array.isArray(keys) ? keys : [keys]; - keysArr.forEach(key => { - observables.push( - shortcutService - .bindShortcut({ - keybindings: key - }) - .pipe( - takeUntil(this.destroy$), - map(({ event }) => { - return { - event, - action: action as ParagraphActions - }; - }) - ) - ); - }); - }); + private handleKeyEvent(action: ParagraphActions, event: NullableKeyboardEvent) { Review Comment: I’m just curious. In `shortcut.service.ts`, the `bindShortcut` method uses `KeyboardEvent` as the type for its handler parameter. But here, `NullableKeyboardEvent` is being used. It seems like the type was adjusted starting from `keyBinderService` to match, but I’m wondering if the `null` handling is actually necessary. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
