- Notebooks Can be Improperly Trusted After Save + Reopen
Fix was to only save if actually trusted.
Also fixed condition where it wasn't correctly skipping if in the queue
* Fix#3439 Trusted doesn't get saved in Notebooks
The main fix is to add a memento to store trust information. This is only needed for saved files - untitled files are always trusted as the user created them.
On clicking trusted or saving a file, the trusted state is cached. In the future, we will also handle code execution here too by sending notification on snapshot state.
I found issue #5506 during testing - existing issue where we should track trusted state changing on run. In the case all cells are ran, the whole notebook should become trusted.
Finally, I did a decent amount of refactoring to move more logic to the model - removing unnecessary calls from components which duplicated model behavior, moving trust notification to the model or at least the notebook service completely.
Added tests and logging for catch handling
VSCode serialization changed rejection handling to only serialize errors, which caused things to break
- Changed to return either account info or a cancel message in the resolve
- Rewrote to use promises. Tracking how to return canceled through 4+ thenables was way trickier than just using a promise
- Updated unit tests to handle new scenario
- Tested integration tests, realized they a) didn't run and b) didn't passed.
- Added vscode dev dependency to fix run issue
- Fixed tests to account for behavior changes in tree state.
- Copied over click handler from VSCode markdown renderer
- Added logic to only support command links if trusted
Now clicking on a link will do an action or open a file.
Created #5238 to track need to support relative links which doesn't work
This fixes an issue where if you copy azdata.proposed.d.ts to
an extension project, it would fail to compile using default
tsconfig.json settings due to missing return type
* Touchbar icon support in notebooks
- updated shortcut keys to only work if notebook is active
- Added icons
- Now have 1 "add cell" icon that prompts for code/text.
This is useful as there wasn't an icon to differentiate
* Fix 2 notebook issues
- Do not create notebook model twice on start
- Do not cause disposed warnings due to markdown cell deserialization
* Fix notebook dirty on open issue
Before model is resolved we weren't getting dirty events.
Solution is to use backing text model until it's ready.
Must hook to the dirty event & notify to get the dot to appear
* Basic touchbar support
* Touchbar support for Run Cell, Run Query, Add Cell
- Add top 3 notebook commands
- Add top query command
Actions only appear on having active editor of expected type.
In order to make Notebook work as expected, added tabindex to support focusing
and hide outline to ensure it doesn't get weird blue outline on click
Note: does not have icons yet, which would be nice.
However can add in subsequent PR once this comes from UX.
This is a partial fix that lays groundwork for full "Prompt to connect" if a kernel needs a connection.
I am waiting on Yurong's refactoring of connection handling before doing any of the prompt work.
- Adds kernel metadata about whether a connection is required.
- For Jupyter, only Spark kernels are listed as requiring a connection
- If this is true and there's no active connection, will show notification and not call execute
In the future, this path will still be used if user is prompted to connect and cancels out.
The future change will be to inject a "connect" handler from notebook.component to the cell callback and use to set connection context
Add back check for textDocuments with same name, should've been there anyhow
On rehydration files show as text docs before clicking as only get
changed by customInputConverter code path.
We should look at this long term - ideally we'd update notebookDocuments
with correct values on initial start. #4958 opened to track this.
* FIX#4513 Notebook stuck at changing kernel
- Intra-provider kernel change didn't happen because we only tried changing kernel on new session creation.
- Inverted the logic (e.g. did the right thing) and renamed the method so it's clearer what we're doing & what the boolean value should be
- Manually tested all the known scenarios
* Fix#4505 Notebooks: New Notebook will not work if existing untitled notebooks are rehydrated
* Also fixes#4508
* Unify behavior across New Notebook entry points
- Use Notebook-{n} as the standard in both entry points
- Use SQL as default provider in both
- Ensure both check for other names and only use free number
* Fix#4029 Ensure changeKernels always resolves, even in error states
This is necessary to unblock reverting the kernel on canceling Python install
- startSession now correctly sets up kernel information, since a kernel is loaded there.
- Remove call to change kernel on session initialize. This isn't needed due to refactor
- Handle kernel change failure by attempting to fall back to old kernel
- ExtensionHost $startNewSession now ensures errors are sent across the wire.
- Update AttachTo and Kernel dropdowns so they handle kernel being available. This is needed since other changes mean the session is likely ready before these get going
* Fix to handle python cancel and load existing scenarios
- Made changes to handle failure flow when Python dialog is canceled
- Made changes to handle initial load fail. Kernel and Attach To dropdowns show No Kernel / None and you can choose a kernel
- Added error wrapping in ext host so that string errors make it across and aren't lost.
* Fix#4452 Notebook is reloaded with wrong kernel
- Await all extension registration before getting providers and providerId
To do this, we need to await way up in the NotebookInput, and promise the model that we'll have values eventually
- Added to both built-in themes. Long term would be good to contribute this back to Seti so it works without a carbon edit tag.
- Issue #4418 tracks remaining problem where for new notebooks it's not set initially. This is blocked by Raj's work so will update once that lands.
* Fix#4356 New Notebook from connection doesn't connect
Fix new notebook error by passing profile instead of ID.
- I could've just sent the ID over, but this fix sets the stage for disconnected connections to work (since we have enough info to properly connect).
- There's a bug in NotebookModel blocking the disconnected connection part working, but Yurong's in progress fixes will unblock this. Hence checking in as-is and working to properly unblock once that's in.
* Support connection profile in commandline service
- Added new context API for things that want to work on commandline and object explorer
- Refactored commandlineservice slightly to be async & have a simpler execution flow (far fewer if/else statements)
* Fix unit tests
- Fixed 2 issues raised by tests (sholdn't do new query if no profile passed, shouldn't error on new query failing)
- Updated unit tests to pass as expected given changes to the APIs.
The Attach To was showing only localhost for SQL, since we overrode the standard kernels from SQL with the ones from Jupyter.
Fix is to save all standard kernels.
Also, added dispose handling for some events I found during debugging and removed unused imports
- Upate vscode-nls to 4.0.0 in notebook extension. Should be fix for Insiders build failure due to localization package failing
- Updated samples to remove as vscode-nls if not needed, or update if still needed
- Updated samples using `npm audit fix`
- Fixed compile errors in all the samples
- Fixes#4129 Overlapping command help windows in notebook
- Do not show parameter hints for inactive cells, to avoid them hanging around when no longer selected
- Fixes#4116 Notebooks: Intellisense Doesn't Work using Add New Connection
- Move connect/disconnect logic to 1 place (code component) instead of 2
- Handle the case where you connect after choosing active cell. We now hook to the event and update connection
- Fix issues in sql session manager where result outputs 0 rows. This was failing to show the empty resultset contents, which is a regression vs. query editor. It also put unhandled error on the debug console
- Fix#3913 Notebook: words selected in other cells should be unselected on cell change
Note: after fix, now looks as follows. Need to do follow up to get correct grid min height
- Jupyter completion item support was awaiting info before responding. Fix is to check if this is even a notebook cell first, then only await stuff if that's true
* Move to using notebook language by default, with override in cell
* Update cell language on kernel change
* Tweak language logic so that it prefers code mirror mode, then falls back since this was failing some notebooks
* Add new package.json contribution to define language magics. These result in cell language changing. Language is cleared out on removing the language magic
* Added support for executing Python, R and Java in the SQL Kernel to prove this out. It converts to the sp_execute_external_script format
TODO in future PR:
* Need to hook up completion item support for magics (issue #4078)
* Should add indicator at the bottom of a cell when an alternate language has been detected (issue #4079)
* On executing Python, R or Java, should add some output showing the generated code (issue #4080)
* Updated version
* On building found it had build break due to unused imports. Turns out none of the code is used (it's just a package.json + SQL file extension) so removed it all
* Removed all unnecessary node module imports, reducing size from 5.4MB to 86KB. We should probably do this for all extensions
* Fix error where kernel name was compared to itself. This doesn't break anything right now since we happen to have special handling of Python3, and for other kernels they share the same set of supported providers (which is what the check is used for). However it needs fixing for next release.
* Fix console error due to queryTextEditor trying to access model before it's ready
* Fix issues where notebook model hooked to session events multiple times
* Removed calls that weren't needed such as loadActiveContexts (if undefined, does nothing) and passing connection to initialize method
- Fix#3844
- Fix#3955
- Specs can be null on early load of Jupyter kernels
- Cells were missing in some reference test .ipynb files. We should be resilient to malformed files if possible.
Fixes#3843. Now includes full fix which limits length and ensures a scrollbar is available
- Set max size for editor. 4000px gets us 200-250 lines before needing a scrollbar.
- Adds layout updating which should also ensure accurage line highlighting to the right of the editor. What's happening is initial size is slightly off, so need to layout a 2nd time (e.g. layout once, let flex figure things out, then layout a 2nd time). This isn't optimal as there's a minor perf hit but it isn't noticeable overall.
To consider in future PRs:
- Add user configurable setting for max length?
- Handle case where we scroll to bottom but scrollbar is at the top.
- Consider how intellisense will work on this. We may need to split into a window around the current code when sending to the kernel as it's quite likely that doing a 12K line intellisense request will be too big.
* Fix#3985 Hide cell toolbar for markdown cells
* Note that I'm still hiding the overall toolbar section per UX feedback
* Also now hiding line numbers per UX feedback..
- This is a recommended update, let me know if you have concerns
- Updated all samples and Azure Data Studio specific extensions with lodash dependency
Fixes#3931
- Align run button correctly so it's centered in new cell
- Refactor to support multi-state button.
- Hidden state is set to show execution count
- Stopped state shows run button
- Running state shows stop button
- Error state (will) show error button. This isn't fully handled right now
- Add execution count to model and to SqlKernel, verify serialization, loading, update matches other notebook viewers
**Notes on implementation**:
I think this is a decent solution for a) showing execution count as text, and b) perfectly centering the run button.
The below solution shows count correctly up to 999 runs (that’s clicking 999 times in a single session), the icon lines up juuust about right with [ ] but for other numbers it is pretty close but probably not exactly right. I wish I could solve this to work better but trying to change text float to center etc. really isn’t working.
**Screenshots**:
With running cell:
- Handles empty file scenario, with fixes along the way for missing metadata (bonus win)
- In non-empty file still shows error and kernel stuck in loading state. #3964 opened to track this issue and fix later
- Fixed so it's now invisible instead of empty when not selected.
- This fixes clickability and issue where it stayed visible in 1 fix
- Also fixed cell output action which used active cell instead of context cell.
This was reviewed / worked on with Smitha and will be signed off on by PM via mail.
1 thing left (make run button look better when not selected) will be one in separate review.
Changes
- Add top/bottom padding to editor so it's not cramped
- Added an (on by default) setting `notebook.overrideEditorTheming`. This controls whether new colors etc. are used for notebook editors or if users should see vanilla UI like in standard editor. Settings under this flag are:
- When unselected, editor has same color as toolbar. On selection it goes back to regular editor view so colors work "right"
- In standard light/dark themes we now use a filled in background color instead of border box.
* Added hover support, adding box shadow and light outline on hovering and the "more actions" button showing on hover
* Added box shadow for dark themes (hooray!)
* Remove border from everything but the code cell unless a cell is selected or hovered over. This ensures this looks like a document
* Fix high contrast theming issues.
- SQLKernel is the only place to listen for batch and query complete messages now
- It routes to the 1 and only future (since can only have 1 at a time
- It handles query cancelation and not-connected issues correctly
- Editor layout gets called sometimes when other events happen (and Notebook isn't visible)
- Add in a layout call on re-setting input so the cell is updated. This fixes the problem by laying out once the UI is visible again.
Note: long term, should really be destroying the UI (while preserving the model), then restoring it including scroll selection etc. and hooking back up to the model. That is... much more work, but something we'll need long term to avoid issues where we have many Notebooks open at once. Not in scope for this PR
- Toolbar background is now differentiated from the editor
- For unselected cells there's no longer a line selection in the cell. This makes it clearer what the active cell is (and cleans the UI up)
- Added `runCell` API. Updated runCell button to listen to events on the model so it'll reflect run cell when called from other sources
- Plumbed through kernelspec info to the extension side so when changed, it's updated
- Fixed bug in ConnectionProfile where it didn't copy from options but instead overrode with empty wrapper functions
Here's the rough test code (it's in the sql-vnext extension and will be out in a separate PR)
```ts
it('Should connect to local notebook server with result 2', async function() {
this.timeout(60000);
let pythonNotebook = Object.assign({}, expectedNotebookContent, { metadata: { kernelspec: { name: "python3", display_name: "Python 3" }}});
let uri = writeNotebookToFile(pythonNotebook);
await ensureJupyterInstalled();
let notebook = await sqlops.nb.showNotebookDocument(uri);
should(notebook.document.cells).have.length(1);
let ran = await notebook.runCell(notebook.document.cells[0]);
should(ran).be.true('Notebook runCell failed');
let cellOutputs = notebook.document.cells[0].contents.outputs;
should(cellOutputs).have.length(1);
let result = (<sqlops.nb.IExecuteResult>cellOutputs[0]).data['text/plain'];
should(result).equal('2');
try {
// TODO support closing the editor. Right now this prompts and there's no override for this. Need to fix in core
// Close the editor using the recommended vscode API
//await vscode.commands.executeCommand('workbench.action.closeActiveEditor');
}
catch (e) {}
});
it('Should connect to remote spark server with result 2', async function() {
this.timeout(240000);
let uri = writeNotebookToFile(expectedNotebookContent);
await ensureJupyterInstalled();
// Given a connection to a server exists
let connectionId = await connectToSparkIntegrationServer();
// When I open a Spark notebook and run the cell
let notebook = await sqlops.nb.showNotebookDocument(uri, {
connectionId: connectionId
});
should(notebook.document.cells).have.length(1);
let ran = await notebook.runCell(notebook.document.cells[0]);
should(ran).be.true('Notebook runCell failed');
// Then I expect to get the output result of 1+1, executed remotely against the Spark endpoint
let cellOutputs = notebook.document.cells[0].contents.outputs;
should(cellOutputs).have.length(4);
let sparkResult = (<sqlops.nb.IStreamResult>cellOutputs[3]).text;
should(sparkResult).equal('2');
try {
// TODO support closing the editor. Right now this prompts and there's no override for this. Need to fix in core
// Close the editor using the recommended vscode API
//await vscode.commands.executeCommand('workbench.action.closeActiveEditor');
}
catch (e) {}
});
});
```
* Refactor JSON and format files to model and fix tabs -> spaces issues
This is in prep for some work to reuse these code files inside the model,
so pushing as its own PR to keep the next piece of work clean.
* Fix#3680 Notebooks: outputs with string arrays rendered incorrectly
- Add support for processing v4 format files loaded from disk
- Prep support for v3 notebooks by adding placeholder code for that
* Fix failing tests and add specific one for this bug
* Remove references to v5
* Handle delayed Notebook provider registration
- Fixes#3197 Notebooks: builtin provider always used on reopen with notebook file visible
- Fixes#3414 Can't refresh kernel after connect to big data cluster
There are 3 parts to this fix:
- If no notebook provider other than the default is installed, we warn users and prompt to install the SQL2019 extension
- We wait on the extension host registration to complete before determining which provider to use
- We know that the extension registration of the provider instance will be after package.json is read, so if we wait after registration for 10 seconds to give this a chance to happen before returning a provider to the front end
* Remove launch.json change that was added accidentally
* Fix timeout not being the expected value
* Removed console log left in during debugging
* Remove unnecessary whitespace
* Fix unit test failure
* Name the registration better, and remove outdated comments
- Set markdown as language for markdown cell
- Fix issue where after loading language from cell metadata, always override it. This made the "language" feature irrelevant in the cell.
- Fixed tests with new behavior (assumption: cell-level language overrides notebook-level definition) and added new test to cover this too
- Add edit API that can be used in the extension
- Separated document and editor classes out since this is the point those get big. I can refactor back in if needed to ease code review
- Based this off text editing APIs but tweaked for the fact this is a cell/array based set of edits
initial support for Notebook extensibility. Fixes#3148 , Fixes#3382.
## Design notes
The extensibility patterns are modeled after the VSCode Document and Editor APIs but need to be different since core editor concepts are different - for example Notebooks have cells, and cells have contents rather than editors which have text lines.
Most importantly, a lot of the code is based on the MainThreadDocumentsAndEditors class, with some related classes (the MainThreadDocuments, and MainThreadEditors) brought in too. Given our current limitations I felt moving to add 3 full sets of extension host API classes was overkill so am currently using one. Will see if we need to change this in the future based on what we add in the additional APIs
## Limitations
The current implementation is limited to visible editors, rather than all documents in the workspace. We are not following the `openDocument` -> `showDocument` pattern, but instead just supporting `showDocument` directly.
## Changes in this PR
- Renamed existing APIs to make clear that they were about notebook contents, not about notebook behavior
- Added new APIs for querying notebook documents and editors
- Added new API for opening a notebook
- Moved `New Notebook` command to an extension, and added an `Open Notebook` command too
- Moved notebook feature flag to the extension
## Not covered in this PR
- Need to actually implement support for defining the provider and connection IDs for a notebook. this will be important to support New Notebook from a big data connection in Object Explorer
- Need to add APIs for adding cells, to support
- Need to implement the metadata for getting full notebook contents. I've only implemented to key APIs needed to make this all work.
Full plumb through of Session support. Also fixed some test issues
- Load session and get necessary information in kernels list
- Run Cell button now works as expected
- Added a ToggleAction base class which can be used for anything that switches icons. I'd still prefer to have this be dynamic and as clean as the extension classes
- Fixed account test unhandled promise rejections (caused by incorrect / invalid tests) that made it hard to see all the test run output.
* Support notebook file types contribution
- Extensions can define a provider and what file types it should be used for
- Verified that this works for Jupyter Content & Server Managers.
- Starts Jupyter server as expected
Not in this PR:
- Support for session manager end to end
- Tests
Implements provider contribution in the MainThreadNotebook, with matching function calls in the ExtHostNotebook class. This will allow us to proxy through notebook providers (specifically, creation of a notebook manager with required content, server managers) from an extension up through to the main process.
Implemented in this PR:
- Callthroughs for content and server manager APIs
- Very basic unit tests covering provider & manager registration
Not implemented:
- Fuller unit tests on the specific callthrough methods for content & server manager.
- Contribution point needed to test this (so we can actually pass through the extension's existing Notebook implementation)
- Fixed issues with sqlCreateTable snippet, which meant it couldn't effectively be used to tab through all fields and then hit execute without errors. Specifically fixed bugs where types like NVARCHAR were incorrectly escaped and the Id part of a column was outside the name section, both causing intellisense & execution breaks
- Changed SchemaName to dbo. This helps hit the "80% case" where objects are in the most common schema
- Removed DatabaseName from most snippets. The core issue is this requires you to manually type the exact database name into the snippet which is really hard. We should consider having separate "with Database" snippets or support SQLCMD variables which would let us default to the current database without needing users to manually type them in as alternatives, but having basic snippets just work on current DB is important.
- Defines a new NotebookService in Azure Data Studio which will be used to interact with notebooks. Since notebooks can require per-file instantiation the provider is just used to create & track managers for a given URI.
- Inject this into notebook.component.ts and pass required parameters that'll be used to properly initialize a manger into the method. Actual initialization not done yet.
- Port over & recompile notebook model code
- Define most required APIs in sqlops.proposed.d.ts. In the future, these will be used by extensions to contribute their own providers.
* Port the Azure Resource Explorer extension to core.
This will enable Azure viewlet by default in the next release.
- Moving this code from the SQL Server 2019 extension to Azure Data Studio core
- Ported tests and verified they work in the integration tests.sh file
- Fixed an issue that caused integration tests to fail if you have a SQL Server 2019 big data cluster endpoint listed, but the extension isn't installed.
The IdGenerator was recreated each time and had a high likelihood of conflicts. Invitably after adding dozens or hundreds of icons you'll start seeing the CSS class replaced and overridden.
The solution is to do like elsewhere: have 1 const that is loaded on first import of the file and keeps a global track.
Side note is that it'd be a good idea to cache CSS rules with the same iconPath so we don't create lots of additional rules unnecessarily. If we reuse the same icon a bunch we should cache them - #2524 is tracking this.
* Add support for clickable links and other webview options
- Added click handling
- Added suport for localResourceRoots to be read in the webview
* Options should not be mandatory
* Ensure the constructor-defined properties are preserved
- This fixes issue where the extensionFolderPath was lost during webview withProperties call in the modelbuilder.
* enableCommandUris should be a getter
* Add position support to webview and to flexContainer
* Fix regressions on editor view caused by merge
* Support editor content change notification, URI property and correctly align toolbar items
* Better fix for toolbar orientation, plus minor editor fixes
* Support position property in editor component
- This needs to be set directly on the editor component so cannot just use CSSStyles feature
- Given its importance to this scenario, it also warrants a dedicated property.
* Fix window resize when action bar clicked
* Renamed per Abbie's suggestion
* Changed name to clarify the comments
* Fixes#1856 Object Explorer needs Icons field for nodes separate from type/subtype
- Adds in the concept of a themeable icon path which matches VSCode's implementation. This should help support theme-based overrides in the future
- Fixes#1867 context menu should be extensible
- Added context keys to support "when" conditions on the new extensions
- Fixes issue where actions like New Query, scripting show up even if these are not valid for the provider type or object type
- Fixed node expansion bug where rapid connect / expand / disconnect could break the app (fix in ObjectExplorerService.onNodeExpanded)
- Major change to how internal actions work. These cannot assume the context has non-serializable objects. Opened up some APIs to make this easier to handle.
- Fixed a number of existing bugs in internal actions.
- Notably, DisconnectAction was adding a listener on each right-click on an active connection and never getting it disposed. This wasn't needed at all due to design changes.
- Another bug fix is that the Manage action now correctly navigates to the DB dashboard for database-level connections. Before this it went to the server-level dashboard.
* Define API for context info
Kevin Cunnane
