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

Viewlet Refactor #85566

Merged
merged 21 commits into from
Dec 9, 2019
Merged

Viewlet Refactor #85566

merged 21 commits into from
Dec 9, 2019

Conversation

sbatten
Copy link
Member

@sbatten sbatten commented Nov 25, 2019

This PR refs #85164

The goal of this PR is to refactor the Viewlet structure so that code useful to enhancing panels in the same way is separated from the Viewlet inheritance path.

This PR

  • Introduces new classes PaneComposite, ViewPaneContainer. ViewPaneContainer encapsulates the SplitView and View management behavior currently unique to Viewlet in a standalone class. As a derived class of Composite, PaneComposite composes ViewPaneContainer allowing this enhancement behavior to be shared between Viewlet and Panel.
  • Updates all existing derived Viewlets, (e.g. Explorer), to extend from ViewPaneContainer. The goal is to make ViewPaneContainer implementations agnostic of their Composite container (Panel or Viewlet).
  • Implements very simple Viewlet classes for each existing Viewlet to instantiate the new ViewPaneContainer.
  • Renames ViewletPane to ViewPane

@sbatten sbatten self-assigned this Nov 25, 2019
@isidorn
Copy link
Contributor

isidorn commented Nov 27, 2019

@sbatten the changes you outlined here make a lot of sense to me.
I did not review the code, since there are quite some changes and a lot of it is duplicated work per viewlet if I am not mistaken. Point being: looks good from a birds eye view! Also if you would like us to sync let me know, though I believe @sandy081 provided great guidance.

@sandy081
Copy link
Member

@stevencl I went through the changes and they all looks good to me over all. I did a quick testing and found following

  • Title of the view is not show in the title of the viewlet when there is only one view

image

  • Drag n drop of views inside viewlet is not working

  • Remote explorer is not showing the filter dropdown

image

Otherwise most of the features are working good. 👏

@isidorn
Copy link
Contributor

isidorn commented Nov 28, 2019

@stevencl let me know if you would like me to test this out. Or even better create a test plan item and assign me to it.

@stevencl
Copy link
Member

@sbatten - I think comments from @isidorn and @sandy081 above were for you instead of me... :-)

@isidorn
Copy link
Contributor

isidorn commented Nov 28, 2019

Yes! Sorry for the ping Steven Clarke :)

@sbatten
Copy link
Member Author

sbatten commented Nov 29, 2019

@sandy081 updated to fixes the issues you found.

This PR will be checked in when master opens after endgame testing. I'll keep in up to date with master as necessary. Let's use an endgame test plan item (I will create and assign) to harden.

@sbatten
Copy link
Member Author

sbatten commented Dec 4, 2019

Bringing in from TPI issue so as not to keep an issue open for non-checked in code:
Debug view is not merged into viewlet header if it only view in debug viewlet #86056

That issue is also not specific to my branch. Since there are no more issue specific to my branch and this has gone through an endgame review, I will merge to avoid more conflicts. If you have additional feedback, we can add it straight to master.

@sbatten sbatten merged commit cabcace into microsoft:master Dec 9, 2019
@sbatten sbatten deleted the wip/viewpanecontainer branch December 9, 2019 23:30
@sbatten sbatten added this to the December/January 2020 milestone Dec 9, 2019
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants