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

Wrong cursor position after using same file in two panels #2688

Closed
lukaszb opened this issue May 28, 2018 · 19 comments
Closed

Wrong cursor position after using same file in two panels #2688

lukaszb opened this issue May 28, 2018 · 19 comments

Comments

@lukaszb
Copy link

lukaszb commented May 28, 2018

Is this a BUG REPORT or FEATURE REQUEST? (choose one):

It's a bug report

What happened:

  1. Open same file in two panels (i.e. side by side) with vim mode enabled
  2. At one buffer (A) jump to line 10
  3. At second buffer (B) click at line 20
  4. Get back to buffer A (at this point we are at line 10) and hit j (for vim "down by line" move)
  5. I am taken to line 21

What did you expect to happen:

At point 5 I should be taken to line 11.

How to reproduce it (as minimally and precisely as possible):

I've described it at the What happened section. Here is a video.

video

Environment:

  • Extension (VsCodeVim) version: 0.12.0
  • VSCode version: 1.23.1
  • OS version: 10.13.2 (17C88)

Update

Seems like reverting one change helped. I've created a PR.

For v0.14+ fix please see the comment

@rxvt
Copy link

rxvt commented May 28, 2018

This is happening for me also a similar setup:

VSCode: 1.23.1
Extension: 0.12.0
OS version: 10.13.4

Kind of ruining my workflow at the moment, would be much appreciated if this was fixed. Thanks.

@rksys
Copy link

rksys commented Jun 11, 2018

The same issue for me, really making my workflow broken.

  • VSCode: 1.24.0
  • Extension: 0.12.0
  • OS version: 10.13.4

@jpoon
Copy link
Member

jpoon commented Jun 12, 2018

x-posting this from the #2721:

editorIdentity is a unique identifier for the editor/file and is used to retrieve vim state. As you change your active editor, we look up it's ID and then retrieve it's vim state.

Now that we've removed viewColumn (which IMO is a bug) from the editorIdentity, any file with the same path will generate the same ID and we now see this issue of the same file opened in the editor having the same vim state (e.g. same cursor position across editors).

We need a better way to generate uniqueness for editorIdentity. In addition to adding back the viewColumn to generate the editorIdentity, I think we'll also need to keep it in sync with it's actual position when a user moves the window from one column to another otherwise the states (e.g. undo stack) will be messed up again.

There looks to be an event we can register in the vscode api that subscribes to view column changes. https://code.visualstudio.com/docs/extensionAPI/vscode-api:

onDidChangeTextEditorViewColumn: Event<TextEditorViewColumnChangeEvent>

@chibicode
Copy link
Contributor

Copying from #2727 - for me it only causes a problem for insert mode.

kapture 2018-06-12 at 18 14 28

bug demo

1. set insert mode
2. create a new pane showing the same file
3. make sure both cursors are at line 1
4. go to the different pane
5. move cursor to a different line (say line 5)
6. come back to the original pane
7. cursor should still be at line 1
8. press down arrow
9. expected: cursor should go to line 2
10. actual: cursor goes to line 6
11. this is not a problem in normal mode

@jpoon
Copy link
Member

jpoon commented Jun 13, 2018

@rebornix onDidChangeTextEditorViewColumn vscode api doesn't exactly fire when i expected it to. it only seems to fire an event when i create a new/delete a new editor group. do you know if there's an api that fires an event when i move a text editor to a new viewcolumn/editor group?

@jpoon
Copy link
Member

jpoon commented Jun 15, 2018

We are blocked on upstream vscode. Filed microsoft/vscode#52068

@chibicode
Copy link
Contributor

chibicode commented Jun 26, 2018

We are blocked on upstream vscode. Filed https://github.com/Microsoft/vscode#52068

It was marked as a duplicate of microsoft/vscode#15178

@jpoon
Copy link
Member

jpoon commented Jun 27, 2018

Thanks @chibicode. For folks following this issue, please thumbs upstream vscode issue!

@lukaszb
Copy link
Author

lukaszb commented Jun 29, 2018

Seems this would be fixed in near future but it still breaks at 0.14. Here is a diff one can use to fix the issue (but note that this will bring back another issue with undo).

diff --git out/src/editorIdentity.js out/src/editorIdentity.js
index 2635d31..414b434 100644
--- out/src/editorIdentity.js
+++ out/src/editorIdentity.js
@@ -1,20 +1,26 @@
 "use strict";
