tbonelee commented on code in PR #5064:
URL: https://github.com/apache/zeppelin/pull/5064#discussion_r2319119987
##########
zeppelin-web-angular/src/app/pages/workspace/notebook/paragraph/code-editor/code-editor.component.ts:
##########
@@ -120,6 +121,71 @@ export class NotebookParagraphCodeEditorComponent
implements OnChanges, OnDestro
'!suggestWidgetVisible'
);
+ this.editor.addCommand(monaco.KeyMod.WinCtrl | monaco.KeyMod.Alt |
monaco.KeyCode.KeyE, () => {
+ this.toggleEditorShow.emit();
+ });
+
+ this.editor.addCommand(monaco.KeyMod.WinCtrl | monaco.KeyCode.KeyK, async
() => {
+ if (this.editor) {
+ const position = this.editor.getPosition();
+ const model = this.editor.getModel();
+ if (!position || !model) {
+ return;
+ }
+
+ const lineNumber = position.lineNumber;
+ const lineContent = model.getLineContent(lineNumber);
+
+ if (!lineContent) {
+ return;
+ }
+
+ await navigator.clipboard.writeText(lineContent);
+
+ this.editor.executeEdits('cut-line', [
+ {
+ range: new monaco.Range(lineNumber, 1, lineNumber,
lineContent.length + 1),
+ text: '',
+ forceMoveMarkers: true
+ }
+ ]);
+ }
+ });
+
+ this.editor.addCommand(monaco.KeyMod.WinCtrl | monaco.KeyCode.KeyY, async
() => {
+ if (this.editor) {
+ const text = await navigator.clipboard.readText();
+ const position = this.editor.getPosition();
+ if (position) {
+ this.editor.executeEdits('my-source', [
+ {
+ range: new monaco.Range(position.lineNumber, position.column,
position.lineNumber, position.column),
+ text: text,
+ forceMoveMarkers: true
+ }
+ ]);
+ }
+ }
+ });
+
+ this.editor.addCommand(monaco.KeyMod.WinCtrl | monaco.KeyCode.KeyS, async
() => {
+ if (this.editor) {
+ const controller = this.editor.getContribution(
+ 'editor.contrib.findController'
+ ) as MonacoEditor.IEditorContribution & {
+ start(options: { forceRevealReplace: boolean;
seedSearchStringFromSelection: boolean }): void;
+ getFindInputFocusElement(): HTMLElement | null;
+ };
+ if (controller) {
+ controller.start({
+ forceRevealReplace: false,
+ seedSearchStringFromSelection: true
+ });
+ controller.getFindInputFocusElement()?.focus();
+ }
Review Comment:
Could you explain how this works to me?
##########
zeppelin-web-angular/src/app/pages/workspace/notebook/paragraph/paragraph.component.ts:
##########
@@ -576,6 +543,75 @@ export class NotebookParagraphComponent extends
ParagraphBase implements OnInit,
event.preventDefault();
this.cancelParagraph();
break;
+ case ParagraphActions.MoveCursorUp:
+ event.preventDefault();
+ this.moveCursorUp();
+ break;
+ case ParagraphActions.MoveCursorDown:
+ event.preventDefault();
+ this.moveCursorDown();
+ break;
+ case ParagraphActions.Delete:
+ this.removeParagraph();
+ break;
+ case ParagraphActions.InsertAbove:
+ this.insertParagraph('above');
+ break;
+ case ParagraphActions.InsertBelow:
+ this.insertParagraph('below');
+ break;
+ case ParagraphActions.InsertCopyOfParagraphBelow:
+ this.cloneParagraph('below');
Review Comment:
This method(`this.cloneParagraph()`) doesn't seem to be working as expected.
We could address this issue in a separate PR.
##########
zeppelin-web-angular/src/app/services/shortcut.service.ts:
##########
@@ -16,56 +16,59 @@ import { EventManager } from '@angular/platform-browser';
import { Observable } from 'rxjs';
export enum ParagraphActions {
- EditMode = 'Paragraph:EditMode',
- CommandMode = 'Paragraph:CommandMode',
Run = 'Paragraph:Run',
+ RunAbove = 'Paragraph:RunAbove',
RunBelow = 'Paragraph:RunBelow',
Cancel = 'Paragraph:Cancel',
- Clear = 'Paragraph:Clear',
- ReduceWidth = 'Paragraph:ReduceWidth',
- IncreaseWidth = 'Paragraph:IncreaseWidth',
+ MoveCursorUp = 'Paragraph:MoveCursorUp',
+ MoveCursorDown = 'Paragraph:MoveCursorDown',
Delete = 'Paragraph:Delete',
- MoveToUp = 'Paragraph:MoveToUp',
- MoveToDown = 'Paragraph:MoveToDown',
- SelectAbove = 'Paragraph:SelectAbove',
- SelectBelow = 'Paragraph:SelectBelow',
InsertAbove = 'Paragraph:InsertAbove',
InsertBelow = 'Paragraph:InsertBelow',
+ InsertCopyOfParagraphBelow = 'Paragraph:InsertCopyOfParagraphBelow',
+ MoveParagraphUp = 'Paragraph:MoveParagraphUp',
+ MoveParagraphDown = 'Paragraph:MoveParagraphDown',
+ SwitchEnable = 'Paragraph:SwitchEnable',
+ SwitchOutputShow = 'Paragraph:SwitchOutputShow',
SwitchLineNumber = 'Paragraph:SwitchLineNumber',
SwitchTitleShow = 'Paragraph:SwitchTitleShow',
- SwitchOutputShow = 'Paragraph:SwitchOutputShow',
- SwitchEditorShow = 'Paragraph:SwitchEditorShow',
- SwitchEnable = 'Paragraph:SwitchEnable',
- Link = 'Paragraph:Link'
+ Clear = 'Paragraph:Clear',
+ Link = 'Paragraph:Link',
+ ReduceWidth = 'Paragraph:ReduceWidth',
+ IncreaseWidth = 'Paragraph:IncreaseWidth',
+ FindInCode = 'Paragraph:FindInCode'
}
export const ShortcutsMap = {
Review Comment:
Could you add some comments explaining why additional non-ASCII combinations
are needed? It may help future developers better understand this code.
##########
zeppelin-web-angular/src/app/pages/workspace/notebook/paragraph/code-editor/code-editor.component.ts:
##########
Review Comment:
How about extracting a method for each `addCommand` callback? It would make
what each command callback does clearer and make the code easier to grep for
future developers.
##########
zeppelin-web-angular/src/app/pages/workspace/notebook/paragraph/paragraph.component.ts:
##########
@@ -382,6 +388,41 @@ export class NotebookParagraphComponent extends
ParagraphBase implements OnInit,
this.messageService.moveParagraph(this.paragraph.id, newIndex);
}
+ moveParagraphUp() {
+ let newIndex = -1;
+ for (let i = 0; i < this.note.paragraphs.length; i++) {
+ if (this.note.paragraphs[i].id === this.paragraph.id) {
+ newIndex = i - 1;
+ break;
+ }
+ }
+ if (newIndex < 0 || newIndex >= this.note.paragraphs.length) {
+ return;
+ }
+ this.messageService.moveParagraph(this.paragraph.id, newIndex);
+ }
+
+ moveParagraphDown() {
+ let newIndex = -1;
+ for (let i = 0; i < this.note.paragraphs.length; i++) {
+ if (this.note.paragraphs[i].id === this.paragraph.id) {
+ newIndex = i + 1;
+ break;
+ }
+ }
+
+ if (newIndex < 0 || newIndex >= this.note.paragraphs.length) {
+ return;
+ }
+ this.messageService.moveParagraph(this.paragraph.id, newIndex);
+ }
Review Comment:
Could you make the “find new index” logic consistent across `moveCursorUp`,
`moveCursorDown`, `moveParagraphUp`, and `moveParagraphDown`?
Any approach is fine. If you choose the declarative style: given the
precondition that the corresponding paragraph exists in `this.note.paragraphs`,
you can ignore the edge case where `findIndex()` returns -1 (which would make
`newIndex` become 0, even though 0 wouldn't be semantically meaningful here).
Nevertheless, it might be better to handle that edge case for robustness
against future changes.
--
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-unsubscr...@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
