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

Generated code suggestion can't be applied to the file correctly #6171

Open
rebornix opened this issue Aug 27, 2024 · 7 comments
Open

Generated code suggestion can't be applied to the file correctly #6171

rebornix opened this issue Aug 27, 2024 · 7 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug
Milestone

Comments

@rebornix
Copy link
Member

Testing #6160

Initial code

preserveElements(indexes: number[]): void {
	this.preservedItems.clear();
	for (const index of indexes) {
		if (index >= 0 && index < this.items.length) {
			const item = this.items[index];
			this.preservedItems.add(item);
			if (!item.row) {
				this.insertItemInDOM(index);
			}
			this.updateItemInDOM(item, index);
		}
	}
}

new code

preserveElements(indexes: number[]): void {
	this.preservedItems.clear();
        let updatePreservedElements = false;
	for (const index of indexes) {
		if (index >= 0 && index < this.items.length) {
			const item = this.items[index];
			this.preservedItems.add(item);
			if (!item.row) {
				this.insertItemInDOM(index);
				updatePreservedElements = true;  
			}
			this.updateItemInDOM(item, index);
		}
	}

	if (updatePreservedElements) {  
            this.rerender();  
        }  
}

The generated code suggestion

        let updatePreservedElements = false;  
        for (const index of indexes) {  
            if (index >= 0 && index < this.items.length) {  
                const item = this.items[index];  
                this.preservedItems.add(item);  
                if (!item.row) {  
                    this.insertItemInDOM(index);  
                    updatePreservedElements = true;  
                }  
            }  
        }  

        if (updatePreservedElements) {  
            this.rerender();  
        }  

However when applied to the editor, the file was in a broken state

image
@alexr00
Copy link
Member

alexr00 commented Aug 28, 2024

This has to do with the re-positioning of comments when the file changes.

@alexr00 alexr00 added the bug Issue identified by VS Code Team member as probable bug label Aug 28, 2024
@alexr00 alexr00 added this to the August 2024 milestone Aug 28, 2024
alexr00 added a commit that referenced this issue Aug 28, 2024
alexr00 added a commit that referenced this issue Aug 28, 2024
@hediet hediet modified the milestones: August 2024, September 2024 Aug 30, 2024
@alexr00
Copy link
Member

alexr00 commented Aug 30, 2024

Here's one of the things that can happen:

  1. User opens a file and makes a comment on a line.
  2. The line position changes because of lines added before the comment. VS Code updates where the decoration is shown, but the line number of the comment in the comment API stays the same.
  3. At this point, everything looks visually correct, but the comment on the extension host no longer matches the line number that's used in the renderer.

@alexr00
Copy link
Member

alexr00 commented Aug 30, 2024

In VS Code core, we're going to need to propagate range changes back to the extension.

@alexr00
Copy link
Member

alexr00 commented Aug 30, 2024

Making this change could break extensions if they're using the VS Code CommentThread API as their data model. I'll need to discuss at the API sync. @chrmarti FYI.

@alexr00
Copy link
Member

alexr00 commented Aug 30, 2024

Thinking about it some more, I think each extension should handle this. Each extension might have a different idea of what file reversion is original. There is also a bug in VS Code that I'll fix, but otherwise, each extension should maintain a mapping of their comment's location to a location in a text document.

@chrmarti
Copy link
Contributor

chrmarti commented Sep 2, 2024

I think it would help extensions if VS Code just moved comments along as lines are added / removed. Are you saying you would need the previous revision to restore the correct position after the window reloads because the updated positions are not stored?

@alexr00
Copy link
Member

alexr00 commented Sep 2, 2024

I'm not sure it's possible for VS Code to know where the right position is.

  1. An extension knows that it should have a comment on line 3 based on revision A
  2. Some revisions happen to the file, and now locally we're on revision C.
  3. The extension knows what a revision means to it, and a revision could mean different things to different extensions (git commits, difference from file on disk, changes since the file was opened).
  4. Using this knowledge, when the file is opened in VS Code, the extension can map the position in revision A to the position in revision C. If this is the first time the file has been opened in VS Code with these comments, then VS Code has no understanding of how to map a position in A to a position in C because it doesn't understand what A and C are.

GHPR already does step 4, and presumably other comment providing extensions do the same.

What VS Code core should definitely do is move the comment based on active editing of the file. Ex., there's a comment on line 3 and you edit the file in VS Code and insert 2 lines above that: the comment should move to line 5.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue identified by VS Code Team member as probable bug
Projects
None yet
Development

No branches or pull requests

4 participants