+const vscode = require("vscode");
+
 Object.defineProperty(exports, "__esModule", { value: true });
 class EditorIdentity {
     constructor(textEditor) {
         this._fileName = (textEditor && textEditor.document && textEditor.document.fileName) || '';
+        this._viewColumn = (textEditor && textEditor.viewColumn) || vscode.ViewColumn.One;
     }
     get fileName() {
         return this._fileName;
     }
+    get viewColumn() {
+      return this._viewColumn;
+    }
     hasSameBuffer(identity) {
         return this.fileName === identity.fileName;
     }
     isEqual(other) {
-        return this.fileName === other.fileName;
+        return this.fileName === other.fileName && this.viewColumn === other.viewColumn;
     }
     toString() {
-        return this.fileName;
+        return this.fileName + this.viewColumn;
     }
 }
 exports.EditorIdentity = EditorIdentity;

you can apply it with patch -p0 < FILENAME at ~/.vscode/extensions/vscodevim.vim-0.14.0/


Updated patch for 0.16:

diff --git out/src/editorIdentity.js out/src/editorIdentity.js
index 2635d31..414b434 100644
--- out/src/editorIdentity.js
+++ out/src/editorIdentity.js
@@ -1,20 +1,26 @@
 "use strict";
+const vscode = require("vscode");
+
 Object.defineProperty(exports, "__esModule", { value: true });
 class EditorIdentity {
     constructor(textEditor) {
         this._fileName = (textEditor && textEditor.document && textEditor.document.fileName) || '';
+        this._viewColumn = (textEditor && textEditor.viewColumn) || vscode.ViewColumn.One;
     }
     get fileName() {
         return this._fileName;
     }
+    get viewColumn() {
+      return this._viewColumn;
+    }
     isEqual(other) {
-        return this.fileName === other.fileName;
+        return this.fileName === other.fileName && this.viewColumn === other.viewColumn;
     }
     toString() {
-        return this.fileName;
+        return this.fileName + this.viewColumn;
     }
 }
 exports.EditorIdentity = EditorIdentity;

@mgood
Copy link

mgood commented Jul 3, 2018

@jpoon thanks for the investigation. Do you know if the upstream VSCode release process has some integration testing that could verify core behavior like this in the Vim extension? Similar bugs seem to pop up every few releases where multi-pane editing breaks due to some upstream change. Since MS seems to be invested in this extension, and this is a pretty critical feature for the Vim editing experience, it would be great if it could be tested prior to a release.

@jpoon
Copy link
Member

jpoon commented Jul 3, 2018

Do you know if the upstream VSCode release process has some integration testing that could verify core behavior like this in the Vim extension?

I don't know what sort of integration tests they do. They do have an insider release of Code that folks can run to get early-access to bits and that's been a source of reported issues.

Since MS seems to be invested in this extension, and this is a pretty critical feature for the Vim editing experience, it would be great if it could be tested prior to a release.

That would be good feedback to give to VS Code. To clarify, this project is not sponsored or supported by Microsoft (aside from the support we get through the GitHub issues of VS Code). A couple of us maintainers/committers are in the employ of Microsoft but we all do this in our off-time.

@jpoon
Copy link
Member

jpoon commented Jul 3, 2018

To sum up this issue, you can pick the worse of the two evils:

  1. Files with the same name will share the same vim state (e.g. history, cursor position, undo stack, etc) or
  2. If you apply the patch outlined by Wrong cursor position after using same file in two panels #2688 (comment), every file will have it's own vim state, but there is a code path where you could have the vim state from one file overwriting that of another

At the moment, we are blocked on VS Code to give us the proper API to solve this.

@lingz
Copy link

lingz commented Aug 22, 2018

Is there any update on this issue, as it has been around for almost 2 months now, and it's a nuisance to re-apply the linked patch everytime the plugin updates.

@jpoon
Copy link
Member

jpoon commented Aug 22, 2018

As mentioned above, this is reliant on the upstream issue: microsoft/vscode#15178.

@captaincaius
Copy link
Contributor

I just noticed this too...

It's a shame that "onDidChangeTextEditorViewColumn" doesn't do what the name implies :-/.

You folks probably already know this, but with experimenting I noticed that in vim... although each split has its own line/column, each buffer only has ONE undo history.

So I wondered if there's another way to do it so we don't care where the editor is positioned... onDidChangeActiveTextEditor says it fires every time you change editors, and some experimentation shows it's called whenever a new :sp is created too :).

It seems to me that if we use that event to ensure we're always referencing the right TextEditor, all should be good. Is there something I'm missing that's preventing us from doing that?

I'll play with it a little today - if there's some reason it won't work, lmk ;).

@captaincaius
Copy link
Contributor

Well, with manual testing it seems the approach works (and without breaking undo \o/). Is there something else it might break that I'm not thinking of?

I'll roll a PR soon, along with any questions this introduces.

@alecthomas
Copy link

Brilliant!

@mattgara
Copy link

This issue is still present, any planned progress?

@J-Fields
Copy link
Member

@vvhitedog This should definitely be fixed. Can you reproduce it reliably? If so, please file a new issue and reference this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests