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.

Sign up for GitHub

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

Jump to bottom

Dynamic composite bar #48037

Merged
merged 2 commits into from
Apr 18, 2018
Merged

Dynamic composite bar #48037

merged 2 commits into from
Apr 18, 2018

Conversation

sandy081
Copy link
Member

  • Add to composite bar when new composite is added
  • Sequential order for in-built viewlets

Required for #43645

@sandy081
- Add to composite bar when new composite is added
f60e0f9 
- Sequential order for in-built viewlets
@sandy081 sandy081 requested a review from isidorn April 17, 2018 08:45
@sandy081 sandy081 added the layout General VS Code workbench layout issues label Apr 17, 2018
@sandy081 sandy081 added this to the April 2018 milestone Apr 17, 2018
@isidorn
Copy link
Contributor

isidorn commented Apr 17, 2018

@sandy081 thanks for the PR. This makes sense.
What I do not understand is why composites which get registered later are added as not pinned to the CompositeBar?
Why do you need that option and distinction?

@sandy081
Copy link
Member Author

sandy081 commented Apr 17, 2018 •
edited

@isidorn If you always pin it, then it will override the existing pinned viewlet when refreshed.

Edit: I see, the newly registered viewlets will not get pinned which is not expected. So we need a way to pin it but not show it by default.

@sandy081
Copy link
Member Author

I will update the PR to handle following correctly

  • When a viewlet is added for the first time, it has to be pinned but do not show
    • Pin it by its order
    • Do not show
  • When a viewlet is added after refresh (was already added before)
    • Pin it at the position it was before
    • Show if it was shown before
  • When a viewlet is removed after refresh but was there before, then pinned should not have it.

Hope I cover all cases.

@isidorn
Copy link
Contributor

isidorn commented Apr 17, 2018

If you always pin it, then it will override the existing pinned viewlet when refreshed.

Can you please clarify this with an example. Just so we are on the same page.
Multiple viewlets can be pinned at the same time. If a user adds Azure viewlet and it is pinned that just means it is shown in the activity bar and this should not affect any other viewlet afaik.

@sandy081
Copy link
Member Author

I think pinning is not the issue. It is about showing the viewlet. When adding a viewlet, it pins and shows automatically. This is overriding the viewlet that is currently shown (after refresh).

@isidorn
Copy link
Contributor

isidorn commented Apr 17, 2018

Ok. Yeah compositeBar should support to show a new pinned viewlet without it being the active one.

@sandy081
- Store all current composites with pinned state
40f2700 
- Retain the composites states
- Compute pinned composites from loaded composite states and  current composites
- Store on shutdown
- Parameter in add api to activate the added composite
- Pin the added composite at currect location
@sandy081
Copy link
Member Author

@isidorn Updated the PR with following changes

  • Store all current composites with pinned state and retain the composites states
  • Compute pinned composites from loaded composite states and current composites
  • Pin the added composite at correct location
  • Parameter in add api to activate the added composite
  • Store states on shutdown

@isidorn
Copy link
Contributor

isidorn commented Apr 18, 2018

@sandy081 I took a look at the PR and previsouly we were storing a list of pinned composite ids.
A now we store a list of objects, where each object has an id and if it is pinned. So basically the difference is that each composite bar also stores unpinned composite ids. Why is that?
Can't the owner of the composite bar store that so we do not change what we store in the composite bar?

@sandy081
Copy link
Member Author

@isidorn Composite bar has the following state

  • All Composites
  • Pinned Composites
  • Order of Composites

It determines which composite has to be pinned or not and in which order (also consults its history sometimes). So I think it is the right one to store this information.

If we move this to the owner, then everybody has to compute, if newly added composite has to be pinned or not by checking the previous state. They should also store the order. It means the composite bar state has to be shared which I think is not a good design.

I also think there is still some general improvements to be done to Composite bar.

  • Context menu is not in the same order as the actions are shown
  • Order is lost if an action is unpinned and pinned back.

I wanted to replace pinnedComposites with initialCompositesStates by renaming it to compositesStates but, I felt that a bigger and risky change. I think we might need to do it eventually to fix/implement above cases. Since this needs order to be stored as separate property in CompositeState.

@sandy081
Copy link
Member Author

Also for your question,

So basically the difference is that each composite bar also stores unpinned composite ids. Why is that?

This is to check if a newly added composite has to be pinned or not which needs initial state.

sandy081
sandy081 commented Apr 18, 2018
View reviewed changes
src/vs/workbench/browser/parts/compositebar/compositeBar.ts
this.pin(compositeData.id, true, i);

const compositeState = this.initialCompositesStates.filter(c => c.id === compositeData.id)[0];
if (!compositeState /* new composites are pinned by default */ || compositeState.pinned) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@isidorn This is the reference why we need initial states

@isidorn
Copy link
Contributor

isidorn commented Apr 18, 2018

Ok this makes sense. I tried it out and nothing seems broken.
Did you test it out with contributed viewlets? To be sure I would try out the following cases:

  • search moved to panel and back to activity bar
  • screen resized such that not all viewlets can be fit - make sure that the active one is still shown always
  • You can rearrange and hide viewlets

@sandy081
Copy link
Member Author

I tried with the contributed viewlets and some of the test cases you mentioned. Will test all cases and confirm.

@sandy081
Copy link
Member Author

@isidorn Tried out all scenarios and it looks good.

@isidorn
Copy link
Contributor

isidorn commented Apr 18, 2018

@sandy081 great, then feel free to merge this in. A test plan item would also be good.

@sandy081
Copy link
Member Author

Thanks and will have the testing part of activity groups contributions.

@sandy081 sandy081 merged commit 3c6305f into master Apr 18, 2018
@joaomoreno joaomoreno deleted the sandy081/dynamicCompositeBar branch April 30, 2018 08:31
@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.
Reviewers

@isidorn isidorn Awaiting requested review from isidorn

Assignees

@isidorn isidorn

Labels
layout General VS Code workbench layout issues
Projects
None yet
Milestone
April 2018
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@sandy081 @isidorn