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

Enable SCM Views to move to panel #90461

Closed
sbatten opened this issue Feb 11, 2020 · 20 comments
Closed

Enable SCM Views to move to panel #90461

sbatten opened this issue Feb 11, 2020 · 20 comments
Assignees
Labels
feature-request Request for new features or functionality on-release-notes Issue/pull request mentioned in release notes on-testplan workbench-views Workbench view issues
Milestone

Comments

@sbatten
Copy link
Member

sbatten commented Feb 11, 2020

Tracking item to enable SCM views to move using new infra. It looks like SCM has some hurdles when moving to the panel. it registers views as they come in but the views have relationships kind of like extensions viewlet. wondering if we need a way to say "this container is a unit and everything must move together"

@sbatten sbatten self-assigned this Feb 11, 2020
@sbatten sbatten added workbench-views Workbench view issues feature-request Request for new features or functionality labels Feb 11, 2020
@sbatten sbatten added this to the February 2020 milestone Feb 11, 2020
@joaomoreno
Copy link
Member

SCM is definitely a strange beast. Its UX baggage (one view controls others' visibilities) makes it hard to reason about when we consider moving views around.

@sandy081
Copy link
Member

Yeah same boat as in extension viewlet views. I decided not to move extension because they are tightly associated to the search functionality. Same applies for SCM too.

@sbatten
Copy link
Member Author

sbatten commented Feb 12, 2020

@joaomoreno due to the fact that other views can be contributed to the scm viewlet and they could be moved, I think we should explore getting scm views to be movable too.

@sbatten sbatten modified the milestones: February 2020, March 2020 Feb 20, 2020
@joaomoreno
Copy link
Member

After discussing this with @sandy081 we've come up with an idea:

  1. Currently, the SCM Providers view acts as a father in a paternity (hint!) relationship to all repository views. When rows are selected in its list, views get visible and vice-versa. This is a UX pattern which does not exist anywhere else in the workbench. We suggest to simply drop this, knowingly annoying some users, breaking up that relationship. We can do this since view management is now such a core part of using VS Code, which it wasn't when this pattern was created. Users can still collapse and hide views by using the usual context actions that work across the workbench.
  2. Once that relationship is broken, then adopting moving views in SCM is easy: we just enable it!

What do you think @sbatten? We could act on step 1 beginning of April for Insiers, once I'm back, and see what the feedback is.

@sbatten
Copy link
Member Author

sbatten commented Feb 28, 2020

So the Providers view would still impact the content of the other views but not the visibility, yes?

Also, remote explorer has a similar yet different issue with the header controlling the views.

@sandy081
Copy link
Member

sandy081 commented Mar 2, 2020

So the Providers view would still impact the content of the other views but not the visibility, yes?

No. With above approach, provides view will not control visibility of the view. Above approach is to align SCM views more with other views and they can be toggled visibility just like other views.

@sbatten
Copy link
Member Author

sbatten commented Mar 2, 2020

I think my phrasing was confusing. I understand that the visibility of other views would no longer be impacted by the providers view.

  • Will the providers view still exist?
  • If so, what is the impact of selecting a provider in that view? Does it change the content of the other views still?

@sandy081
Copy link
Member

sandy081 commented Mar 2, 2020

Will the providers view still exist?

Yes. It is useful to have because it can show the details of incoming changes and can sync from there.

If so, what is the impact of selecting a provider in that view? Does it change the content of the other views still?

As said before, selecting does not impact other views visibilities.

@sbatten
Copy link
Member Author

sbatten commented Mar 2, 2020

@Tyriar and I discussed this and think that simplifying the current experience could make this a lot easier. Perhaps the repo views that are currently split up into multiple views should just be a single view. This way if someone moves the view to the panel, they don't have to do this for every single repo. One option would be to have the contents of the single repo view update with the providers view selection. The second option would be to have a merged view of all the repositories. i.e. exactly like today except the views are under a single view.

@Tyriar also suggested having a mixed mode where some of the focusing/visibility interactions would still work if the views are in their "home" container with the providers view. This gets complicated if some of them are and some are not.

@sbatten
Copy link
Member Author

sbatten commented Mar 4, 2020

@eamodio was browsing through the SCM repository pane code and because SCM is very special it's handling of visibility might be sketchy, but I will leave this to @joaomoreno since he is an expert in all things views.

@joaomoreno
Copy link
Member

joaomoreno commented Apr 3, 2020

Perhaps the repo views that are currently split up into multiple views should just be a single view. This way if someone moves the view to the panel, they don't have to do this for every single repo.

This simplifies view management but definitely complicates SCM usage. There definitely is the use case to have multiple repositories visible, we can't take that away. And to mix them in the same scrollable view just won't cut it...

After marinating on this for a month, I still think the proposal above makes sense: just disconnect the repositories view selection from visibility of other views. Everything else stays as today.

Thoughts?

@sbatten
Copy link
Member Author

sbatten commented Apr 3, 2020

With the current proposal though

  1. a user opens workspace A with a git repo
  2. moves the repo changes to the panel
  3. closes workspace A and opens workspace B
  4. The changes view for this repo is still in the sidebar and they must move it again.

If the user wants SCM in the panel, they will have to repeat it over and over again.

Another thought is to somehow generalize the concept of grouped views so that any view moved in a group takes all of its group to the new container.

@joaomoreno
Copy link
Member

Isn't the recommendation there for the user to simply move the entire SCM view container to the panel?

@sandy081
Copy link
Member

sandy081 commented Apr 6, 2020

I see the point from @sbatten. But SCM changes views are workspace scoped views. For example hiding a repository change view does not impact the repository change view in another window. Is not the same expected when the repository changes view is moved? It is also clear from UI as these views are called differently in each workspace?

IMO logical grouping comes from specific area like SCM and may be as @joaomoreno mentioned users can move SCM container in that case?

@sbatten
Copy link
Member Author

sbatten commented Apr 9, 2020

we do not currently do anything that would allow us to know that moving the SCM view container by dragging the SCM icon from the activity bar is a logical grouping. we could support this, but is this the intention?

I see the point about scm being workspace specific but we had not yet enabled view movement which is a much more obvious representation of differences between workspaces. i.e. if a user moves a view to the panel and every other view remembers its location, they should expect that scm does the same. i don't think users of single-repo (the most common case) think of each changes view as a different view. also the repository view is unique in that it only applies to workspaces with multiple repos and thus has some workspace context associated with it.

@sbatten
Copy link
Member Author

sbatten commented Apr 27, 2020

With view container movement, it would be possible to move the entire SCM container to the panel; however, we need to ensure that all SCM views will behave before allowing it. We should try this in debt week.

@sbatten sbatten modified the milestones: April 2020, May 2020 Apr 27, 2020
@johncalvinyoung
Copy link

As a user who usually has multiple repos in workspace scope who badly wants to move the SCM view around, I'm alright treating the various change views as one view.

@joaomoreno
Copy link
Member

I've attempted to simply enable this, but given the reach onto the view model that the SCM Providers view has, it completely blew up. I need to postpone this for next month, sorry all!

@sbatten
Copy link
Member Author

sbatten commented Jun 26, 2020

fyi @joaomoreno 3158fc0

@joaomoreno
Copy link
Member

Awesome, thanks! Let's close this!

@joaomoreno joaomoreno added on-testplan on-release-notes Issue/pull request mentioned in release notes labels Jun 30, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Aug 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request Request for new features or functionality on-release-notes Issue/pull request mentioned in release notes on-testplan workbench-views Workbench view issues
Projects
None yet
Development

No branches or pull requests

4 participants