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

Enhance views service #87505

Merged
merged 9 commits into from Jan 13, 2020
Merged

Enhance views service #87505

merged 9 commits into from Jan 13, 2020

Conversation

@sbatten
Copy link
Member

sbatten commented Dec 21, 2019

refs #87461

@sandy081 let me know if this looks on the right path. I ran into an issue with the views service now depending on the panel service and the panel service depending on the views service, both in the constructor. you can see I work around this with the instantiation service but still the panel service needs delay its panel registration call somehow (see setTimeout call in constructor). I don't think this part is quite right so I'm looking for suggestions. Depending on a few things, sometimes I see the outline view rendered twice in the panel when moved during testing so need to iron that out as well. I think it might just be caused by the setTimeout though.

wip
@sbatten sbatten self-assigned this Dec 21, 2019
@sbatten sbatten added this to the January 2020 milestone Dec 21, 2019
@sbatten sbatten requested a review from sandy081 Dec 21, 2019
@sandy081

This comment has been minimized.

Copy link
Member

sandy081 commented Dec 23, 2019

Went through the changes. They make sense and I have following suggestions:

  1. Now, ViewsService and ViewsRegistry are having similar functionality - registering views and providing events. Is it possible to extract that into a model say ViewDescriptorsModel that can store view descriptors and provide events when something happens? Then. ViewsService and ViewsRegistry can simply use this model as composition and duplication of code can

  2. To your issues regarding cyclic dependency between ViewsService and PanelService - I would suggest to separate PanelPart and PanelService just like ActivityBarPart and SidebarPart. @isidorn let me know if it makes sense to you.

Copy link
Member

sandy081 left a comment

Provided feedback in the comment

@sandy081 sandy081 requested a review from isidorn Dec 23, 2019
@isidorn

This comment has been minimized.

Copy link
Contributor

isidorn commented Dec 23, 2019

@sbatten If the splitup of the PanelPart makes your life easier than I am fully suporting this. For lack of a better name you would introduce something like PanelTitlePart. The downside of this approach is that from the UX perspective the panel and sidebar do not match, since the ActivityPart and the SidebarPart are really different and you can have one without the other, we do not have a notion like that in the Panel.
Bottom line: we should get rid of the timeout, if there are no better option than splitting up works for me.

sbatten added 2 commits Jan 9, 2020
@sbatten

This comment has been minimized.

Copy link
Member Author

sbatten commented Jan 9, 2020

@sandy081 @isidorn I explored splitting up the panelpart and panelservice as well as alternatives. I settled on delaying the viewsservice's usage of panel service by using the panel registry directly. its not the perfect solution but i found it better than the timeout as well as significantly simpler than the splitting of the panelpart/service.

@sbatten

This comment has been minimized.

Copy link
Member Author

sbatten commented Jan 9, 2020

@sandy081 I'm not sure I understand the model suggestion above. The interfaces look similar but the actual logic is a bit different. When I tried to logic it out, I felt like I was adding an equivalent amount of code with not much benefit. Perhaps we should have a call in the morning.

@@ -281,6 +281,7 @@ export class PanelPart extends CompositePart<Panel> implements IPanelService {
}

private onPanelOpen(panel: IPanel): void {
const viewsService = this.instantiationService.invokeFunction(accessor => accessor.get(IViewsService));

This comment has been minimized.

Copy link
@sandy081

sandy081 Jan 10, 2020

Member

This can be removed I suppose as you are having IViewService defined in the constructor

}

private async openComposite(compositeId: string, location: ViewContainerLocation, focus?: boolean): Promise<IPaneComposite | undefined> {
const panelService = this.instantiationService.invokeFunction(accessor => accessor.get(IPanelService));

This comment has been minimized.

Copy link
@sandy081

sandy081 Jan 10, 2020

Member

I do not think this is correct usage of services. If it is difficult to extract panel part, how about extracting views service into IViewDescriptorService and IViewsService ? Former deals only with view and view container descriptors and later deals with views (opening, focusing etc) ?

@sandy081

This comment has been minimized.

Copy link
Member

sandy081 commented Jan 10, 2020

Re model suggestions:

ViewsService and ViewsRegistry are maintaining stores for view descriptors. My suggestion is to have a ViewDescriptorStore that can store view descriptors per container and trigger events when there are changes so that the registry and service can use this store?

Not sure if I will be available for call today as I might be leaving early.

Copy link
Member

sandy081 left a comment

LGTM. I would prefer calling IViewOpenerService as IViewService as it is dealing with views and the current IViewOpenerService as IViewDescriptorService as it is dealing with view descriptors.

@sbatten sbatten merged commit 91283c1 into microsoft:master Jan 13, 2020
4 of 5 checks passed
4 of 5 checks passed
linux
Details
windows
Details
darwin
Details
VS Code in progress
Details
license/cla All CLA requirements met.
Details
@sbatten sbatten deleted the sbatten:enhanceViewsService branch Jan 13, 2020
aeschli added a commit that referenced this pull request Jan 13, 2020
Add view-moving functionality to viewsservice which has been renamed to viewdescriptorservice. also includes outline view moving
@sbatten sbatten added the on-testplan label Jan 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.