Implement base custom editor support #9812
Conversation
|
Still have to do undo/redo and more manual testing But opening notebooks and creating blank ones should work. |
package.json
Outdated
| "webviewEditors": [ | ||
| { | ||
| "viewType": "NativeEditorProvider.ipynb", | ||
| "displayName": "Jupyter Notebook Editor", |
There was a problem hiding this comment.
Do we need the Editor part?
I think we should drop that. #Resolved
There was a problem hiding this comment.
| /** | ||
| * Opens a file with a custom editor | ||
| */ | ||
| openEditor(file: Uri): Thenable<void | undefined>; |
There was a problem hiding this comment.
Why not Promise<void> Why void | undefined?
| openEditor(file: Uri): Thenable<void | undefined>; | |
| openEditor(file: Uri): Promise<void>; | |
| ``` #Resolved |
There was a problem hiding this comment.
Because it maps internally to calling something else that returns that.
In reply to: 372156630 [](ancestors = 372156630)
There was a problem hiding this comment.
Actually I can just await inside. You're right, it makes more sense to return a promise.
In reply to: 372479925 [](ancestors = 372479925,372156630)
| } | ||
|
|
||
| // Set these contents into the storage before the file opens | ||
| await this.getStorage(uri, contents); |
There was a problem hiding this comment.
Probably good to rename the method from getStorage to something else...,
We're performing a get operation however we're expecting a side effect (implied by the words Set these contents....).
Hence the get is not really a get, but does more... #Resolved
There was a problem hiding this comment.
src/client/datascience/types.ts
Outdated
|
|
||
| export const ILoadableNotebookStorage = Symbol('ILoadableNotebookStorage'); | ||
|
|
||
| export interface ILoadableNotebookStorage extends INotebookStorage { |
There was a problem hiding this comment.
Shouldn't these two be separate interfaces and let a class implement both if required?
I.e. one that provides the operations to read/write from a file/disc and the other that provides operations to manage the content (INotebookStorage).
E.g.
interface I..... {
load(file: Uri): Promise<INotebookStorage>;
save(storage: INotebookStorage, file: Uri): Promise<void>;
saveAs(storage: INotebookStorage): Promise<void>;
}
``` #ResolvedThere was a problem hiding this comment.
Separating into two interfaces make make help get rid of the promise to in getCells, getContents and getJson, as INotebookStorage would have been resolved.
#Resolved
There was a problem hiding this comment.
I don't think the promises would go away. Contents and json are after the fact. Load does not determine them.
Have to think about separating them. Maybe two classes even. NotebookStorage and NotebookModel
In reply to: 372520118 [](ancestors = 372520118)
There was a problem hiding this comment.
Although the NotebookModel couldn't be 'loaded' from the service container because it would need a 'load' model then too.
In reply to: 372522427 [](ancestors = 372522427,372520118)
There was a problem hiding this comment.
I think I'll keep them the same for now.
In reply to: 372522852 [](ancestors = 372522852,372522427,372520118)
There was a problem hiding this comment.
That sounds more reasonable. I'll see about decoupling the interfaces.
In reply to: 372525975 [](ancestors = 372525975)
There was a problem hiding this comment.
Isn't that the case only because we're trying to save the notebook with the updated metadata information?
I.e. if we had the metadata loaded, then getJson would be sync, hence save would also be sync (in the sense we won't need to await on anything for save to complete).
This is something we can address later. #WontFix
There was a problem hiding this comment.
Though I'd still like to see two separate interfaces, even if the same class implements the two. #Resolved
There was a problem hiding this comment.
Yes, the metadata is async because it updates when we run a cell.
In reply to: 372531329 [](ancestors = 372531329)
There was a problem hiding this comment.
If a cell isn't run, then we need to look for the usable jupyter and use that as the kernel information.
In reply to: 372537166 [](ancestors = 372537166,372531329)
|
|
||
| export interface INotebookStorage { | ||
| readonly file: Uri; | ||
| readonly isDirty: boolean; |
There was a problem hiding this comment.
Probably best to rename this to INotebookContents or INotebookModel
This doesn't deal with any storage, but the raw contents of the ipynb file. #Resolved
There was a problem hiding this comment.
| disposableRegistry.push(commandManager.registerCommand(Commands.NotebookStorage_ModifyCells, NativeEditorStorage.handleModifyCells)); | ||
| disposableRegistry.push(commandManager.registerCommand(Commands.NotebookStorage_RemoveCell, NativeEditorStorage.handleRemoveCell)); | ||
| disposableRegistry.push(commandManager.registerCommand(Commands.NotebookStorage_SwapCells, NativeEditorStorage.handleSwapCells)); | ||
| disposableRegistry.push(commandManager.registerCommand(Commands.NotebookStorage_UpdateVersion, NativeEditorStorage.handleUpdateVersionInfo)); |
There was a problem hiding this comment.
We could instead have a wrapper handler (handler that handles the command, checks if there's a notebook and if there is then call the callback with that notebook)
E.g.
type PromiseFunctionWithFirstArgOfResource = (...any: [Uri | undefined, ...any[]]) => Promise<any>;
async function callback(handler, resource) {
const args = Array.prototype.slice.call(arguments).slice(1);
const storage = await NativeEditorStorage.getStorage(resource);
if (storage) {
handler(storage, ...args);
}
}
commandManager.registerCommand(....InsiderCell, callback.bind(undefined, NativeEditorStorage.handleDeleteAllCells);
// finally
private static handleDeleteAllCells(storage: INotebookStorage, resource: Uri): Promise<void> {
``` #ResolvedThere was a problem hiding this comment.
Sure that's what I was essentially doing, but then you just said callbacks were bad.
In reply to: 372513072 [](ancestors = 372513072)
There was a problem hiding this comment.
It avoids the callback in the handler and makes that cleaner.
But thats only a suggestion, no need to change. Personally I'd prefer to avoid callbacks. #Resolved
src/client/datascience/interactive-ipynb/nativeEditorStorage.ts
Outdated
Show resolved
Hide resolved
| @@ -62,6 +62,17 @@ export namespace Commands { | |||
| export const ScrollToCell = 'python.datascience.scrolltocell'; | |||
| export const CreateNewNotebook = 'python.datascience.createnewnotebook'; | |||
| export const ViewJupyterOutput = 'python.datascience.viewJupyterOutput'; | |||
|
|
|||
| // Make sure to put these into the package .json | |||
There was a problem hiding this comment.
Does this need to happen now? #WontFix
There was a problem hiding this comment.
There was a problem hiding this comment.
Users would need to run VSCode with a special CLI argument as well. At least that's what I thought. #ByDesign
src/client/datascience/interactive-ipynb/nativeEditorStorage.ts
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## ds/custom_editor #9812 +/- ##
====================================================
- Coverage 60.58% 60.35% -0.23%
====================================================
Files 552 551 -1
Lines 29852 29536 -316
Branches 4527 4451 -76
====================================================
- Hits 18086 17827 -259
+ Misses 10748 10717 -31
+ Partials 1018 992 -26
Continue to review full report at Codecov.
|
|
Kudos, SonarCloud Quality Gate passed!
|
For #9255 and #9514
First step in implementing the custom editor support.
Split the nativeEditor into a View model (nativeEditor.ts) and a model (nativeEditorStorage.ts).
Communication between them now uses custom commands.
Fixes #9510, #9511, #9512, #9515, #9513