Skip to content

Fix failure of revealing webview with viewColumn which does not exist #87832

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Vigilans
Copy link
Member

@Vigilans Vigilans commented Dec 29, 2019

Currently, create a webview and reveal a webview's behavior is not consistent, when a viewColumn option is provided, but that column(group) does not exist:

  • Create a webview with SIDE_GROUP will open the webview on the active group's side
  • Reveal a webview with SIDE_GROUP will do nothing (revealed on the webview's current group)

The root cause of such inconsistency is in that createWebview forward the viewColumn(group) information to editorService.openEditor, which will be handled by editorService.findTargetGroup.

However, revealWebview gets hands dirty to find the target group by itself, and some important logic is missing, e.g. handling of SIDE_GROUP. So when passing viewColumn to be 1 (start from 0) when there's only 1 column, it will be revealed to column 0 instead of 1.

This PR makes editorService.findTargetGroup public, and foward the group number to it to reuse its logic, and keep the behavior of revealing and creating consistent.

This PR fixes #71608.

@Vigilans Vigilans changed the title Vigilans/fix webview reveal Fix failure of revealing webview with viewColumn which does not exist Dec 29, 2019
@Vigilans Vigilans force-pushed the vigilans/fix-webview-reveal branch from 8191d3f to 4e444a1 Compare December 29, 2019 05:05
@Vigilans Vigilans force-pushed the vigilans/fix-webview-reveal branch from 4e444a1 to a8ef4b4 Compare December 29, 2019 10:17
@kieferrm kieferrm requested a review from mjbvz December 29, 2019 17:51
@@ -869,6 +869,8 @@ export class DelegatingEditorService implements IEditorService {

createInput(input: IResourceEditor): IEditorInput { return this.editorService.createInput(input); }

findTargetGroup(input: IEditorInput, options?: IEditorOptions, group?: OpenInEditorGroup): IEditorGroup { return this.editorService.findTargetGroup(input, options, group); }
Copy link
Collaborator

Choose a reason for hiding this comment

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

@bpasero Can you please take a look at how this change exposes findTargetGroup. Is there a better way to accomplish this?

@bpasero
Copy link
Member

bpasero commented Jan 8, 2020

@mjbvz how is createWebview different from revealWebview? Can you not use IEditorService.openEditor for both cases and let the editor service handle the group?

@mjbvz mjbvz added this to the October 2020 milestone Oct 2, 2020
@mjbvz mjbvz modified the milestones: January 2021, February 2021 Jan 29, 2021
@joaomoreno joaomoreno changed the base branch from master to main February 15, 2021 08:51
@mjbvz mjbvz modified the milestones: February 2021, March 2021 Feb 26, 2021
@mjbvz mjbvz modified the milestones: March 2021, April 2021 Mar 24, 2021
@arozovyk
Copy link

arozovyk commented Apr 8, 2021

Any updates on this?

@mjbvz mjbvz modified the milestones: April 2021, May 2021 Apr 29, 2021
@mjbvz mjbvz modified the milestones: May 2021, Backlog Jun 1, 2021
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.

WebviewPanel.reveal won't reveals to SIDE_GROUP if the specified group does not exist
5 participants