Skip to content
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

Separate IModifiedFileEntry into text and notebook #234388

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

DonJayamanne
Copy link
Contributor

No description provided.

if (entry.kind === 'text' && snapshotEntry.kind === 'text') {
entry.restoreFromSnapshot(snapshotEntry);
} else if (entry.kind === 'notebook' && snapshotEntry.kind === 'notebook') {
throw new Error('Not implemented');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We now have a way to capture/restore snapshots for notebooks
And given we have interfaces for each, we can have separate concrete implementations for each

} satisfies ISnapshotEntry;
const deserializeSnapshotEntry = (entry: ISnapshotEntryDTO) => {
if (entry.kind === 'notebook') {
throw new Error('Not implemented');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do later, pending creation/restoring of snapshots for notebooks

@@ -491,7 +491,7 @@ class DiffHunkWidget implements IOverlayWidget {


constructor(
readonly entry: IModifiedFileEntry,
readonly entry: IModifiedTextFileEntry,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class only deals with editors for text files, editors for notebooks are already handled separately.

restoreFromSnapshot(snapshot: ITextSnapshotEntry): void;
}

export interface IModifiedNotebookFileEntry extends IModifiedAnyFileEntry {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separation of IModifiedFileEntry into text and notebook.
Similarly the snapshot interfaces have been separated, giving each their own implementations

@@ -86,7 +86,7 @@ class NotebookChatEditorController extends Disposable {
if (!model || !session) {
return;
}
return session.entries.read(r).find(e => isEqual(e.modifiedURI, model.uri));
return session.entries.read(r).find(e => isEqual(e.modifiedURI, model.uri) && e.kind === 'text') as IModifiedTextFileEntry | undefined;
Copy link
Contributor Author

@DonJayamanne DonJayamanne Nov 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notebooks today only deal with the textual entries.
Once we bake in full support for notebooks, this will go away

import { IChatService } from '../../common/chatService.js';
import { ChatEditingSnapshotTextModelContentProvider, ChatEditingTextModelContentProvider } from './chatEditingTextModelContentProviders.js';

export class ChatEditingModifiedFileEntry extends Disposable implements IModifiedFileEntry {

export class ChatEditingModifiedFileEntry extends Disposable implements IModifiedTextFileEntry {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The plan is to create a new ChatEditingModifiedNotebookFileEntry class with custom handling for notebooks
That will then be responsible for handling notebook edits and the like (however quite a lot will be shared between the two)

@@ -184,8 +184,6 @@ export class ChatEditorSaving extends Disposable implements IWorkbenchContributi
}

private _reportSaved(entry: IModifiedFileEntry) {
assertType(entry instanceof ChatEditingModifiedFileEntry);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This no longer applies, as we can have a separate implementation for notebooks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant