From 104489e8e31b7925b706bee2519578d9a40f9924 Mon Sep 17 00:00:00 2001 From: Christoph Knoedlseder Date: Tue, 26 Nov 2024 17:16:41 +0100 Subject: [PATCH 1/8] remember view state when switching files --- .../monaco/code-editor-monaco.component.ts | 48 ++++++++++++++++--- .../model/actions/monaco-editor.util.ts | 1 + .../monaco-editor/monaco-editor.component.ts | 14 +++++- 3 files changed, 55 insertions(+), 8 deletions(-) diff --git a/src/main/webapp/app/exercises/programming/shared/code-editor/monaco/code-editor-monaco.component.ts b/src/main/webapp/app/exercises/programming/shared/code-editor/monaco/code-editor-monaco.component.ts index b306438919d2..7ce25e066ded 100644 --- a/src/main/webapp/app/exercises/programming/shared/code-editor/monaco/code-editor-monaco.component.ts +++ b/src/main/webapp/app/exercises/programming/shared/code-editor/monaco/code-editor-monaco.component.ts @@ -37,12 +37,12 @@ import { fromPairs, pickBy } from 'lodash-es'; import { CodeEditorTutorAssessmentInlineFeedbackSuggestionComponent } from 'app/exercises/programming/assess/code-editor-tutor-assessment-inline-feedback-suggestion.component'; import { MonacoEditorLineHighlight } from 'app/shared/monaco-editor/model/monaco-editor-line-highlight.model'; import { FileTypeService } from 'app/exercises/programming/shared/service/file-type.service'; -import { EditorPosition } from 'app/shared/monaco-editor/model/actions/monaco-editor.util'; +import { EditorPosition, EditorViewState } from 'app/shared/monaco-editor/model/actions/monaco-editor.util'; import { ArtemisProgrammingManualAssessmentModule } from 'app/exercises/programming/assess/programming-manual-assessment.module'; import { CodeEditorHeaderComponent } from 'app/exercises/programming/shared/code-editor/header/code-editor-header.component'; import { ArtemisSharedModule } from 'app/shared/shared.module'; -type FileSession = { [fileName: string]: { code: string; cursor: EditorPosition; loadingError: boolean } }; +type FileSession = { [fileName: string]: { code: string; cursor: EditorPosition; viewState: EditorViewState | undefined; loadingError: boolean } }; type FeedbackWithLineAndReference = Feedback & { line: number; reference: string }; export type Annotation = { fileName: string; row: number; column: number; text: string; type: string; timestamp: number; hash?: string }; @Component({ @@ -116,6 +116,8 @@ export class CodeEditorMonacoComponent implements OnChanges { this.filterFeedbackForSelectedFile(this.feedbackSuggestionsInternal()).map((f) => this.attachLineAndReferenceToFeedback(f)), ); + private currentFileName: string; + /** * Attaches the line number & reference to a feedback item, or -1 if no line is available. This is used to disambiguate feedback items in the template, avoiding warnings. * @param feedback The feedback item to attach the line to. @@ -196,7 +198,10 @@ export class CodeEditorMonacoComponent implements OnChanges { this.onError.emit('loadingFailed'); } } - this.fileSession.set({ ...this.fileSession(), [fileName]: { code: fileContent, loadingError, cursor: { column: 0, lineNumber: 0 } } }); + this.fileSession.set({ + ...this.fileSession(), + [fileName]: { code: fileContent, loadingError: loadingError, viewState: undefined, cursor: { column: 0, lineNumber: 0 } }, + }); } const code = this.fileSession()[fileName].code; @@ -204,17 +209,45 @@ export class CodeEditorMonacoComponent implements OnChanges { // Since fetching the file may take some time, we need to check if the file is still selected. if (!this.binaryFileSelected() && this.selectedFile() === fileName) { - this.editor().changeModel(fileName, code); - this.editor().setPosition(this.fileSession()[fileName].cursor); + this.manageEditorViewState(fileName, code); } this.loadingCount.set(this.loadingCount() - 1); } + /** + * Manages the view state of the monaco editor. The mechanism is as follows: + * When the user is currently viewing a file and switches to a different one (denoted by {@link fileName}), the view + * state of the current file is stored in its session. When switching back to that file, the view state is restored. + * When the current file is also the first file that the user selects, we can't set the view state before 'switching' + * to that file, because there is no previous file. Here we set the view state after the editor is fully initialized. + * @param fileName The name of the selected file, i.e. the file that the user wants to switch to + * @param code The code contained in the file + */ + manageEditorViewState(fileName: string, code: string): void { + const isFirstSelectedFile: boolean = this.currentFileName === undefined; + if (!isFirstSelectedFile) { + this.fileSession()[this.currentFileName].viewState = this.editor().saveViewState(); + } + this.currentFileName = fileName; + + // This initializes the view state + this.editor().changeModel(fileName, code); + this.editor().setPosition(this.fileSession()[fileName].cursor); + if (isFirstSelectedFile) { + this.fileSession()[this.currentFileName].viewState = this.editor().saveViewState(); + } + this.editor().restoreViewState(this.fileSession()[fileName].viewState); + } + onFileTextChanged(text: string): void { if (this.selectedFile() && this.fileSession()[this.selectedFile()!]) { const previousText = this.fileSession()[this.selectedFile()!].code; + const previousViewState = this.fileSession()[this.selectedFile()!].viewState; if (previousText !== text) { - this.fileSession.set({ ...this.fileSession(), [this.selectedFile()!]: { code: text, loadingError: false, cursor: this.editor().getPosition() } }); + this.fileSession.set({ + ...this.fileSession(), + [this.selectedFile()!]: { code: text, loadingError: false, viewState: previousViewState, cursor: this.editor().getPosition() }, + }); this.onFileContentChange.emit({ file: this.selectedFile()!, fileContent: text }); } } @@ -395,6 +428,7 @@ export class CodeEditorMonacoComponent implements OnChanges { async onFileChange(fileChange: FileChange) { if (fileChange instanceof RenameFileChange) { this.fileSession.set(this.fileService.updateFileReferences(this.fileSession(), fileChange)); + this.currentFileName = fileChange.newFileName; for (const annotation of this.annotationsArray) { if (annotation.fileName === fileChange.oldFileName) { annotation.fileName = fileChange.newFileName; @@ -405,7 +439,7 @@ export class CodeEditorMonacoComponent implements OnChanges { this.fileSession.set(this.fileService.updateFileReferences(this.fileSession(), fileChange)); this.storeAnnotations([fileChange.fileName]); } else if (fileChange instanceof CreateFileChange && fileChange.fileType === FileType.FILE) { - this.fileSession.set({ ...this.fileSession(), [fileChange.fileName]: { code: '', cursor: { lineNumber: 0, column: 0 }, loadingError: false } }); + this.fileSession.set({ ...this.fileSession(), [fileChange.fileName]: { code: '', cursor: { lineNumber: 0, column: 0 }, viewState: undefined, loadingError: false } }); } this.setBuildAnnotations(this.annotationsArray); } diff --git a/src/main/webapp/app/shared/monaco-editor/model/actions/monaco-editor.util.ts b/src/main/webapp/app/shared/monaco-editor/model/actions/monaco-editor.util.ts index 9c8e6688324f..078c5d271e72 100644 --- a/src/main/webapp/app/shared/monaco-editor/model/actions/monaco-editor.util.ts +++ b/src/main/webapp/app/shared/monaco-editor/model/actions/monaco-editor.util.ts @@ -6,6 +6,7 @@ export type MonacoEditorTextModel = monaco.editor.ITextModel; export type EditorPosition = monaco.IPosition; export type EditorRange = monaco.IRange; export type EditorOptions = monaco.editor.IEditorOptions; +export type EditorViewState = monaco.editor.ICodeEditorViewState; // Enums export const KeyModifier = monaco.KeyMod; export const KeyCode = monaco.KeyCode; diff --git a/src/main/webapp/app/shared/monaco-editor/monaco-editor.component.ts b/src/main/webapp/app/shared/monaco-editor/monaco-editor.component.ts index 73bb9f68f931..371b3525a43c 100644 --- a/src/main/webapp/app/shared/monaco-editor/monaco-editor.component.ts +++ b/src/main/webapp/app/shared/monaco-editor/monaco-editor.component.ts @@ -8,7 +8,7 @@ import { MonacoEditorLineDecorationsHoverButton } from './model/monaco-editor-li import { TextEditorAction } from 'app/shared/monaco-editor/model/actions/text-editor-action.model'; import { TranslateService } from '@ngx-translate/core'; import { MonacoEditorOptionPreset } from 'app/shared/monaco-editor/model/monaco-editor-option-preset.model'; -import { Disposable, EditorPosition, EditorRange, MonacoEditorTextModel } from 'app/shared/monaco-editor/model/actions/monaco-editor.util'; +import { Disposable, EditorPosition, EditorRange, EditorViewState, MonacoEditorTextModel } from 'app/shared/monaco-editor/model/actions/monaco-editor.util'; import { MonacoTextEditorAdapter } from 'app/shared/monaco-editor/model/actions/adapter/monaco-text-editor.adapter'; import { MonacoEditorService } from 'app/shared/monaco-editor/monaco-editor.service'; import { getOS } from 'app/shared/util/os-detector.util'; @@ -176,6 +176,18 @@ export class MonacoEditorComponent implements OnInit, OnDestroy { this._editor.setPosition(position); } + saveViewState(): EditorViewState | undefined { + return this._editor.saveViewState() ?? undefined; + } + + restoreViewState(viewState: EditorViewState | undefined): void { + if (viewState) { + return this._editor.restoreViewState(viewState); + } else { + return undefined; + } + } + setSelection(range: EditorRange): void { this._editor.setSelection(range); } From 278a505b47ea0e50782183484622c3c8e216a674 Mon Sep 17 00:00:00 2001 From: Christoph Knoedlseder Date: Fri, 29 Nov 2024 14:59:47 +0100 Subject: [PATCH 2/8] adapt tests --- .../monaco/code-editor-monaco.component.ts | 2 +- .../code-editor-monaco.component.spec.ts | 18 +++++++++++------- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/src/main/webapp/app/exercises/programming/shared/code-editor/monaco/code-editor-monaco.component.ts b/src/main/webapp/app/exercises/programming/shared/code-editor/monaco/code-editor-monaco.component.ts index 7ce25e066ded..56569372e970 100644 --- a/src/main/webapp/app/exercises/programming/shared/code-editor/monaco/code-editor-monaco.component.ts +++ b/src/main/webapp/app/exercises/programming/shared/code-editor/monaco/code-editor-monaco.component.ts @@ -215,7 +215,7 @@ export class CodeEditorMonacoComponent implements OnChanges { } /** - * Manages the view state of the monaco editor. The mechanism is as follows: + * Manages the model and view state of the monaco editor. The mechanism is as follows: * When the user is currently viewing a file and switches to a different one (denoted by {@link fileName}), the view * state of the current file is stored in its session. When switching back to that file, the view state is restored. * When the current file is also the first file that the user selects, we can't set the view state before 'switching' diff --git a/src/test/javascript/spec/component/code-editor/code-editor-monaco.component.spec.ts b/src/test/javascript/spec/component/code-editor/code-editor-monaco.component.spec.ts index 9b658fbf5ece..f1d14128f865 100644 --- a/src/test/javascript/spec/component/code-editor/code-editor-monaco.component.spec.ts +++ b/src/test/javascript/spec/component/code-editor/code-editor-monaco.component.spec.ts @@ -154,7 +154,7 @@ describe('CodeEditorMonacoComponent', () => { const changeModelStub = jest.spyOn(comp.editor(), 'changeModel').mockImplementation(); const presentFileName = 'present-file'; const presentFileSession = { - [presentFileName]: { code: 'code\ncode', cursor: { lineNumber: 1, column: 2 }, loadingError: false }, + [presentFileName]: { code: 'code\ncode', cursor: { lineNumber: 1, column: 2 }, viewState: undefined, loadingError: false }, }; fixture.detectChanges(); comp.fileSession.set(presentFileSession); @@ -165,7 +165,7 @@ describe('CodeEditorMonacoComponent', () => { expect(loadFileFromRepositoryStub).toHaveBeenCalledExactlyOnceWith(fileToLoad.fileName); expect(comp.fileSession()).toEqual({ ...presentFileSession, - [fileToLoad.fileName]: { code: fileToLoad.fileContent, cursor: { column: 0, lineNumber: 0 }, loadingError: false }, + [fileToLoad.fileName]: { code: fileToLoad.fileContent, cursor: { column: 0, lineNumber: 0 }, viewState: comp.editor().saveViewState(), loadingError: false }, }); expect(setPositionStub).toHaveBeenCalledTimes(2); expect(changeModelStub).toHaveBeenCalledTimes(2); @@ -174,7 +174,7 @@ describe('CodeEditorMonacoComponent', () => { it('should load a selected file after a loading error', async () => { const fileToLoad = { fileName: 'file-to-load', fileContent: 'some code' }; // File session after loading fails - const fileSession = { [fileToLoad.fileName]: { code: '', loadingError: true, cursor: { lineNumber: 0, column: 0 } } }; + const fileSession = { [fileToLoad.fileName]: { code: '', loadingError: true, cursor: { lineNumber: 0, column: 0 }, viewState: undefined } }; const loadedFileSubject = new BehaviorSubject(fileToLoad); loadFileFromRepositoryStub.mockReturnValue(loadedFileSubject); comp.fileSession.set(fileSession); @@ -182,7 +182,9 @@ describe('CodeEditorMonacoComponent', () => { fixture.detectChanges(); await new Promise(process.nextTick); expect(loadFileFromRepositoryStub).toHaveBeenCalledOnce(); - expect(comp.fileSession()).toEqual({ [fileToLoad.fileName]: { code: fileToLoad.fileContent, loadingError: false, cursor: { lineNumber: 0, column: 0 } } }); + expect(comp.fileSession()).toEqual({ + [fileToLoad.fileName]: { code: fileToLoad.fileContent, loadingError: false, cursor: { lineNumber: 0, column: 0 }, viewState: comp.editor().saveViewState() }, + }); }); it('should not load binaries into the editor', async () => { @@ -214,7 +216,9 @@ describe('CodeEditorMonacoComponent', () => { await new Promise(process.nextTick); expect(loadFileFromRepositoryStub).toHaveBeenCalledOnce(); expect(errorCallbackStub).toHaveBeenCalledExactlyOnceWith(errorCode); - expect(comp.fileSession()).toEqual({ [fileToLoad.fileName]: { code: '', loadingError: true, cursor: { lineNumber: 0, column: 0 } } }); + expect(comp.fileSession()).toEqual({ + [fileToLoad.fileName]: { code: '', loadingError: true, cursor: { lineNumber: 0, column: 0 }, viewState: comp.editor().saveViewState() }, + }); }); it('should discard local changes when the editor is refreshed', async () => { @@ -224,13 +228,13 @@ describe('CodeEditorMonacoComponent', () => { loadFileFromRepositoryStub.mockReturnValue(reloadedFileSubject); fixture.componentRef.setInput('selectedFile', fileToReload.fileName); comp.fileSession.set({ - [fileToReload.fileName]: { code: 'some local undiscarded changes', cursor: { lineNumber: 0, column: 0 }, loadingError: false }, + [fileToReload.fileName]: { code: 'some local undiscarded changes', cursor: { lineNumber: 0, column: 0 }, viewState: undefined, loadingError: false }, }); fixture.componentRef.setInput('editorState', EditorState.CLEAN); // Simulate a refresh of the editor. await comp.ngOnChanges({ editorState: new SimpleChange(EditorState.REFRESHING, EditorState.CLEAN, false) }); expect(comp.fileSession()).toEqual({ - [fileToReload.fileName]: { code: fileToReload.fileContent, cursor: { lineNumber: 0, column: 0 }, loadingError: false }, + [fileToReload.fileName]: { code: fileToReload.fileContent, cursor: { lineNumber: 0, column: 0 }, loadingError: false, viewState: comp.editor().saveViewState() }, }); expect(editorResetStub).toHaveBeenCalledOnce(); }); From 2d91d47b191ce1231495e4a8de16e994575d4872 Mon Sep 17 00:00:00 2001 From: Christoph Knoedlseder Date: Fri, 29 Nov 2024 16:51:54 +0100 Subject: [PATCH 3/8] add viewstate everywhere --- .../code-editor-monaco.component.spec.ts | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/src/test/javascript/spec/component/code-editor/code-editor-monaco.component.spec.ts b/src/test/javascript/spec/component/code-editor/code-editor-monaco.component.spec.ts index f1d14128f865..5191f7c3f2cb 100644 --- a/src/test/javascript/spec/component/code-editor/code-editor-monaco.component.spec.ts +++ b/src/test/javascript/spec/component/code-editor/code-editor-monaco.component.spec.ts @@ -117,11 +117,11 @@ describe('CodeEditorMonacoComponent', () => { [() => fixture.componentRef.setInput('disableActions', true), true], [() => fixture.componentRef.setInput('commitState', CommitState.CONFLICT), true], [() => fixture.componentRef.setInput('selectedFile', undefined), true], - [() => comp.fileSession.set({ ['file']: { code: '', cursor: { lineNumber: 0, column: 0 }, loadingError: true } }), true], // TODO: convert to signal + [() => comp.fileSession.set({ ['file']: { code: '', cursor: { lineNumber: 0, column: 0 }, loadingError: true, viewState: undefined } }), true], // TODO: convert to signal ])('should correctly lock the editor on changes', (setup: () => void, shouldLock: boolean) => { fixture.componentRef.setInput('selectedFile', 'file'); comp.fileSession.set({ - [comp.selectedFile()!]: { code: 'some code', cursor: { lineNumber: 0, column: 0 }, loadingError: false }, + [comp.selectedFile()!]: { code: 'some code', cursor: { lineNumber: 0, column: 0 }, loadingError: false, viewState: undefined }, }); fixture.detectChanges(); setup(); @@ -131,7 +131,7 @@ describe('CodeEditorMonacoComponent', () => { it('should update the file session and notify when the file content changes', () => { const selectedFile = 'file'; const fileSession = { - [selectedFile]: { code: 'some unchanged code', cursor: { lineNumber: 1, column: 1 }, loadingError: false }, + [selectedFile]: { code: 'some unchanged code', cursor: { lineNumber: 1, column: 1 }, loadingError: false, viewState: undefined }, }; const newCode = 'some new code'; const valueCallbackStub = jest.fn(); @@ -142,7 +142,7 @@ describe('CodeEditorMonacoComponent', () => { comp.onFileTextChanged(newCode); expect(valueCallbackStub).toHaveBeenCalledExactlyOnceWith({ file: selectedFile, fileContent: newCode }); expect(comp.fileSession()).toEqual({ - [selectedFile]: { ...fileSession[selectedFile], code: newCode }, + [selectedFile]: { ...fileSession[selectedFile], code: newCode, viewState: undefined }, }); }); @@ -191,7 +191,7 @@ describe('CodeEditorMonacoComponent', () => { const changeModelSpy = jest.spyOn(comp.editor(), 'changeModel'); const fileName = 'file-to-load'; comp.fileSession.set({ - [fileName]: { code: '\0\0\0\0 (binary content)', loadingError: false, cursor: { lineNumber: 0, column: 0 } }, + [fileName]: { code: '\0\0\0\0 (binary content)', loadingError: false, cursor: { lineNumber: 0, column: 0 }, viewState: undefined }, }); fixture.detectChanges(); fixture.componentRef.setInput('selectedFile', fileName); @@ -242,7 +242,7 @@ describe('CodeEditorMonacoComponent', () => { it('should only load the currently selected file', async () => { const changeModelSpy = jest.spyOn(comp.editor(), 'changeModel'); // Occurs when the first file load takes a while, but the user has already selected another file. - comp.fileSession.set({ ['file2']: { code: 'code2', cursor: { lineNumber: 0, column: 0 }, loadingError: false } }); + comp.fileSession.set({ ['file2']: { code: 'code2', cursor: { lineNumber: 0, column: 0 }, loadingError: false, viewState: undefined } }); fixture.detectChanges(); fixture.componentRef.setInput('selectedFile', 'file1'); const longLoadingFileSubject = new Subject(); @@ -262,7 +262,7 @@ describe('CodeEditorMonacoComponent', () => { fixture.detectChanges(); const selectedFile = 'file1'; const fileSession = { - [selectedFile]: { code: 'code\ncode', cursor: { lineNumber: 1, column: 2 }, loadingError: false }, + [selectedFile]: { code: 'code\ncode', cursor: { lineNumber: 1, column: 2 }, loadingError: false, viewState: undefined }, }; comp.fileSession.set(fileSession); fixture.componentRef.setInput('selectedFile', selectedFile); @@ -430,8 +430,8 @@ describe('CodeEditorMonacoComponent', () => { const newFileName = 'new-file-name'; const otherFileName = 'other-file'; const fileSession = { - [oldFileName]: { code: 'renamed', cursor: { lineNumber: 0, column: 0 }, loadingError: false }, - [otherFileName]: { code: 'unrelated', cursor: { lineNumber: 0, column: 0 }, loadingError: false }, + [oldFileName]: { code: 'renamed', cursor: { lineNumber: 0, column: 0 }, loadingError: false, viewState: undefined }, + [otherFileName]: { code: 'unrelated', cursor: { lineNumber: 0, column: 0 }, loadingError: false, viewState: undefined }, }; fixture.detectChanges(); comp.fileSession.set({ ...fileSession }); @@ -447,8 +447,8 @@ describe('CodeEditorMonacoComponent', () => { const fileToDeleteName = 'file-to-delete'; const otherFileName = 'other-file'; const fileSession = { - [fileToDeleteName]: { code: 'will be deleted', cursor: { lineNumber: 0, column: 0 }, loadingError: false }, - [otherFileName]: { code: 'unrelated', cursor: { lineNumber: 0, column: 0 }, loadingError: false }, + [fileToDeleteName]: { code: 'will be deleted', cursor: { lineNumber: 0, column: 0 }, loadingError: false, viewState: undefined }, + [otherFileName]: { code: 'unrelated', cursor: { lineNumber: 0, column: 0 }, loadingError: false, viewState: undefined }, }; fixture.detectChanges(); comp.fileSession.set({ ...fileSession }); @@ -463,7 +463,7 @@ describe('CodeEditorMonacoComponent', () => { const fileToCreateName = 'file-to-create'; const otherFileName = 'other-file'; const fileSession = { - [otherFileName]: { code: 'unrelated', cursor: { lineNumber: 0, column: 0 }, loadingError: false }, + [otherFileName]: { code: 'unrelated', cursor: { lineNumber: 0, column: 0 }, loadingError: false, viewState: undefined }, }; fixture.detectChanges(); comp.fileSession.set({ ...fileSession }); @@ -471,7 +471,7 @@ describe('CodeEditorMonacoComponent', () => { await comp.onFileChange(createFileChange); expect(comp.fileSession()).toEqual({ [otherFileName]: fileSession[otherFileName], - [fileToCreateName]: { code: '', cursor: { lineNumber: 0, column: 0 }, loadingError: false }, + [fileToCreateName]: { code: '', cursor: { lineNumber: 0, column: 0 }, loadingError: false, viewState: undefined }, }); }); From 585e749018669285b6a92b5421ee5a50a3015a3d Mon Sep 17 00:00:00 2001 From: Christoph Knoedlseder Date: Mon, 16 Dec 2024 17:13:54 +0100 Subject: [PATCH 4/8] use scrollTop attribute to remember scroll position --- .../monaco/code-editor-monaco.component.ts | 61 +++++++------------ .../monaco-editor/monaco-editor.component.ts | 14 ++--- 2 files changed, 27 insertions(+), 48 deletions(-) diff --git a/src/main/webapp/app/exercises/programming/shared/code-editor/monaco/code-editor-monaco.component.ts b/src/main/webapp/app/exercises/programming/shared/code-editor/monaco/code-editor-monaco.component.ts index 56569372e970..30857db9b8d2 100644 --- a/src/main/webapp/app/exercises/programming/shared/code-editor/monaco/code-editor-monaco.component.ts +++ b/src/main/webapp/app/exercises/programming/shared/code-editor/monaco/code-editor-monaco.component.ts @@ -37,12 +37,12 @@ import { fromPairs, pickBy } from 'lodash-es'; import { CodeEditorTutorAssessmentInlineFeedbackSuggestionComponent } from 'app/exercises/programming/assess/code-editor-tutor-assessment-inline-feedback-suggestion.component'; import { MonacoEditorLineHighlight } from 'app/shared/monaco-editor/model/monaco-editor-line-highlight.model'; import { FileTypeService } from 'app/exercises/programming/shared/service/file-type.service'; -import { EditorPosition, EditorViewState } from 'app/shared/monaco-editor/model/actions/monaco-editor.util'; +import { EditorPosition } from 'app/shared/monaco-editor/model/actions/monaco-editor.util'; import { ArtemisProgrammingManualAssessmentModule } from 'app/exercises/programming/assess/programming-manual-assessment.module'; import { CodeEditorHeaderComponent } from 'app/exercises/programming/shared/code-editor/header/code-editor-header.component'; import { ArtemisSharedModule } from 'app/shared/shared.module'; -type FileSession = { [fileName: string]: { code: string; cursor: EditorPosition; viewState: EditorViewState | undefined; loadingError: boolean } }; +type FileSession = { [fileName: string]: { code: string; cursor: EditorPosition; scrollTop: number; loadingError: boolean } }; type FeedbackWithLineAndReference = Feedback & { line: number; reference: string }; export type Annotation = { fileName: string; row: number; column: number; text: string; type: string; timestamp: number; hash?: string }; @Component({ @@ -116,8 +116,6 @@ export class CodeEditorMonacoComponent implements OnChanges { this.filterFeedbackForSelectedFile(this.feedbackSuggestionsInternal()).map((f) => this.attachLineAndReferenceToFeedback(f)), ); - private currentFileName: string; - /** * Attaches the line number & reference to a feedback item, or -1 if no line is available. This is used to disambiguate feedback items in the template, avoiding warnings. * @param feedback The feedback item to attach the line to. @@ -159,7 +157,8 @@ export class CodeEditorMonacoComponent implements OnChanges { this.editor().reset(); } if ((changes.selectedFile && this.selectedFile()) || editorWasRefreshed) { - await this.selectFileInEditor(this.selectedFile()); + const previousFileName: string | undefined = changes.selectedFile.previousValue; + await this.selectFileInEditor(previousFileName, this.selectedFile()); this.setBuildAnnotations(this.annotationsArray); this.newFeedbackLines.set([]); this.renderFeedbackWidgets(); @@ -177,17 +176,17 @@ export class CodeEditorMonacoComponent implements OnChanges { this.editor().layout(); } - async selectFileInEditor(fileName: string | undefined): Promise { - if (!fileName) { + async selectFileInEditor(previousFileName: string | undefined, selectedFileName: string | undefined): Promise { + if (!selectedFileName) { // There is nothing to be done, as the editor will be hidden when there is no file. return; } this.loadingCount.set(this.loadingCount() + 1); - if (!this.fileSession()[fileName] || this.fileSession()[fileName].loadingError) { + if (!this.fileSession()[selectedFileName] || this.fileSession()[selectedFileName].loadingError) { let fileContent = ''; let loadingError = false; try { - fileContent = await firstValueFrom(this.repositoryFileService.getFile(fileName).pipe(timeout(CodeEditorMonacoComponent.FILE_TIMEOUT))).then( + fileContent = await firstValueFrom(this.repositoryFileService.getFile(selectedFileName).pipe(timeout(CodeEditorMonacoComponent.FILE_TIMEOUT))).then( (fileObj) => fileObj.fileContent, ); } catch (error) { @@ -200,53 +199,38 @@ export class CodeEditorMonacoComponent implements OnChanges { } this.fileSession.set({ ...this.fileSession(), - [fileName]: { code: fileContent, loadingError: loadingError, viewState: undefined, cursor: { column: 0, lineNumber: 0 } }, + [selectedFileName]: { code: fileContent, loadingError: loadingError, scrollTop: 0, cursor: { column: 0, lineNumber: 0 } }, }); } - const code = this.fileSession()[fileName].code; + const code = this.fileSession()[selectedFileName].code; this.binaryFileSelected.set(this.fileTypeService.isBinaryContent(code)); // Since fetching the file may take some time, we need to check if the file is still selected. - if (!this.binaryFileSelected() && this.selectedFile() === fileName) { - this.manageEditorViewState(fileName, code); + if (!this.binaryFileSelected() && this.selectedFile() === selectedFileName) { + this.switchToSelectedFile(previousFileName, selectedFileName, code); } this.loadingCount.set(this.loadingCount() - 1); } - /** - * Manages the model and view state of the monaco editor. The mechanism is as follows: - * When the user is currently viewing a file and switches to a different one (denoted by {@link fileName}), the view - * state of the current file is stored in its session. When switching back to that file, the view state is restored. - * When the current file is also the first file that the user selects, we can't set the view state before 'switching' - * to that file, because there is no previous file. Here we set the view state after the editor is fully initialized. - * @param fileName The name of the selected file, i.e. the file that the user wants to switch to - * @param code The code contained in the file - */ - manageEditorViewState(fileName: string, code: string): void { - const isFirstSelectedFile: boolean = this.currentFileName === undefined; - if (!isFirstSelectedFile) { - this.fileSession()[this.currentFileName].viewState = this.editor().saveViewState(); - } - this.currentFileName = fileName; - - // This initializes the view state - this.editor().changeModel(fileName, code); - this.editor().setPosition(this.fileSession()[fileName].cursor); - if (isFirstSelectedFile) { - this.fileSession()[this.currentFileName].viewState = this.editor().saveViewState(); + switchToSelectedFile(previousFileName: string | undefined, selectedFileName: string, code: string): void { + // if a file was previously selected, we save the old scrollTop before switching to another file + if (previousFileName && this.fileSession()[previousFileName]) { + this.fileSession()[previousFileName].scrollTop = this.editor().getScrollTop(); } - this.editor().restoreViewState(this.fileSession()[fileName].viewState); + this.editor().changeModel(selectedFileName, code); + this.editor().setPosition(this.fileSession()[selectedFileName].cursor); + this.editor().setScrollTop(this.fileSession()[this.selectedFile()!].scrollTop ?? 0); } onFileTextChanged(text: string): void { if (this.selectedFile() && this.fileSession()[this.selectedFile()!]) { const previousText = this.fileSession()[this.selectedFile()!].code; - const previousViewState = this.fileSession()[this.selectedFile()!].viewState; + const previousScrollTop = this.fileSession()[this.selectedFile()!].scrollTop; if (previousText !== text) { this.fileSession.set({ ...this.fileSession(), - [this.selectedFile()!]: { code: text, loadingError: false, viewState: previousViewState, cursor: this.editor().getPosition() }, + [this.selectedFile()!]: { code: text, loadingError: false, scrollTop: previousScrollTop, cursor: this.editor().getPosition() }, }); this.onFileContentChange.emit({ file: this.selectedFile()!, fileContent: text }); } @@ -428,7 +412,6 @@ export class CodeEditorMonacoComponent implements OnChanges { async onFileChange(fileChange: FileChange) { if (fileChange instanceof RenameFileChange) { this.fileSession.set(this.fileService.updateFileReferences(this.fileSession(), fileChange)); - this.currentFileName = fileChange.newFileName; for (const annotation of this.annotationsArray) { if (annotation.fileName === fileChange.oldFileName) { annotation.fileName = fileChange.newFileName; @@ -439,7 +422,7 @@ export class CodeEditorMonacoComponent implements OnChanges { this.fileSession.set(this.fileService.updateFileReferences(this.fileSession(), fileChange)); this.storeAnnotations([fileChange.fileName]); } else if (fileChange instanceof CreateFileChange && fileChange.fileType === FileType.FILE) { - this.fileSession.set({ ...this.fileSession(), [fileChange.fileName]: { code: '', cursor: { lineNumber: 0, column: 0 }, viewState: undefined, loadingError: false } }); + this.fileSession.set({ ...this.fileSession(), [fileChange.fileName]: { code: '', cursor: { lineNumber: 0, column: 0 }, scrollTop: 0, loadingError: false } }); } this.setBuildAnnotations(this.annotationsArray); } diff --git a/src/main/webapp/app/shared/monaco-editor/monaco-editor.component.ts b/src/main/webapp/app/shared/monaco-editor/monaco-editor.component.ts index 321676d21b49..ad49c129270f 100644 --- a/src/main/webapp/app/shared/monaco-editor/monaco-editor.component.ts +++ b/src/main/webapp/app/shared/monaco-editor/monaco-editor.component.ts @@ -8,7 +8,7 @@ import { MonacoEditorLineDecorationsHoverButton } from './model/monaco-editor-li import { TextEditorAction } from 'app/shared/monaco-editor/model/actions/text-editor-action.model'; import { TranslateService } from '@ngx-translate/core'; import { MonacoEditorOptionPreset } from 'app/shared/monaco-editor/model/monaco-editor-option-preset.model'; -import { Disposable, EditorPosition, EditorRange, EditorViewState, MonacoEditorTextModel } from 'app/shared/monaco-editor/model/actions/monaco-editor.util'; +import { Disposable, EditorPosition, EditorRange, MonacoEditorTextModel } from 'app/shared/monaco-editor/model/actions/monaco-editor.util'; import { MonacoTextEditorAdapter } from 'app/shared/monaco-editor/model/actions/adapter/monaco-text-editor.adapter'; import { MonacoEditorService } from 'app/shared/monaco-editor/monaco-editor.service'; import { getOS } from 'app/shared/util/os-detector.util'; @@ -189,16 +189,12 @@ export class MonacoEditorComponent implements OnInit, OnDestroy { this._editor.setPosition(position); } - saveViewState(): EditorViewState | undefined { - return this._editor.saveViewState() ?? undefined; + getScrollTop(): number { + return this._editor.getScrollTop(); } - restoreViewState(viewState: EditorViewState | undefined): void { - if (viewState) { - return this._editor.restoreViewState(viewState); - } else { - return undefined; - } + setScrollTop(scrollTop: number) { + this._editor.setScrollTop(scrollTop); } setSelection(range: EditorRange): void { From 25cf2b966e4081e577482473ee918a541955952b Mon Sep 17 00:00:00 2001 From: Christoph Knoedlseder Date: Wed, 18 Dec 2024 13:51:45 +0100 Subject: [PATCH 5/8] test for scroll position --- .../monaco/code-editor-monaco.component.ts | 2 +- .../code-editor-monaco.component.spec.ts | 68 +++++++++++-------- 2 files changed, 42 insertions(+), 28 deletions(-) diff --git a/src/main/webapp/app/exercises/programming/shared/code-editor/monaco/code-editor-monaco.component.ts b/src/main/webapp/app/exercises/programming/shared/code-editor/monaco/code-editor-monaco.component.ts index 30857db9b8d2..dcc1be221733 100644 --- a/src/main/webapp/app/exercises/programming/shared/code-editor/monaco/code-editor-monaco.component.ts +++ b/src/main/webapp/app/exercises/programming/shared/code-editor/monaco/code-editor-monaco.component.ts @@ -157,7 +157,7 @@ export class CodeEditorMonacoComponent implements OnChanges { this.editor().reset(); } if ((changes.selectedFile && this.selectedFile()) || editorWasRefreshed) { - const previousFileName: string | undefined = changes.selectedFile.previousValue; + const previousFileName: string | undefined = changes.selectedFile?.previousValue; await this.selectFileInEditor(previousFileName, this.selectedFile()); this.setBuildAnnotations(this.annotationsArray); this.newFeedbackLines.set([]); diff --git a/src/test/javascript/spec/component/code-editor/code-editor-monaco.component.spec.ts b/src/test/javascript/spec/component/code-editor/code-editor-monaco.component.spec.ts index 5191f7c3f2cb..75f5f8f09ca2 100644 --- a/src/test/javascript/spec/component/code-editor/code-editor-monaco.component.spec.ts +++ b/src/test/javascript/spec/component/code-editor/code-editor-monaco.component.spec.ts @@ -84,7 +84,7 @@ describe('CodeEditorMonacoComponent', () => { it('should not try to load a file if none is selected', async () => { const editorChangeModelSpy = jest.spyOn(comp.editor(), 'changeModel'); fixture.detectChanges(); - await comp.selectFileInEditor(undefined); + await comp.selectFileInEditor(undefined, undefined); expect(editorChangeModelSpy).not.toHaveBeenCalled(); expect(loadFileFromRepositoryStub).not.toHaveBeenCalled(); }); @@ -117,11 +117,11 @@ describe('CodeEditorMonacoComponent', () => { [() => fixture.componentRef.setInput('disableActions', true), true], [() => fixture.componentRef.setInput('commitState', CommitState.CONFLICT), true], [() => fixture.componentRef.setInput('selectedFile', undefined), true], - [() => comp.fileSession.set({ ['file']: { code: '', cursor: { lineNumber: 0, column: 0 }, loadingError: true, viewState: undefined } }), true], // TODO: convert to signal + [() => comp.fileSession.set({ ['file']: { code: '', cursor: { lineNumber: 0, column: 0 }, loadingError: true, scrollTop: 0 } }), true], // TODO: convert to signal ])('should correctly lock the editor on changes', (setup: () => void, shouldLock: boolean) => { fixture.componentRef.setInput('selectedFile', 'file'); comp.fileSession.set({ - [comp.selectedFile()!]: { code: 'some code', cursor: { lineNumber: 0, column: 0 }, loadingError: false, viewState: undefined }, + [comp.selectedFile()!]: { code: 'some code', cursor: { lineNumber: 0, column: 0 }, loadingError: false, scrollTop: 0 }, }); fixture.detectChanges(); setup(); @@ -131,7 +131,7 @@ describe('CodeEditorMonacoComponent', () => { it('should update the file session and notify when the file content changes', () => { const selectedFile = 'file'; const fileSession = { - [selectedFile]: { code: 'some unchanged code', cursor: { lineNumber: 1, column: 1 }, loadingError: false, viewState: undefined }, + [selectedFile]: { code: 'some unchanged code', cursor: { lineNumber: 1, column: 1 }, loadingError: false, scrollTop: 0 }, }; const newCode = 'some new code'; const valueCallbackStub = jest.fn(); @@ -142,7 +142,7 @@ describe('CodeEditorMonacoComponent', () => { comp.onFileTextChanged(newCode); expect(valueCallbackStub).toHaveBeenCalledExactlyOnceWith({ file: selectedFile, fileContent: newCode }); expect(comp.fileSession()).toEqual({ - [selectedFile]: { ...fileSession[selectedFile], code: newCode, viewState: undefined }, + [selectedFile]: { ...fileSession[selectedFile], code: newCode, scrollTop: 0 }, }); }); @@ -154,18 +154,18 @@ describe('CodeEditorMonacoComponent', () => { const changeModelStub = jest.spyOn(comp.editor(), 'changeModel').mockImplementation(); const presentFileName = 'present-file'; const presentFileSession = { - [presentFileName]: { code: 'code\ncode', cursor: { lineNumber: 1, column: 2 }, viewState: undefined, loadingError: false }, + [presentFileName]: { code: 'code\ncode', cursor: { lineNumber: 1, column: 2 }, scrollTop: 0, loadingError: false }, }; fixture.detectChanges(); comp.fileSession.set(presentFileSession); fixture.componentRef.setInput('selectedFile', fileToLoad.fileName); - await comp.selectFileInEditor(fileToLoad.fileName); + await comp.selectFileInEditor(presentFileName, fileToLoad.fileName); fixture.componentRef.setInput('selectedFile', presentFileName); - await comp.selectFileInEditor(presentFileName); + await comp.selectFileInEditor(fileToLoad.fileName, presentFileName); expect(loadFileFromRepositoryStub).toHaveBeenCalledExactlyOnceWith(fileToLoad.fileName); expect(comp.fileSession()).toEqual({ ...presentFileSession, - [fileToLoad.fileName]: { code: fileToLoad.fileContent, cursor: { column: 0, lineNumber: 0 }, viewState: comp.editor().saveViewState(), loadingError: false }, + [fileToLoad.fileName]: { code: fileToLoad.fileContent, cursor: { column: 0, lineNumber: 0 }, scrollTop: 0, loadingError: false }, }); expect(setPositionStub).toHaveBeenCalledTimes(2); expect(changeModelStub).toHaveBeenCalledTimes(2); @@ -174,7 +174,7 @@ describe('CodeEditorMonacoComponent', () => { it('should load a selected file after a loading error', async () => { const fileToLoad = { fileName: 'file-to-load', fileContent: 'some code' }; // File session after loading fails - const fileSession = { [fileToLoad.fileName]: { code: '', loadingError: true, cursor: { lineNumber: 0, column: 0 }, viewState: undefined } }; + const fileSession = { [fileToLoad.fileName]: { code: '', loadingError: true, cursor: { lineNumber: 0, column: 0 }, scrollTop: 0 } }; const loadedFileSubject = new BehaviorSubject(fileToLoad); loadFileFromRepositoryStub.mockReturnValue(loadedFileSubject); comp.fileSession.set(fileSession); @@ -183,7 +183,7 @@ describe('CodeEditorMonacoComponent', () => { await new Promise(process.nextTick); expect(loadFileFromRepositoryStub).toHaveBeenCalledOnce(); expect(comp.fileSession()).toEqual({ - [fileToLoad.fileName]: { code: fileToLoad.fileContent, loadingError: false, cursor: { lineNumber: 0, column: 0 }, viewState: comp.editor().saveViewState() }, + [fileToLoad.fileName]: { code: fileToLoad.fileContent, loadingError: false, cursor: { lineNumber: 0, column: 0 }, scrollTop: 0 }, }); }); @@ -191,11 +191,11 @@ describe('CodeEditorMonacoComponent', () => { const changeModelSpy = jest.spyOn(comp.editor(), 'changeModel'); const fileName = 'file-to-load'; comp.fileSession.set({ - [fileName]: { code: '\0\0\0\0 (binary content)', loadingError: false, cursor: { lineNumber: 0, column: 0 }, viewState: undefined }, + [fileName]: { code: '\0\0\0\0 (binary content)', loadingError: false, cursor: { lineNumber: 0, column: 0 }, scrollTop: 0 }, }); fixture.detectChanges(); fixture.componentRef.setInput('selectedFile', fileName); - await comp.selectFileInEditor(fileName); + await comp.selectFileInEditor(undefined, fileName); expect(changeModelSpy).not.toHaveBeenCalled(); expect(comp.binaryFileSelected()).toBeTrue(); }); @@ -217,7 +217,7 @@ describe('CodeEditorMonacoComponent', () => { expect(loadFileFromRepositoryStub).toHaveBeenCalledOnce(); expect(errorCallbackStub).toHaveBeenCalledExactlyOnceWith(errorCode); expect(comp.fileSession()).toEqual({ - [fileToLoad.fileName]: { code: '', loadingError: true, cursor: { lineNumber: 0, column: 0 }, viewState: comp.editor().saveViewState() }, + [fileToLoad.fileName]: { code: '', loadingError: true, cursor: { lineNumber: 0, column: 0 }, scrollTop: 0 }, }); }); @@ -228,13 +228,13 @@ describe('CodeEditorMonacoComponent', () => { loadFileFromRepositoryStub.mockReturnValue(reloadedFileSubject); fixture.componentRef.setInput('selectedFile', fileToReload.fileName); comp.fileSession.set({ - [fileToReload.fileName]: { code: 'some local undiscarded changes', cursor: { lineNumber: 0, column: 0 }, viewState: undefined, loadingError: false }, + [fileToReload.fileName]: { code: 'some local undiscarded changes', cursor: { lineNumber: 0, column: 0 }, scrollTop: 0, loadingError: false }, }); fixture.componentRef.setInput('editorState', EditorState.CLEAN); // Simulate a refresh of the editor. await comp.ngOnChanges({ editorState: new SimpleChange(EditorState.REFRESHING, EditorState.CLEAN, false) }); expect(comp.fileSession()).toEqual({ - [fileToReload.fileName]: { code: fileToReload.fileContent, cursor: { lineNumber: 0, column: 0 }, loadingError: false, viewState: comp.editor().saveViewState() }, + [fileToReload.fileName]: { code: fileToReload.fileContent, cursor: { lineNumber: 0, column: 0 }, loadingError: false, scrollTop: 0 }, }); expect(editorResetStub).toHaveBeenCalledOnce(); }); @@ -242,15 +242,15 @@ describe('CodeEditorMonacoComponent', () => { it('should only load the currently selected file', async () => { const changeModelSpy = jest.spyOn(comp.editor(), 'changeModel'); // Occurs when the first file load takes a while, but the user has already selected another file. - comp.fileSession.set({ ['file2']: { code: 'code2', cursor: { lineNumber: 0, column: 0 }, loadingError: false, viewState: undefined } }); + comp.fileSession.set({ ['file2']: { code: 'code2', cursor: { lineNumber: 0, column: 0 }, loadingError: false, scrollTop: 0 } }); fixture.detectChanges(); fixture.componentRef.setInput('selectedFile', 'file1'); const longLoadingFileSubject = new Subject(); loadFileFromRepositoryStub.mockReturnValue(longLoadingFileSubject); // We do not await the promise here, as we want to simulate the user selecting another file while the first one is still loading. - const firstFileChange = comp.selectFileInEditor('file1'); + const firstFileChange = comp.selectFileInEditor(undefined, 'file1'); fixture.componentRef.setInput('selectedFile', 'file2'); - await comp.selectFileInEditor('file2'); + await comp.selectFileInEditor('file2', 'file2'); longLoadingFileSubject.next({ fileName: 'file1', fileContent: 'some code that took a while to retrieve' }); await firstFileChange; expect(changeModelSpy).toHaveBeenCalledExactlyOnceWith('file2', 'code2'); @@ -262,11 +262,11 @@ describe('CodeEditorMonacoComponent', () => { fixture.detectChanges(); const selectedFile = 'file1'; const fileSession = { - [selectedFile]: { code: 'code\ncode', cursor: { lineNumber: 1, column: 2 }, loadingError: false, viewState: undefined }, + [selectedFile]: { code: 'code\ncode', cursor: { lineNumber: 1, column: 2 }, loadingError: false, scrollTop: 0 }, }; comp.fileSession.set(fileSession); fixture.componentRef.setInput('selectedFile', selectedFile); - await comp.selectFileInEditor(selectedFile); + await comp.selectFileInEditor(undefined, selectedFile); expect(setPositionStub).toHaveBeenCalledExactlyOnceWith(fileSession[selectedFile].cursor); expect(changeModelStub).toHaveBeenCalledExactlyOnceWith(selectedFile, fileSession[selectedFile].code); }); @@ -430,8 +430,8 @@ describe('CodeEditorMonacoComponent', () => { const newFileName = 'new-file-name'; const otherFileName = 'other-file'; const fileSession = { - [oldFileName]: { code: 'renamed', cursor: { lineNumber: 0, column: 0 }, loadingError: false, viewState: undefined }, - [otherFileName]: { code: 'unrelated', cursor: { lineNumber: 0, column: 0 }, loadingError: false, viewState: undefined }, + [oldFileName]: { code: 'renamed', cursor: { lineNumber: 0, column: 0 }, loadingError: false, scrollTop: 0 }, + [otherFileName]: { code: 'unrelated', cursor: { lineNumber: 0, column: 0 }, loadingError: false, scrollTop: 0 }, }; fixture.detectChanges(); comp.fileSession.set({ ...fileSession }); @@ -447,8 +447,8 @@ describe('CodeEditorMonacoComponent', () => { const fileToDeleteName = 'file-to-delete'; const otherFileName = 'other-file'; const fileSession = { - [fileToDeleteName]: { code: 'will be deleted', cursor: { lineNumber: 0, column: 0 }, loadingError: false, viewState: undefined }, - [otherFileName]: { code: 'unrelated', cursor: { lineNumber: 0, column: 0 }, loadingError: false, viewState: undefined }, + [fileToDeleteName]: { code: 'will be deleted', cursor: { lineNumber: 0, column: 0 }, loadingError: false, scrollTop: 0 }, + [otherFileName]: { code: 'unrelated', cursor: { lineNumber: 0, column: 0 }, loadingError: false, scrollTop: 0 }, }; fixture.detectChanges(); comp.fileSession.set({ ...fileSession }); @@ -463,7 +463,7 @@ describe('CodeEditorMonacoComponent', () => { const fileToCreateName = 'file-to-create'; const otherFileName = 'other-file'; const fileSession = { - [otherFileName]: { code: 'unrelated', cursor: { lineNumber: 0, column: 0 }, loadingError: false, viewState: undefined }, + [otherFileName]: { code: 'unrelated', cursor: { lineNumber: 0, column: 0 }, loadingError: false, scrollTop: 0 }, }; fixture.detectChanges(); comp.fileSession.set({ ...fileSession }); @@ -471,7 +471,7 @@ describe('CodeEditorMonacoComponent', () => { await comp.onFileChange(createFileChange); expect(comp.fileSession()).toEqual({ [otherFileName]: fileSession[otherFileName], - [fileToCreateName]: { code: '', cursor: { lineNumber: 0, column: 0 }, loadingError: false, viewState: undefined }, + [fileToCreateName]: { code: '', cursor: { lineNumber: 0, column: 0 }, loadingError: false, scrollTop: 0 }, }); }); @@ -481,4 +481,18 @@ describe('CodeEditorMonacoComponent', () => { comp.highlightLines(1, 2); expect(highlightStub).toHaveBeenCalledExactlyOnceWith(1, 2, CodeEditorMonacoComponent.CLASS_DIFF_LINE_HIGHLIGHT); }); + + it('should remember scroll position', async () => { + const setScrollTopStub = jest.spyOn(comp.editor(), 'setScrollTop'); + const scrolledFile = 'file'; + const scrollTop = 42; + const fileSession = { + [scrolledFile]: { code: 'unrelated', cursor: { lineNumber: 0, column: 0 }, loadingError: false, scrollTop: scrollTop }, + }; + fixture.detectChanges(); + comp.fileSession.set(fileSession); + fixture.componentRef.setInput('selectedFile', scrolledFile); + await comp.selectFileInEditor(undefined, scrolledFile); + expect(setScrollTopStub).toHaveBeenCalledExactlyOnceWith(scrollTop); + }); }); From 729b5632e74f14c725d87494fa074ecacc706b8b Mon Sep 17 00:00:00 2001 From: Christoph Knoedlseder Date: Thu, 19 Dec 2024 22:38:56 +0100 Subject: [PATCH 6/8] fix integration test --- .../code-editor/code-editor-container.integration.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/javascript/spec/integration/code-editor/code-editor-container.integration.spec.ts b/src/test/javascript/spec/integration/code-editor/code-editor-container.integration.spec.ts index 5d02812f588b..2c2d1ba569bf 100644 --- a/src/test/javascript/spec/integration/code-editor/code-editor-container.integration.spec.ts +++ b/src/test/javascript/spec/integration/code-editor/code-editor-container.integration.spec.ts @@ -258,7 +258,7 @@ describe('CodeEditorContainerIntegration', () => { const loadFile = async (fileName: string, fileContent: string) => { getFileStub.mockReturnValue(of({ fileContent })); container.fileBrowser.selectedFile = fileName; - await container.monacoEditor.selectFileInEditor(fileName); + await container.monacoEditor.selectFileInEditor(undefined, fileName); }; it('should initialize all components correctly if all server calls are successful', fakeAsync(() => { From ff867da9bae448f574f34c94bc099bdd97158c07 Mon Sep 17 00:00:00 2001 From: Christoph Knoedlseder Date: Fri, 20 Dec 2024 12:36:35 +0100 Subject: [PATCH 7/8] remove viewstate from the util --- .../app/shared/monaco-editor/model/actions/monaco-editor.util.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/main/webapp/app/shared/monaco-editor/model/actions/monaco-editor.util.ts b/src/main/webapp/app/shared/monaco-editor/model/actions/monaco-editor.util.ts index 078c5d271e72..9c8e6688324f 100644 --- a/src/main/webapp/app/shared/monaco-editor/model/actions/monaco-editor.util.ts +++ b/src/main/webapp/app/shared/monaco-editor/model/actions/monaco-editor.util.ts @@ -6,7 +6,6 @@ export type MonacoEditorTextModel = monaco.editor.ITextModel; export type EditorPosition = monaco.IPosition; export type EditorRange = monaco.IRange; export type EditorOptions = monaco.editor.IEditorOptions; -export type EditorViewState = monaco.editor.ICodeEditorViewState; // Enums export const KeyModifier = monaco.KeyMod; export const KeyCode = monaco.KeyCode; From ff52315961b909a41f495324b5243d3ab92a7f4e Mon Sep 17 00:00:00 2001 From: Christoph Knoedlseder Date: Fri, 20 Dec 2024 12:54:23 +0100 Subject: [PATCH 8/8] store scollTop before selecting file! --- .../monaco/code-editor-monaco.component.ts | 28 +++++++++---------- .../code-editor-monaco.component.spec.ts | 16 +++++------ .../code-editor-container.integration.spec.ts | 2 +- 3 files changed, 23 insertions(+), 23 deletions(-) diff --git a/src/main/webapp/app/exercises/programming/shared/code-editor/monaco/code-editor-monaco.component.ts b/src/main/webapp/app/exercises/programming/shared/code-editor/monaco/code-editor-monaco.component.ts index dcc1be221733..60ea6ca7fa0e 100644 --- a/src/main/webapp/app/exercises/programming/shared/code-editor/monaco/code-editor-monaco.component.ts +++ b/src/main/webapp/app/exercises/programming/shared/code-editor/monaco/code-editor-monaco.component.ts @@ -158,7 +158,11 @@ export class CodeEditorMonacoComponent implements OnChanges { } if ((changes.selectedFile && this.selectedFile()) || editorWasRefreshed) { const previousFileName: string | undefined = changes.selectedFile?.previousValue; - await this.selectFileInEditor(previousFileName, this.selectedFile()); + // we save the old scrollTop before switching to another file + if (previousFileName && this.fileSession()[previousFileName]) { + this.fileSession()[previousFileName].scrollTop = this.editor().getScrollTop(); + } + await this.selectFileInEditor(this.selectedFile()); this.setBuildAnnotations(this.annotationsArray); this.newFeedbackLines.set([]); this.renderFeedbackWidgets(); @@ -176,17 +180,17 @@ export class CodeEditorMonacoComponent implements OnChanges { this.editor().layout(); } - async selectFileInEditor(previousFileName: string | undefined, selectedFileName: string | undefined): Promise { - if (!selectedFileName) { + async selectFileInEditor(fileName: string | undefined): Promise { + if (!fileName) { // There is nothing to be done, as the editor will be hidden when there is no file. return; } this.loadingCount.set(this.loadingCount() + 1); - if (!this.fileSession()[selectedFileName] || this.fileSession()[selectedFileName].loadingError) { + if (!this.fileSession()[fileName] || this.fileSession()[fileName].loadingError) { let fileContent = ''; let loadingError = false; try { - fileContent = await firstValueFrom(this.repositoryFileService.getFile(selectedFileName).pipe(timeout(CodeEditorMonacoComponent.FILE_TIMEOUT))).then( + fileContent = await firstValueFrom(this.repositoryFileService.getFile(fileName).pipe(timeout(CodeEditorMonacoComponent.FILE_TIMEOUT))).then( (fileObj) => fileObj.fileContent, ); } catch (error) { @@ -199,25 +203,21 @@ export class CodeEditorMonacoComponent implements OnChanges { } this.fileSession.set({ ...this.fileSession(), - [selectedFileName]: { code: fileContent, loadingError: loadingError, scrollTop: 0, cursor: { column: 0, lineNumber: 0 } }, + [fileName]: { code: fileContent, loadingError: loadingError, scrollTop: 0, cursor: { column: 0, lineNumber: 0 } }, }); } - const code = this.fileSession()[selectedFileName].code; + const code = this.fileSession()[fileName].code; this.binaryFileSelected.set(this.fileTypeService.isBinaryContent(code)); // Since fetching the file may take some time, we need to check if the file is still selected. - if (!this.binaryFileSelected() && this.selectedFile() === selectedFileName) { - this.switchToSelectedFile(previousFileName, selectedFileName, code); + if (!this.binaryFileSelected() && this.selectedFile() === fileName) { + this.switchToSelectedFile(fileName, code); } this.loadingCount.set(this.loadingCount() - 1); } - switchToSelectedFile(previousFileName: string | undefined, selectedFileName: string, code: string): void { - // if a file was previously selected, we save the old scrollTop before switching to another file - if (previousFileName && this.fileSession()[previousFileName]) { - this.fileSession()[previousFileName].scrollTop = this.editor().getScrollTop(); - } + switchToSelectedFile(selectedFileName: string, code: string): void { this.editor().changeModel(selectedFileName, code); this.editor().setPosition(this.fileSession()[selectedFileName].cursor); this.editor().setScrollTop(this.fileSession()[this.selectedFile()!].scrollTop ?? 0); diff --git a/src/test/javascript/spec/component/code-editor/code-editor-monaco.component.spec.ts b/src/test/javascript/spec/component/code-editor/code-editor-monaco.component.spec.ts index 75f5f8f09ca2..31a18a01eb44 100644 --- a/src/test/javascript/spec/component/code-editor/code-editor-monaco.component.spec.ts +++ b/src/test/javascript/spec/component/code-editor/code-editor-monaco.component.spec.ts @@ -84,7 +84,7 @@ describe('CodeEditorMonacoComponent', () => { it('should not try to load a file if none is selected', async () => { const editorChangeModelSpy = jest.spyOn(comp.editor(), 'changeModel'); fixture.detectChanges(); - await comp.selectFileInEditor(undefined, undefined); + await comp.selectFileInEditor(undefined); expect(editorChangeModelSpy).not.toHaveBeenCalled(); expect(loadFileFromRepositoryStub).not.toHaveBeenCalled(); }); @@ -159,9 +159,9 @@ describe('CodeEditorMonacoComponent', () => { fixture.detectChanges(); comp.fileSession.set(presentFileSession); fixture.componentRef.setInput('selectedFile', fileToLoad.fileName); - await comp.selectFileInEditor(presentFileName, fileToLoad.fileName); + await comp.selectFileInEditor(fileToLoad.fileName); fixture.componentRef.setInput('selectedFile', presentFileName); - await comp.selectFileInEditor(fileToLoad.fileName, presentFileName); + await comp.selectFileInEditor(presentFileName); expect(loadFileFromRepositoryStub).toHaveBeenCalledExactlyOnceWith(fileToLoad.fileName); expect(comp.fileSession()).toEqual({ ...presentFileSession, @@ -195,7 +195,7 @@ describe('CodeEditorMonacoComponent', () => { }); fixture.detectChanges(); fixture.componentRef.setInput('selectedFile', fileName); - await comp.selectFileInEditor(undefined, fileName); + await comp.selectFileInEditor(fileName); expect(changeModelSpy).not.toHaveBeenCalled(); expect(comp.binaryFileSelected()).toBeTrue(); }); @@ -248,9 +248,9 @@ describe('CodeEditorMonacoComponent', () => { const longLoadingFileSubject = new Subject(); loadFileFromRepositoryStub.mockReturnValue(longLoadingFileSubject); // We do not await the promise here, as we want to simulate the user selecting another file while the first one is still loading. - const firstFileChange = comp.selectFileInEditor(undefined, 'file1'); + const firstFileChange = comp.selectFileInEditor('file1'); fixture.componentRef.setInput('selectedFile', 'file2'); - await comp.selectFileInEditor('file2', 'file2'); + await comp.selectFileInEditor('file2'); longLoadingFileSubject.next({ fileName: 'file1', fileContent: 'some code that took a while to retrieve' }); await firstFileChange; expect(changeModelSpy).toHaveBeenCalledExactlyOnceWith('file2', 'code2'); @@ -266,7 +266,7 @@ describe('CodeEditorMonacoComponent', () => { }; comp.fileSession.set(fileSession); fixture.componentRef.setInput('selectedFile', selectedFile); - await comp.selectFileInEditor(undefined, selectedFile); + await comp.selectFileInEditor(selectedFile); expect(setPositionStub).toHaveBeenCalledExactlyOnceWith(fileSession[selectedFile].cursor); expect(changeModelStub).toHaveBeenCalledExactlyOnceWith(selectedFile, fileSession[selectedFile].code); }); @@ -492,7 +492,7 @@ describe('CodeEditorMonacoComponent', () => { fixture.detectChanges(); comp.fileSession.set(fileSession); fixture.componentRef.setInput('selectedFile', scrolledFile); - await comp.selectFileInEditor(undefined, scrolledFile); + await comp.selectFileInEditor(scrolledFile); expect(setScrollTopStub).toHaveBeenCalledExactlyOnceWith(scrollTop); }); }); diff --git a/src/test/javascript/spec/integration/code-editor/code-editor-container.integration.spec.ts b/src/test/javascript/spec/integration/code-editor/code-editor-container.integration.spec.ts index 2c2d1ba569bf..5d02812f588b 100644 --- a/src/test/javascript/spec/integration/code-editor/code-editor-container.integration.spec.ts +++ b/src/test/javascript/spec/integration/code-editor/code-editor-container.integration.spec.ts @@ -258,7 +258,7 @@ describe('CodeEditorContainerIntegration', () => { const loadFile = async (fileName: string, fileContent: string) => { getFileStub.mockReturnValue(of({ fileContent })); container.fileBrowser.selectedFile = fileName; - await container.monacoEditor.selectFileInEditor(undefined, fileName); + await container.monacoEditor.selectFileInEditor(fileName); }; it('should initialize all components correctly if all server calls are successful', fakeAsync(() => {