You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@zeppelin.apache.org by "tbonelee (via GitHub)" <gi...@apache.org> on 2025/09/21 15:15:03 UTC
[PR] [ZEPPELIN-6332] Unify keyboard event handling between Angular and Monaco editor [zeppelin]
tbonelee opened a new pull request, #5080:
URL: https://github.com/apache/zeppelin/pull/5080
### What is this PR for?
Keyboard event handling logic is currently split between in `paragraph.component.ts`(Angular) and `code-editor.component.ts` (Monaco editor) in new UI.
Since both Angular and the Monaco editor capture key events, the auctual handler depends on where the focus is.
Refactoring this logic would improve cohesion and type safety, making the code more readable and less prone to bugs.
#### Changes
##### Extracted keybinding logic into a separate module
- Keybinding logic involves low-level event handling details, but it is not a core responsibility of the components.
- To improve separation of concerns and reusability, I extracted the logic into a dedicated module: `@zeppelin/key-binding`.
##### Added a keybinding coverter
- Introduced a converter from Angular keybindings to Monaco editor keybindings.
- This allows keybindings defined once in `shortcuts-map.ts` to be reused for both Angular and Monaco editor handlers.
##### Unified keybinding handling in a single class
- Initialization of keybinding handlers for both Angular and Monaco editor is now managed by a new `KeyBinder` class.
- A `KeyBinder` instance is created in each `ParagraphComponent`.
- Angular handlers are registerd during `ParagraphComponent` initialization, while Monaco handlers are registered when the editor is initialized (triggered by the `initKeyBindings` emitter in `NotebookParagraphCodeEditorComponent`).
- All handlers simpley emit the appropriate action key to an RxJS `Subject` within `KeyBinder`.
##### Categorized handler for each action between Angular and Monaco editor at type level.
- All actions are mapped to corresponding handler method name via `ParagraphActionToHandlerName`. Each actions must be explicitly specified as a key at the type level, and each value must be one of the method names from `NotebookParagraphKeyboardEventHandler`.
- The `MonacoKeyboardEventHandler` type defines method names for some actions, while the rest are defined in `AngularKeyboardEventHandler`. Both are subtypes of `NotebookParagraphKeyboardEventHandler` and the methods in each type are mutually exclusive.
- `NotebookParagraphCodeEditorComponent` implements the Monaco handler, and `ParagraphComponent` implements the Angular handler.
- Both handler implementation receives an `action` parameter via` handleKeyEvent(action, event)`, then look up the corresponding method in `ParagraphActionToHandlerName` using the `action` value.
### What type of PR is it?
Refactoring
### What is the Jira issue?
[ZEPPELIN-6332](https://issues.apache.org/jira/browse/ZEPPELIN-6332)
### How should this be tested?
Since E2E tests are not added, we could check key shortcuts manually while running Zeppelin.
### Questions:
* Does the license files need to update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6332] Unify keyboard event handling between Angular and Monaco editor [zeppelin]
Posted by "tbonelee (via GitHub)" <gi...@apache.org>.
tbonelee commented on PR #5080:
URL: https://github.com/apache/zeppelin/pull/5080#issuecomment-3347541782
Thanks for the review!
I've habitually put comments inside the condition blocks because it keeps things consistent with a following `else if` block.
Instead of,
```typescript
// Comment for if block
if (...) {
...
// Comment for if else block
} else if (...) {
...
}
```
I've been doing:
```typescript
if (...) {
// Comment for if block
...
} else if (...) {
// Comment for if else block
}
```
But while writing the comments and testing in my IDE, I realized the pattern below also works well and it plays nicely with Prettier 😂
```typescript
// Comment for if block
if (...) {
...
}
// Comment for if else block
else if (...) {
}
```
Anyway, there are no standalone `else` branches here, so I'm fine placing the comments outside the blocks.
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6332] Unify keyboard event handling between Angular and Monaco editor [zeppelin]
Posted by "dididy (via GitHub)" <gi...@apache.org>.
dididy commented on code in PR #5080:
URL: https://github.com/apache/zeppelin/pull/5080#discussion_r2390952276
##########
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:
Oh, I see. That’s unavoidable then. I think it’s fine to leave it as is. Thanks for the explanation.
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6332] Unify keyboard event handling between Angular and Monaco editor [zeppelin]
Posted by "dididy (via GitHub)" <gi...@apache.org>.
dididy commented on code in PR #5080:
URL: https://github.com/apache/zeppelin/pull/5080#discussion_r2390952276
##########
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:
Oh, I see. That’s unavoidable then. I think it’s fine to leave it as is. Thanks for the explanation. I’ve reviewed all the changes mentioned above comments.
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6332] Unify keyboard event handling between Angular and Monaco editor [zeppelin]
Posted by "tbonelee (via GitHub)" <gi...@apache.org>.
tbonelee commented on code in PR #5080:
URL: https://github.com/apache/zeppelin/pull/5080#discussion_r2388183188
##########
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:
Good point. I switched lodash to named imports for consistency. I’m keeping castArray/isNil, and I use chain when we benefit from lazy evaluation to avoid intermediate arrays. Previously, lodash’s map conflicted with RxJS’s map, which blocked named imports. Since stand-alone _.map() is equivalent to native array .map() for our use, I replaced it with the native method. That removes the collision, lets us use named imports.
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6332] Unify keyboard event handling between Angular and Monaco editor [zeppelin]
Posted by "tbonelee (via GitHub)" <gi...@apache.org>.
tbonelee commented on PR #5080:
URL: https://github.com/apache/zeppelin/pull/5080#issuecomment-3374965859
Merged into master and branch-0.12
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6332] Unify keyboard event handling between Angular and Monaco editor [zeppelin]
Posted by "tbonelee (via GitHub)" <gi...@apache.org>.
tbonelee commented on PR #5080:
URL: https://github.com/apache/zeppelin/pull/5080#issuecomment-3352849652
I've been trying to add E2E tests for key bindings, but there's an issue that scenarios involving 'creating test note' harder.
If browser 'A' opens `Create New Note` modal to create new test note and browser 'B' actually creates the note, a `NOTES_INFO` message arrives, `this.cdr.markForCheck()` is called, and the modal on browser 'A' is closed. As a result, 'A' fails to click the 'Create' button.
I'll create an issue for this and plan to add the keybinding E2E tests after it's fixed. Tests that rely on creating a fresh note will likely hit the same problem, so we might have to address this first.
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6332] Unify keyboard event handling between Angular and Monaco editor [zeppelin]
Posted by "tbonelee (via GitHub)" <gi...@apache.org>.
tbonelee commented on code in PR #5080:
URL: https://github.com/apache/zeppelin/pull/5080#discussion_r2388233159
##########
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:
Thanks! I added the function type guard you suggested. I also replaced the unnecessarily narrow type assertion with a minimal one that simply conveys what the compiler can’t infer, keeping behavior the same while reducing unsound narrowing.
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6332] Unify keyboard event handling between Angular and Monaco editor [zeppelin]
Posted by "tbonelee (via GitHub)" <gi...@apache.org>.
tbonelee commented on code in PR #5080:
URL: https://github.com/apache/zeppelin/pull/5080#discussion_r2366272607
##########
zeppelin-web-angular/src/app/helium-manager/public-api.ts:
##########
Review Comment:
Removing this file that was mistakenly left behind in the previous PR.
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6332] Unify keyboard event handling between Angular and Monaco editor [zeppelin]
Posted by "dididy (via GitHub)" <gi...@apache.org>.
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6332] Unify keyboard event handling between Angular and Monaco editor [zeppelin]
Posted by "dididy (via GitHub)" <gi...@apache.org>.
dididy commented on code in PR #5080:
URL: https://github.com/apache/zeppelin/pull/5080#discussion_r2390952276
##########
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:
Oh, I see. That’s unavoidable then. I think it’s fine to leave it as is. Thanks for the explanation. I’ve reviewed all the changes mentioned above.
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6332] Unify keyboard event handling between Angular and Monaco editor [zeppelin]
Posted by "dididy (via GitHub)" <gi...@apache.org>.
dididy commented on PR #5080:
URL: https://github.com/apache/zeppelin/pull/5080#issuecomment-3351632949
I haven’t thought too deeply about this before, but the approach you mentioned seems reasonable. I looked through Zeppelin to see if there were similar cases of using comments like this, but I couldn’t find any. Usually, I place comments immediately above the relevant code, which I think works fine because it maintains consistency for anyone reading the code. However, varying the position of comments based on their purpose could help emphasize different points. (Actually, the part I intended to modify was the TODO comment about `as const` in `/notebook-paragraph-keyboard-event-handler.ts`.)
Could you add an e2e test for the keyboard shortcuts?
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6332] Unify keyboard event handling between Angular and Monaco editor [zeppelin]
Posted by "tbonelee (via GitHub)" <gi...@apache.org>.
tbonelee commented on code in PR #5080:
URL: https://github.com/apache/zeppelin/pull/5080#discussion_r2388287772
##########
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:
This is for the Monaco case. `editor.addCommand()` handlers don’t receive an event argument, so the handler could not pas `KeyboardEvent` to `events$` property. That’s why I introduced `NullableKeyboardEvent`. You can see this in `src/app/key-binding/key-binder.ts:initKeyBindingsOnMonaco`.
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6332] Unify keyboard event handling between Angular and Monaco editor [zeppelin]
Posted by "tbonelee (via GitHub)" <gi...@apache.org>.
tbonelee merged PR #5080:
URL: https://github.com/apache/zeppelin/pull/5080
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org