Virtual concat document with extra lines #25
Conversation
…nt for changes at the end
|
Hopefully you can merge this change into your version. |
|
thank you! |
| } | ||
| return symbols ?? undefined; | ||
| } | ||
|
|
||
| public toIncomingWorkspaceEdit(workspaceEdit: WorkspaceEdit | null | undefined): WorkspaceEdit | undefined { |
There was a problem hiding this comment.
A lot of the change is changing 'toIncoming' to 'toNotebook' and 'toOutgoing' to 'toConcat'. I kept forgetting which was 'outgoing' and which was 'incoming'
There was a problem hiding this comment.
Smart, feels much clearer to me this way.
| // Diff should have an array of numbers. These are | ||
| // offsets in the old text translating to offsets in the new text | ||
| if (diff.length > 0) { | ||
| const oldTextStart = diff[0][0]; |
There was a problem hiding this comment.
Is there a way to maybe document what the fastMyersDiff returns? Even with the comment I'm not fully sure on what our diff indexes are here.
There was a problem hiding this comment.
Like having this in from the documentation on npm "it produces a sequence of quadruples [sx, ex, sy, ey], where [sx, ex) indicates a range to delete from xs and [sy, ey) indicates a range from ys to replace the deleted material with. Simple deletions are indicated when sy === ey and simple insertions when sx === ex"
There was a problem hiding this comment.
It does have docs? Do you mean put a link to the docs here? Otherwise the comment would be rather large.
There was a problem hiding this comment.
I put more comments and a link.
There was a problem hiding this comment.
Thanks that helps. It was reading a bit funny to me since it was looking at the individual diffs all as one big diff here (taking the start of the first diff to the end of the last diff) but that works for what this is doing.
src/notebookConcatDocument.ts
Outdated
| public realRangeOf(cellUri: vscode.Uri) { | ||
| // Get all the real spans | ||
| const realSpans = this._spans.filter((s) => s.uri.toString() == cellUri.toString() && s.inRealCell); | ||
| const startOffset = realSpans[0].startOffset | 0; |
There was a problem hiding this comment.
Do you want logical OR here?
| public locationAt(positionOrRange: vscode.Range | vscode.Position): vscode.Location { | ||
|
|
||
| public notebookLocationAt(positionOrRange: vscode.Range | vscode.Position): vscode.Location { | ||
| // positionOrRange should be in concat ranges |
There was a problem hiding this comment.
Minor suggestion:
const range = (positionOrRange instanceof vscode.Position) ? new vscode.Range(positionOrRange, positionOrRange) : positionOrRange;
Maybe pedantic, but then you could use range for the rest of the function instead of positionOrRange when you are assuring that you have a Range here.
There was a problem hiding this comment.
Yes that's clearer, will fix
| ): NotebookSpan { | ||
| // Compute fragment based on cell uri | ||
| const fragment = | ||
| cellUri.scheme === InteractiveInputScheme ? -1 : parseInt(cellUri.fragment.substring(2) || '0'); |
There was a problem hiding this comment.
Doesn't the fallback need to be 0 not '0'?
There was a problem hiding this comment.
No actually it's inside the parse. So it has to parse the '0'
There was a problem hiding this comment.
Whoops totally missed that. It just looked odd, I see it now.
This change is a new way to handle magics in the concat document. This is a prerequisite to solving bugs like:
microsoft/vscode-jupyter#6987
microsoft/vscode-jupyter#1510
Essentially there are two parallel documents - the real text entered by the user, and the text we send to pylance.
These are tracked with a series of spans. Spans are in order from top of notebook to bottom with each span tracking:
These spans are then used to translate edits between the two different variations of the text.
Edits were particularly difficult because they don't map to the edits in the original (real) text. I used something called fast myers diff to compute new offsets.