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

Fixed #2688. Revert "Removed viewcolumn from editoridentity information" #2721

Closed
wants to merge 2 commits into from

Conversation

lukaszb
Copy link

@lukaszb lukaszb commented Jun 11, 2018

I've pinpointed breaking change using git bisect. Just tried to revert this and see if it helps. Not sure if it breaks anything or relates to some other changes so feel free to treat this PR as a simple hint.

@jpoon
Copy link
Member

jpoon commented Jun 11, 2018

We [ @Chillee ] removed the viewColumn (#2559) as that was causing issues with the undo stack.

Considering #2688, 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.

@lukaszb
Copy link
Author

lukaszb commented Jun 11, 2018

@jpoon thanks for clarification. However, imho opening same buffer side by side is more common than using undo action. That's only me, though.

@jpoon
Copy link
Member

jpoon commented Jun 11, 2018

However, imho opening same buffer side by side is more common than using undo action. That's only me, though.

Are you advocating that we fix (1) same buffer side by side by re-introducing (2) #2007?

@lukaszb
Copy link
Author

lukaszb commented Jun 12, 2018

@jpoon well, for me broken undo is much less intrusive into my workflow than broken cursor position among multiple open buffers. I.e. I do lot of vue-js programming with single file components (markup, code and styles sit within same file) and have to work with same file opened at two panes pretty much all the time. I have not enough knowledge to be more helpful here - I don't even fully understand what is the impact of editorIdentity change on the undo or cursor position (apparently there are no tests for it so it's hard to tell what it breaks or what it supports).

Currently I simply replace ~/.vscode/extensions/vscodevim.vim-XXX with repo of my PR's branch, run npm i && npm run build as a workaround.

@jpoon
Copy link
Member

jpoon commented Jun 12, 2018

editorIdentity is a unique identifier for the editor/file and is used to retrieve vim state. (

let [curHandler, isNewModeHandler] = await ModeHandlerMap.getOrCreate(activeEditorId.toString());
). 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). Let me know if there's any knowledge gaps I can help fill.

As mentioned in an earlier comment,

Considering #2688, 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.

If we attempt a fix, we should fix it properly and not replace one bug with another especially considering the undo issue was causing so much grief to so many users.

@lukaszb
Copy link
Author

lukaszb commented Jun 12, 2018

@jpoon agree and thanks for explanation. Closing this PR then.

@lukaszb lukaszb closed this Jun 12, 2018
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.

None yet

2 participants