Introduce Trust Book in Book Viewlet #9414
Conversation
…hub.com/microsoft/azuredatastudio into dev/joberume/trust-notebook-directories
Duplicate NotebookRange variable
joberume
changed the title
src/sql/workbench/api/browser/mainThreadNotebookDocumentsAndEditors.ts
Outdated
Show resolved
Hide resolved
|
You also have a merge conflict so make sure you're merging in the latest changes from master |
| * Determines whether a notebook is part of this book. | ||
| * @param notebookUri Notebook uri Full URI of the notebook. | ||
| */ | ||
| public hasNotebook(notebookUri: string): boolean { |
There was a problem hiding this comment.
I don't think I agree that this is the best place for this. The tree item should be just about the actual tree itself and not necessarily know anything about the contents of the book (only that it may have child book tree items)
Also we already have a getNotebook method on that model. Why can't you just use that? https://github.com/microsoft/azuredatastudio/blob/master/extensions/notebook/src/book/bookModel.ts#L50
There was a problem hiding this comment.
Some issues I'm seeing :
- The trust isn't set immediately upon opening and when it is set it doesn't update the toolbar so it still shows untrusted. If you close and reopen the notebook it'll show correctly though
- There seems to be a lot of issues around getting the trust to persist correctly. I'm seeing a lot of instances where I've trusted a book but the notebooks under it are still showing as untrusted.
So those are obviously big issues. But at this point I think that aside from a few more clean up items I think we should just get this merged in and then your team can start fully testing it and fixing things one by one. That'll make it a lot easier to track and review the changes. This PR is too overloaded at this point and it's getting hard to review and track all the issues.
@chlafreniere You good with that?
| @@ -41,6 +44,7 @@ export class BookTreeViewProvider implements vscode.TreeDataProvider<BookTreeIte | |||
| this.initialize(workspaceFolders).catch(e => console.error(e)); | |||
| this.viewId = view; | |||
| this.prompter = new CodeAdapter(); | |||
| this._bookTrustManager = new BookTrustManager(this.books, apiWrapper); | |||
| this._apiWrapper = apiWrapper ? apiWrapper : new ApiWrapper(); | |||
There was a problem hiding this comment.
While you're here please remove the _apiWrapper var and instead just use the one passed in directly
| if (bookPathToTrust) { | ||
| let trustChanged = this._bookTrustManager.setBookAsTrusted(bookPathToTrust); | ||
|
|
||
| if (this._apiWrapper) { |
There was a problem hiding this comment.
Get rid of this check once you've removed the _apiWrapper var
| * @param isTrusted True if the notebook is to be trusted, false otherwise. | ||
| */ | ||
| async setTrusted(notebookUri: URI, isTrusted: boolean): Promise<boolean> { | ||
| if (isTrusted) { |
There was a problem hiding this comment.
I'd rather we get rid of all untrusted logic if we're not going to support that. You mentioned that you need it for testing - could you point out where that is? There should be some other way to achieve that functionality (such as clearing the config or something) without needing to use this.
Investigated this. The path for updating the trust notebook state via Mementos only reflected changes after the notebook is opened. Fixed this by updating the live notebook as well. |
joberume
requested review from
chlafreniere
and removed request for
chlafreniere
Can you clarify what the remaining known issues are? |
|
Please be sure to merge latest from master; then the component detection issues will go away and builds will succeed. |
I believe issue #1 and #2 are similar. Was not able to repro this. Need to talk to get Book. |
This change introduces a "Trust Book" function that trusts all notebooks in a Book. If a new notebook is introduced after having trusted the book, and it is not added to the table of contents of that book, then that trust will not be extended to that notebook (no inadvertent permission granting).
This PR fixes #8864