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

pr for ease of review #85096

Closed
wants to merge 1 commit into from
Closed

Conversation

sbatten
Copy link
Member

@sbatten sbatten commented Nov 19, 2019

never merge
for @sandy081 @isidorn

@sbatten sbatten self-assigned this Nov 19, 2019
@sandy081
Copy link
Member

sandy081 commented Nov 19, 2019

Note: We gonna use pane for split views.

Not sure if we are on right track. I see it is very confusing to have PanelViewlet which provides functionality of Viewlet and panes. This is restricting only Viewlets to extend this functionality because of the inheritance.

I would suggest to do following

  1. Move panes specific functionality from PanelViewlet to a new class called PaneContainer.

  2. Lets use composition instead of inheritance. Merge PanelViewlet into ViewContainerViewlet and let ViewContainerViewlet uses PaneContainer to manage views.

  3. Move views specific functionality from ViewContainerViewlet into a new class called ViewContaier that extends PanelContainerWidget. This will make ViewContainerViewlet slick and contains only viewlet specific knowledge.

  4. Use ViewContaier when moving Panels to view containers and views just like how ViewContainerViewlet does.

@isidorn
Copy link
Contributor

isidorn commented Nov 19, 2019

After discussing with @sandy081 we drew this picture together to make things more clear for everybody.

Current state
WhatsApp Image 2019-11-19 at 11 21 33 (1)

Future state
WhatsApp Image 2019-11-19 at 11 21 33

Some notes:

  • I like the idea of renaming Split View Panels to Pane.
  • I agree with everything @sandy081 pointed out in the previous comment. The best place to start is on the left part of the picture - restructuring the viewlet infrastructure to make it align better with our future architecture. Changing Panels is step2
  • Viewlet = Composite + Viewlet specifc knowledge
  • ViewViewelet = Viewlet + ViewContainer (knowledge about registering new views)
  • PaneContainer = A container around SplitView which can add panes, remove panes
  • ViewContainer = PaneContainer + knowledge around view, registering views etc.
  • PaneContainer and ViewContainer should probably be just merged into one class. We believe this will simplify things. This class cares about registergin views and forwarding everything to the SplitView

@isidorn
Copy link
Contributor

isidorn commented Nov 19, 2019

We showed the arhictecture in the standup and the feedback was that this makes sense, however long term we should merge the Viewlet and Panel which aligns with what we discussed as a goal.
fyi @jrieken

@sbatten sbatten closed this Nov 20, 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.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants