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

Make SwitchHeaderSource use workbench.editor.revealIfOpen setting #8857

Merged
merged 10 commits into from
Feb 18, 2022

Conversation

joelmsmith
Copy link
Contributor

@joelmsmith joelmsmith commented Feb 13, 2022

This change makes the SwitchHeaderSource command respect the workbench.editor.revealIfOpen setting.

C_Cpp.SwitchHeaderSource has different behavior depending on whether the destination file is visible in a different editor pane or not. This commit adds an option headerSourcePaneJumping which controls whether the command will jump to a different editor if the destination file is already visible there. The default value of true preserves the command's original behavior.

Personally I do not want the default behavior; I want it to simply toggle back and forth between header / source in the same editor that is focused, regardless of what the other ones are displaying.

I'm new to vscode and still finding my way around, so it is likely that I've missed or overlooked something. Feedback/guidance is welcome.

headerSourcePaneJumping controls whether the SwitchHeaderSource command
will jump to a different editor split if the destination file is already
visible.
@ghost
Copy link

ghost commented Feb 13, 2022

CLA assistant check
All CLA requirements met.

Copy link
Contributor

@sean-mcmanus sean-mcmanus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine to me, other than the one comment.

Extension/src/LanguageServer/client.ts Outdated Show resolved Hide resolved
Extension/package.json Outdated Show resolved Hide resolved
Copy link
Contributor

@sean-mcmanus sean-mcmanus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bobbrow @jureid Any thoughts on the naming/text?

@joelmsmith
Copy link
Contributor Author

Hi @sean-mcmanus, thanks for looking at this. I'm happy to change any of the naming/text if there's a a way to phrase things that better aligns with the project's terminology.

@bobbrow
Copy link
Member

bobbrow commented Feb 16, 2022

Is there any desire for this to be a more general setting to cover any cases where we would open/focus a text editor? (For example, goto definition)

@joelmsmith
Copy link
Contributor Author

As far as I can tell, Go to Definition, Go To Declaration, and Go To Type Definition all operate within the focused editor.

@bobbrow
Copy link
Member

bobbrow commented Feb 16, 2022

If the definition is outside the current document, we should be opening a new file. But I forget whether that is done by vscode or us.

Copy link
Contributor

@sean-mcmanus sean-mcmanus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of adding a new setting, maybe the switching should read the VS Code workbench.editor.revealIfOpen setting unless I'm misunderstanding that setting or there's a reason to separate out this particular case? I think that defaults to false. i.e. check the result of vscode.workspace.getConfiguration("workbench.editor.revealIfOpen ".

@joelmsmith
Copy link
Contributor Author

@sean-mcmanus - I'll take a look at this. I did not know about this setting, but it sounds like making SwitchHeaderSource obey it will do what I want.
@bobbrow - a new file gets opened (or the tab focused, if it is already opened), but for those 'Go To' commands this happens in the same split (or what the Reval If Open setting refers to as "currently active editor group").

@joelmsmith joelmsmith changed the title Add setting for SwitchHeaderSource UI behavior Make SwitchHeaderSource use workbench.editor.revealIfOpen setting Feb 17, 2022
Extension/src/LanguageServer/extension.ts Outdated Show resolved Hide resolved
@sean-mcmanus sean-mcmanus added this to the 1.9.1 (insiders2) milestone Feb 18, 2022
@sean-mcmanus sean-mcmanus merged commit 111a653 into microsoft:main Feb 18, 2022
@joelmsmith joelmsmith deleted the updateSwitchHeaderSource branch February 19, 2022 03:09
@github-actions github-actions bot locked and limited conversation to collaborators Apr 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants