-
Notifications
You must be signed in to change notification settings - Fork 217
Split Learning reset into exercise and unit #3635
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
7ee76aa
ea8912b
528285f
2c23117
c66ba6f
b699b29
a5ed8ea
6f6ad12
b65757e
8dd449e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -326,11 +326,81 @@ export class LearningTools { | |
| */ | ||
| async resetExercise(): Promise<StateSnapshot> { | ||
| await this.ensureInitialized(); | ||
| this.throwIfNotQSharpCourse(); | ||
| return this.invoke(async () => { | ||
| await this.service.resetExercise("chat"); | ||
| // Resolve the target from the editor — the selected cell for notebook | ||
| // courses — rather than the stored position, matching hint/solution. | ||
| // A destructive reset must never silently act on a different cell, so | ||
| // if a workbook is focused but its selected cell can't be identified, | ||
| // fail loudly instead of falling back to the stored position. | ||
| if (this.notebookSelectionUnidentified()) { | ||
| throw new CopilotToolError( | ||
| "I couldn't tell which cell is selected — it has no stable id yet. " + | ||
| "Click into the exercise cell you want to reset and try again, or reset the whole unit.", | ||
| ); | ||
| } | ||
| const state = this.serializeState(true); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Existing calls to |
||
| await this.service.resetExerciseAt(state.position.location, "chat"); | ||
| await this.showActivity(); | ||
| return { state: this.serializeState(false) }; // Q# only | ||
| return { state: this.serializeState(true) }; | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * True when the learner is on a course workbook but the selected cell has no | ||
| * stable id, so {@link serializeState} would fall back to the stored | ||
| * position — unsafe for reset. Judged from the active editor's URI so a | ||
| * different unit's workbook is still covered. | ||
| */ | ||
| private notebookSelectionUnidentified(): boolean { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure I understand why we needed a new method. Does this do something meaningfully different than |
||
| const editor = vscode.window.activeNotebookEditor; | ||
| if (!editor || !this.service.isCourseWorkbook(editor.notebook.uri)) { | ||
| return false; | ||
| } | ||
| const selection = editor.selections[0]; | ||
| if (!selection) { | ||
| return true; | ||
| } | ||
| const cellId = editor.notebook.cellAt(selection.start).metadata?.id; | ||
| return typeof cellId !== "string"; | ||
| } | ||
|
|
||
| /** | ||
| * Reset an entire unit, clearing completion for all of its activities. | ||
| * Defaults to the current unit. | ||
| */ | ||
| async resetUnit(input?: { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why parameterize it? Reset exercise targets the current exercise or nothing. |
||
| unitId?: string; | ||
| }): Promise<{ unitId: string; unitTitle: string } & StateSnapshot> { | ||
| await this.ensureInitialized(); | ||
| return this.invoke(async () => { | ||
| // With no explicit unit, resolve the target from the notebook the | ||
| // learner is viewing rather than the stored position: sync the position | ||
| // to the active workbook first, matching how the other tools resolve | ||
| // from the editor. Best-effort — with no workbook focused we fall back | ||
| // to the stored current unit. | ||
| if (!input?.unitId) { | ||
| const activeNotebook = vscode.window.activeNotebookEditor?.notebook.uri; | ||
| if (activeNotebook) { | ||
| await this.service.syncToWorkbook(activeNotebook); | ||
| } | ||
| } | ||
|
|
||
| // Unit reset is notebook-only; the service rejects Q# courses. | ||
| const { unitId, unitTitle } = await this.service.resetUnit( | ||
| { unitId: input?.unitId }, | ||
| "chat", | ||
| ); | ||
|
HABER7789 marked this conversation as resolved.
|
||
|
|
||
| // The reset closed the workbook and notebook courses don't use the | ||
| // lesson panel, so re-open the fresh copy. The open command resolves the | ||
| // notebook from the current position, so move there first — the reset | ||
| // unit isn't necessarily the one the learner was on. | ||
| await this.service.goTo({ unitId }, "chat"); | ||
| await vscode.commands.executeCommand( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can't just ask the service to open the notebook?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we even want to reopen the notebook if it wasn't the current unit? |
||
| "qsharp-vscode.learningOpenNotebook", | ||
| ); | ||
|
|
||
| return { unitId, unitTitle, state: this.serializeState(false) }; | ||
| }); | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this message is an explanation to copilot, rather than to the user. I'm also not sure what "no stable id yet" means. I might go with something closer to "the current activity isn't an exercise".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I'd probably eliminate this whole check and have resetExerciseAt throw if it's unhappy with the state.
this.invokeexists to translate service errors into copilot errors.