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

showTextDocument & vscode.diff always default to ViewColumn.One #27408

Closed
eamodio opened this issue May 29, 2017 · 8 comments · Fixed by #34649
Closed

showTextDocument & vscode.diff always default to ViewColumn.One #27408

eamodio opened this issue May 29, 2017 · 8 comments · Fixed by #34649
Assignees
Labels
api under-discussion Issue is under discussion for relevance, priority, approach

Comments

@eamodio
Copy link
Contributor

eamodio commented May 29, 2017

  • VSCode Version: Code - Insiders 1.13.0-insider (770206a, 2017-05-26T05:14:23.944Z)
  • OS Version: Windows_NT ia32 10.0.16199

These 2 apis should honor the ViewColumn if specified, but if left undefined they should default to the column of the active editor

Having this will eliminate code in many extensions such as:

(window.activeTextEditor && window.activeTextEditor.viewColumn) || 1

Which fails for anything that isn't a text document (since viewColumn will be missing)

@jrieken jrieken added api under-discussion Issue is under discussion for relevance, priority, approach labels May 30, 2017
@jrieken
Copy link
Member

jrieken commented May 30, 2017

Which fails for anything that isn't a text document (since viewColumn will be missing)

Unsure what that means?

@eamodio
Copy link
Contributor Author

eamodio commented May 30, 2017

Sorry, I meant that the window.activeTextEditor could be undefined if the actual active "editor" is a non-text document (image, extension, etc). And window.activeTextEditor.viewColumn can be undefined for a diff editor (maybe other cases?). So an extension can't reliably get the real "active" column

@bpasero
Copy link
Member

bpasero commented Sep 15, 2017

+1 for not setting ViewColumn.ONE as default if not specified so that we prefer the currently active editor column when documents open.

@eamodio
Copy link
Contributor Author

eamodio commented Sep 15, 2017

@bpasero PR #27409 addresses it, but it conflicts with another change see: #27409 (comment)

Any thoughts on coming to a resolution on that -- I would LOVE to see this fixed.

@bpasero
Copy link
Member

bpasero commented Sep 15, 2017

@eamodio we have an option for editors to reveal if they are open in another group (see revealIfOpened).

I am actually surprised that #25801 was fixed and marked as important because the normal workbench behaviour is to open an editor in the currently active editor group. On top of that, the solution also does not seem right because what if I start in column 2 and I open a document in column 3 and then open the document from column 2 again. It opens in column 1?

@eamodio
Copy link
Contributor Author

eamodio commented Sep 15, 2017

@bpasero I was surprised as well as that was my argument too :)

@eamodio
Copy link
Contributor Author

eamodio commented Sep 15, 2017

I could rebase that PR again (since its quite old) if you'd like, but it will still fail because of a test checking for #25801

@jrieken
Copy link
Member

jrieken commented Sep 15, 2017

I am actually surprised that #25801 was fixed and marked as important because the normal workbench behaviour is to open an editor in the currently active editor group.

Well, we had this behaviour in place, extensions rely on it #25801 (comment), and we don't want to break them. That's why this is tricky

On top of that, the solution also does not seem right because what if I start in column 2 and I open a document in column 3 and then open the document from column 2 again. It opens in column 1?

The solution is to use column 1 if not specified, no column-math happens: c9963a4

eamodio added a commit to eamodio/vscode that referenced this issue Sep 19, 2017
jrieken added a commit that referenced this issue Sep 20, 2017
@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api under-discussion Issue is under discussion for relevance, priority, approach
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants