Show file changes as a tree in Source Control view #7505

westbury · 2020-04-06T09:14:18Z

Replaces file change list in Source Control view with tree view. This fixes #4835

What it does

The change lists are replaced by a single tree that uses TreeWidget. A toggle button has been added so the user can toggle between a flat view and a tree view.

What will not be covered by this PR

The diff widget

How to test

Try out the Source Control view. This is the only view that should have changed. The 'flat' mode should look mostly the same but even in flat mode it is now in a tree, with each group (staged, unstaged etc) being collapsable. Test with both the Git vs-code built-in and with the native Git extension.

Review checklist

[x ] as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

as a reviewer, I agree to behave in accordance with the review guidelines

akosyakov · 2020-04-08T07:34:33Z

I have not looked at code, only played a bit with it. I really like this feature, it makes code reviews so easier by checking what kind of extensions PR is addressing. Thank you ❤️

I've noticed that:

Files are not grouped properly for sub folders. See the screenshots below.
In the tree mode there should not be description, only in the flat mode.
I cannot only use keyboard to navigate, in similar ways like in the debug view, between different views with tab, arrow keys and enter. I hope you are using ViewContainer to inherit this logic instead of reimplementing it. You may want to check out how a new vsx-registry extension does it.
Styling is broken: not all rows have the same height, i.e. part headers are way to bigger than files and folders. It should be similar to the debug and extensions view. Again please reuse ViewContainer to ensure proper styling.
Folders are not themed properly. In final PR we have to test that we switch between different color and icon themes. It should look good in all of them. fyi theming guidelines just for reference: https://github.com/eclipse-theia/theia/wiki/Coding-Guidelines#theming

Here how changes of this PR look in VS Code for the minimal icon theme:

The same in Theia with this PR:

westbury · 2020-04-13T14:07:44Z

Files are not grouped properly for sub folders. See the screenshots below.

That was actually deliberate design. If there was only a single file in a folder then the file is shown with the folder as the path. This might have been more obvious if the full path was not shown in the version you looked at. However, to avoid confusion I have changed this so that folders are always shown in the tree, just like VS Code.

I cannot only use keyboard to navigate

I think that has now been fixed. I can tab from the commit line then cursor around the tree.

I hope you are using ViewContainer to inherit this logic instead of reimplementing it

This logic for cursor keys is implemented in tree-widget. To fix the key cursors I only had to remove the original key cursor code that was in the old Source Control view. I have not re-implemented anything from ViewContainer.

Styling is broken: not all rows have the same height

This PR is not intended to change the styling. The height of the group headers has not been changed. The original components are used for the group headers and resource rows.

akosyakov · 2020-04-15T12:52:20Z

... However, to avoid confusion I have changed this so that folders are always shown in the tree, just like VS Code.

👍

I think that has now been fixed. I can tab from the commit line then cursor around the tree.

I meant that i can navigate to section heard and use keyboard collapse/expand it as well as move between sections. I think you fixed navigation between sections. It should be good enough for the start.

This PR is not intended to change the styling. The height of the group headers has not been changed. The original components are used for the group headers and resource rows.

But it does not look or behave nice? I think we should fix it before merging.

header should look the same as other rows
expand/collapse should not cause any jumping effects
folders should have proper icons across all icon and color themes
folder and files styles should be the same, i.e. folder labels should be rendered with description color in tree mode

Other issues:

the tree state is lost while switching between flat/tree modes:
do a change in .theia/settings.json, try to discard them in tree mode on different levels: section header, folder and file. For me it works only on section header level.
navigation left/right does not as before
- it should iterate through all changes in the file, not jump to next file immediately
- in flat mode it does not go anywhere sometimes
- we need to check that the diff view is not affected, since navigation code was shared before
what open file supposed to do for folders? it should be removed

BTW could you file a separate issue for the diff widget? It would be super cool to have it there. At least till we cannot use the gitlens vscode extension.

akosyakov · 2020-04-15T12:54:00Z

@eclipse-theia/ecd-theia-committers please help with testing. We need to test that all features are working in tree and flat modes on different levels (header section, folder, sub folder, a file). We should also make sure that:

theming is proper
it is easy to operate with keyboard
there is no unexpected UI effects
it should work against @theia/git as well as again vscode built-in git extension
- a case when they are installed together is not interesting one

akosyakov · 2020-04-15T13:06:58Z

Filed #7581. If someone is up to open another PR against this PR which adds integration tests (as it was done during Monaco migration) please do. It does not mean that we should automate everything now, but maybe add some test for regressions. For instance this PR (in the current state) breaks how we handle left/right keys to navigate through all changes.

vince-fugnitto

A couple of things I noticed quickly, I'll do more testing as I go:

the changes section is quite large compared to master and needs to be fixed:

the icons for folders are incorrect (we should either not display icons like in vscode for folder nodes, or we should apply the correct icon by using the filestat which is able to determine if a resource is a folder). The alignment of nodes is also off (when using the seti icon theme for example).

the ordering of changes is inconsistent with vscode, we should follow the same ordering behavior as the explorer:

with the changes, I am unable to select the ellipsis toolbar item to display the context menu of actions

westbury · 2020-04-20T08:39:25Z

the changes section is quite large compared to master and needs to be fixed:

I've removed the 5px margin that had been there.

the icons for folders are incorrect

A FileStat is now used, as you suggested, to get the icon from the label provider. It now shows as a folder icon.

the ordering of changes is inconsistent with vscode, we should follow the same ordering behavior as the explorer

Now sorted the same as vs-code with folders first then files

with the changes, I am unable to select the ellipsis toolbar item to display the context menu of actions

Yes, I saw this too. However I have also seen it on the master branch. I will need to investigate this.

westbury · 2020-04-20T09:07:57Z

I meant that i can navigate to section heard and use keyboard collapse/expand it as well as move between sections. I think you fixed navigation between sections. It should be good enough for the start.

OK. I have put back the code that uses the left-right cursor keys to move through the changes within a file. However I integrated it with the implementation in tree-widget so the keys also go in and out of folders as they do in the explorer. It does not work very well, IMO, but then neither does it work well in the existing Source Control view. We need to decide how the keys should work first as this is not at all clear to me.

But it does not look or behave nice?

That is outside my area of expertise. I can run it by our UX team if that helps?

header should look the same as other rows

Surely headers should not look the same? As long as extenders can style headers as they wish then I suppose that would be ok. I have, however, removed the 5px margin from the group headers.

expand/collapse should not cause any jumping effects

This is caused by bvaughn/react-virtualized#610

folders should have proper icons across all icon and color themes

A FileStat is now passed instead of URI to LabelProvider as per Vince's suggestion. This should fix this.

folder and files styles should be the same, i.e. folder labels should be rendered with description color in tree mode

Done

the tree state is lost while switching between flat/tree modes

vs-code loses tree state too. However vs-code expands the tree by default whereas this PR collapses by default. Perhaps I will run this by our UX team.

do a change in .theia/settings.json, try to discard them in tree mode on different levels: section header, folder and file. For me it works only on section header level.

I missed this comment, still need to fix it...

navigation left/right does not as before

it should iterate through all changes in the file, not jump to next file immediately

in flat mode it does not go anywhere sometimes

Yes, more work is needed here, though the details of how this is expected to work is not clear to me. Currently the selected change in each file is separately remembered, so as one cursors right down the file tree and into the file, it is very confusing.

what open file supposed to do for folders? it should be removed

This has now been removed

BTW could you file a separate issue for the diff widget?

#7616

akosyakov · 2020-04-22T09:19:51Z

Perhaps I will run this by our UX team.

I think it is good idea. I can check then your proposal with @svenefftinge and we decide?

Although could not we just align it as it on master, i.e. it should iterate through file changes, if a file does not have anymore changes, then jump to next/previous file. The tree should only reflect it. It is important for us in Gitpod in the PR context.

It does not seem to work like that with the recent changes as well. It only iterates only changes in a single file.

akosyakov · 2020-04-22T09:34:37Z

This is caused by bvaughn/react-virtualized#610

Can we do something about it? It looks very disturbing.

vs-code loses tree state too. However vs-code expands the tree by default whereas this PR collapses by default. Perhaps I will run this by our UX team.

Let's expand i then.

akosyakov · 2020-04-22T09:35:27Z

I cannot open diff editors with enter always.

westbury · 2020-04-26T22:59:26Z

I cannot open diff editors with enter always.

I've changed this so that the enter is processed by TreeWidget when on folder (opens folder) and opens diff when on file resource (as Source Control view previously did).

westbury · 2020-04-26T23:05:15Z

This is caused by bvaughn/react-virtualized#610

Can we do something about it? It looks very disturbing.

I removed the css for the group header that controlled the height of the header (which was carried over from the original Source Control view). By making the height the same the jump is no longer seen. If extenders were to customize the header rows to have a different height then they can turn off react-virtualized.

vs-code expands the tree by default whereas this PR collapses by default.

Let's expand i then.

Now expanded by default. I have put in an option so it is easy for extenders to collapse by default (as one of the reasons for implementing this PR was because large change sets created too many rows which locked things up)

westbury · 2020-04-26T23:11:48Z

I think it is good idea. I can check then your proposal with @svenefftinge and we decide?

Although could not we just align it as it on master, i.e. it should iterate through file changes, if a file does not have anymore changes, then jump to next/previous file. The tree should only reflect it. It is important for us in Gitpod in the PR context.

It does not seem to work like that with the recent changes as well. It only iterates only changes in a single file.

I think it now works as well as can be. See comments in code for scm-tree-widget handleLeft and handleRight. I presume to be a bug the issue where cursor left and right between files goes not to first/last chuck in next/previous file as one would expect but goes to the current chunk selection in that file. This is the behavior on master so I have left it as is. It can be changed in another PR if necessary.

akosyakov · 2020-04-27T07:16:36Z

@westbury It works quite well for me now. I've noticed 2 things only so far:

section actions are not rendered like before:
Before:
After:
mode toggle disappears

packages/scm/src/browser/style/index.css

akosyakov · 2020-04-27T07:20:49Z

packages/scm/src/browser/style/index.css

@@ -190,8 +198,6 @@
 }

 .theia-scm .scmItem:hover {
-    background-color: var(--theia-list-hoverBackground);


Why did we remove VS Code theming? Did you check that VS Code don't apply it? The same for scm-theia-header:hover

This is necessary because otherwise the theming from the original source control view messes with the tree selection and looks awful.

Could you file a follow-up issue? We need to double check how it should be applied and do it. But I don't want to hinder this PR with it.

packages/scm/src/browser/scm-frontend-module.ts

akosyakov · 2020-04-27T07:29:50Z

packages/scm/src/browser/scm-service.ts

@@ -107,14 +105,4 @@ export class ScmService {
        return repository;
    }

-    protected updateContextKeys(): void {


Did you test VS Code extensions? I.e. that commands are available in the command palette properly. The PR seems to change the semantic when we assign it. We need to test that it still confirms to VS Code extensions' expectations.

It seems to work. I had to hack the builtin I built myself to check the new menu location but it would be good to see the folder menu working when the 1.39 builtin works. I left the 'string' URI though I don't know if it is used.

azatsarynnyy · 2020-04-27T18:00:50Z

I've used the one published on Open VSX registry https://open-vsx.org/api/vscode/git/1.39.1/file/vscode.git-1.39.1.vsix

I've just tried that one with master branch and I see the same blank source control view there. Is this extension working in master for you?

@westbury in master it displays the warning message and also doesn't work

akosyakov · 2020-04-28T07:32:47Z

What is the summary? We don't test vs-code built-in git support for now or someone to test https://registry.npmjs.org/@theia/vscode-builtin-git/-/vscode-builtin-git-0.2.1.tgz at least?

#7505 (comment)

@westbury Do you know this yellow box warning was coming from our widget?

packages/scm/src/browser/scm-widget.tsx

packages/scm/src/browser/scm-commit-widget.tsx

azatsarynnyy · 2020-04-28T08:33:03Z

What is the summary?

@akosyakov from #7505 (comment) I see that VS Code built-in git extension, which is currently used by Che, works w/o any regression with this PR changes.
And https://open-vsx.org/api/vscode/git/1.39.1/file/vscode.git-1.39.1.vsix doesn't work on both: master and with this PR. There's almost the same blank source control view.
So, the PR doesn't affect Che. @vinokurig correct me if I'm wrong, please. Thx!

westbury · 2020-04-28T08:36:07Z

@westbury Do you know this yellow box warning was coming from our widget?

Yes. This is shown when there is no selected repository. In this case we are not getting any repositories found by any providers because the builtin, for whatever unknown reason, is not finding any.

akosyakov · 2020-04-28T10:27:23Z

Yes. This is shown when there is no selected repository. In this case we are not getting any repositories found by any providers because the builtin, for whatever unknown reason, is not finding any.

@westbury I meant this box is displayed on master, but not with this branch. I think it is caused by #7505 (comment) Could you have a look at comments please?

I've tested today the diff view as well. It does not seem to be affected as well.

westbury · 2020-04-28T23:59:17Z

section actions are not rendered like before:

That was because the notification-count-container, when in a tree-widget, was explicitely hidden by css in search-in-workspace. I assume that the intent was that it should only have been hidden for search-in-workspace, not globally, so I've refined the rule with an additional class.

mode toggle disappears

Sorry. Last minute cleanup that broke it. I reverted that. The code in scm-contribution to get the ScmWidget from the ViewContainer is necessary because it is not otherwise passed on to ScmWidget without changing ViewContainer.

akosyakov · 2020-04-29T08:05:25Z

@westbury I will wait till next release is through and then test again (hope last time). I believe the release should be tomorrow evening. @marcdumais-work ?

akosyakov · 2020-04-29T08:13:35Z

btw the build does not compile on travis for any platform

akosyakov · 2020-04-30T07:21:41Z

There is no commit input anymore and amend is also gone:

I've tried to reset layout, but it does not help.

packages/plugin-ext/src/main/browser/menus/menus-contribution-handler.ts

Signed-off-by: Nigel Westbury <nigelipse@miegel.org>

westbury · 2020-04-30T23:00:41Z

There is no commit input anymore and amend is also gone

I have not been able to reproduce this.

akosyakov · 2020-05-01T07:16:05Z

@westbury It seems something was cached in my browser, cleaning caches and reload helped.

akosyakov · 2020-05-01T07:19:01Z

Is there a preference to configure it? If I want to enable tree mode for my user always? It would be good to reuse from VS Code: https://github.com/microsoft/vscode/blob/8dff64169dfc8170e28ee59f0131001ad6e43954/src/vs/workbench/contrib/scm/browser/scm.contribution.ts#L138

akosyakov

Thank you, that's very cool feature! ❤️ Hope we will have it for the diff view one day too.

westbury · 2020-05-01T08:44:51Z

Thank you @akosyakov for reviewing this. Based also on Eclipse Che's testing of the builtins (#7505 (comment)), I'm merging.

akosyakov · 2020-05-01T08:59:21Z

@westbury Could you file a follow-up for #7505 (comment) please?

westbury mentioned this pull request Apr 7, 2020

Move 'last commit' and amend support so still there when using git vscode built-in #7152

Open

akosyakov added the scm issues related to the source control manager label Apr 8, 2020

westbury force-pushed the scm-tree branch from 4c22254 to 0574f8e Compare April 13, 2020 13:52

westbury marked this pull request as ready for review April 13, 2020 14:07

akosyakov requested review from vinokurig, elaihau, lmcbout and vince-fugnitto April 15, 2020 12:52

vince-fugnitto reviewed Apr 15, 2020

View reviewed changes

westbury force-pushed the scm-tree branch from 0574f8e to a6a3905 Compare April 20, 2020 08:30

westbury mentioned this pull request Apr 20, 2020

The Git diff editor should show the changed files as a tree #7616

Closed

westbury force-pushed the scm-tree branch from 6e8a111 to 1634bca Compare April 26, 2020 22:56

akosyakov reviewed Apr 27, 2020

View reviewed changes

packages/scm/src/browser/style/index.css Outdated Show resolved Hide resolved

akosyakov reviewed Apr 27, 2020

View reviewed changes

packages/scm/src/browser/scm-frontend-module.ts Outdated Show resolved Hide resolved

akosyakov reviewed Apr 27, 2020

View reviewed changes

akosyakov reviewed Apr 28, 2020

View reviewed changes

packages/scm/src/browser/scm-widget.tsx Outdated Show resolved Hide resolved

akosyakov reviewed Apr 28, 2020

View reviewed changes

packages/scm/src/browser/scm-commit-widget.tsx Outdated Show resolved Hide resolved

akosyakov reviewed Apr 28, 2020

View reviewed changes

packages/scm/src/browser/scm-commit-widget.tsx Outdated Show resolved Hide resolved

westbury force-pushed the scm-tree branch from 1634bca to d611dd6 Compare April 28, 2020 23:50

westbury mentioned this pull request Apr 29, 2020

SCM View Should Allow Multi-Select #7659

Closed

westbury force-pushed the scm-tree branch from d611dd6 to dd852e4 Compare April 29, 2020 14:05

akosyakov reviewed Apr 30, 2020

View reviewed changes

packages/plugin-ext/src/main/browser/menus/menus-contribution-handler.ts Show resolved Hide resolved

westbury force-pushed the scm-tree branch from dd852e4 to afb4558 Compare April 30, 2020 22:13

Show file changes as a tree in Source Control view

01377bd

Signed-off-by: Nigel Westbury <nigelipse@miegel.org>

westbury force-pushed the scm-tree branch from afb4558 to 01377bd Compare April 30, 2020 22:17

westbury mentioned this pull request Apr 30, 2020

support scm/resourceFolder/context menu contribution #6915

Closed

akosyakov approved these changes May 1, 2020

View reviewed changes

westbury merged commit 87677a2 into eclipse-theia:master May 1, 2020

westbury deleted the scm-tree branch May 1, 2020 08:45

westbury mentioned this pull request May 1, 2020

support search in scm view #5413

Closed

westbury mentioned this pull request May 1, 2020

Source Control view should have preference for default mode ('tree' or 'flat') #7713

Closed

Show file changes as a tree in Source Control view #7505

Show file changes as a tree in Source Control view #7505

Conversation

westbury commented Apr 6, 2020 • edited Loading

What it does

What will not be covered by this PR

How to test

Review checklist

Reminder for reviewers

akosyakov commented Apr 8, 2020 • edited Loading

westbury commented Apr 13, 2020 • edited Loading

akosyakov commented Apr 15, 2020 • edited Loading

akosyakov commented Apr 15, 2020 • edited Loading

akosyakov commented Apr 15, 2020

vince-fugnitto left a comment • edited Loading

Choose a reason for hiding this comment

westbury commented Apr 20, 2020

westbury commented Apr 20, 2020 • edited Loading

akosyakov commented Apr 22, 2020 • edited Loading

akosyakov commented Apr 22, 2020

akosyakov commented Apr 22, 2020

westbury commented Apr 26, 2020 • edited Loading

westbury commented Apr 26, 2020 • edited Loading

westbury commented Apr 26, 2020

akosyakov commented Apr 27, 2020

akosyakov Apr 27, 2020

Choose a reason for hiding this comment

westbury Apr 29, 2020

Choose a reason for hiding this comment

akosyakov Apr 30, 2020

Choose a reason for hiding this comment

akosyakov Apr 27, 2020

Choose a reason for hiding this comment

westbury Apr 29, 2020

Choose a reason for hiding this comment

azatsarynnyy commented Apr 27, 2020

akosyakov commented Apr 28, 2020

azatsarynnyy commented Apr 28, 2020

westbury commented Apr 28, 2020

akosyakov commented Apr 28, 2020

westbury commented Apr 28, 2020

akosyakov commented Apr 29, 2020

akosyakov commented Apr 29, 2020

akosyakov commented Apr 30, 2020

westbury commented Apr 30, 2020

akosyakov commented May 1, 2020

akosyakov commented May 1, 2020

akosyakov left a comment

Choose a reason for hiding this comment

westbury commented May 1, 2020

akosyakov commented May 1, 2020

westbury commented Apr 6, 2020 •

edited

Loading

akosyakov commented Apr 8, 2020 •

edited

Loading

westbury commented Apr 13, 2020 •

edited

Loading

akosyakov commented Apr 15, 2020 •

edited

Loading

akosyakov commented Apr 15, 2020 •

edited

Loading

vince-fugnitto left a comment •

edited

Loading

westbury commented Apr 20, 2020 •

edited

Loading

akosyakov commented Apr 22, 2020 •

edited

Loading

westbury commented Apr 26, 2020 •

edited

Loading

westbury commented Apr 26, 2020 •

edited

Loading