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

Possible fix for #3455: Compare EditorIdentity when handling window.onDidChangeTextEditorSelection. #3539

Conversation

coddingtonbear
Copy link

@coddingtonbear coddingtonbear commented Feb 28, 2019

What this PR does / why we need it:

This PR changes two things:

  • Alters EditorIdentity such that it uses the ViewColumn of the editor for differentiating between multiple editors open to the same file using changes long ago proposed by @lukaszb (thanks for posting a patch!).
  • Updates our handler for vscode.window.onDidChangeTextEditorSelection such that it uses that EditorIdentity when deciding whether to handle an incoming event instead of simply comparing documents.

Which issue(s) this PR fixes

#3455

Special notes for your reviewer:

@jpoon
Copy link
Member

jpoon commented Mar 4, 2019

Iirc, there is a case where this breaks undo. #2688

@coddingtonbear
Copy link
Author

Yes! I have seen a situation in which it seems like the undo history is shared between panes now, and when undoing something in one pane, it might undo an action that took place in the other. I wasn't immediately sure that that was a regression this change created, but am ?glad to hear that you can confirm that it is.

Would it be your opinion that this approach toward solving this problem is unworkable (meaning, I shouldn't bother spending time figuring out how vscode-vim handles undo queues), or is it worthwhile to keep digging into things to see if I can fix undo handling with the changes in this PR?

@jpoon
Copy link
Member

jpoon commented Mar 5, 2019

This was an old issue so not sure if I recall the details but the current behaviour was the lesser of two evils given the limitation with the vscode API there's an upstream issue somewhere (On my phone so can't link).

At the moment, we only have one notion of an editor id but I feel like we might need to split this up as there Is state specific to:

  • Unique to the text editor -- cursor position, movements, action history
  • Unique to the file -- undo/redo

@coddingtonbear
Copy link
Author

Just to be clear, if we're expecting undo/redo to be shared among all open editors to the same file -- and in hindsight, I can't imagine it any other way -- I think this is operating as expected for me so far in the ways you've described above.

The weird thing I do think isn't working is that search/replace within a visual selection isn't working. I haven't dug into it very deeply, but given the things this PR changed, I can think of several ideas about what might've become broken and will try to find a little time to dig deeper this week.

@coddingtonbear coddingtonbear force-pushed the fix_3455_check_editoridentity_when_updating_selection branch from 7d97e7a to 062768d Compare July 8, 2019 18:13
@J-Fields
Copy link
Member

J-Fields commented Sep 1, 2019

Hey @coddingtonbear, thanks for this PR!
I'm seeing some misbehavior with undo/redo. Here's an example:

  1. Open new document
  2. :vsp
  3. o1<esc>o2<esc>o3<esc>
  4. Undo/redo to see they operate on single line additions
  5. Go to other side and see undo undoes all 3 lines
  6. Try to redo on first side - doesn't work

@coddingtonbear
Copy link
Author

Oh, sure, this totally doesn't work in the latest version of vscodevim, @J-Fields, and I don't have a lot of time to update it, so I think I'm just going to close this PR 🤷‍♂️ .

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

Successfully merging this pull request may close these issues.

3 participants