Use ScmTreeWidget in both commit details and in the diff widget #8084
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
westbury
added
git
1 task
akosyakov
reviewed
Jun 29, 2020
akosyakov
reviewed
Jun 29, 2020
akosyakov
reviewed
Jun 29, 2020
akosyakov
reviewed
Jun 29, 2020
packages/git/src/browser/history/git-history-frontend-module.ts
Outdated
Show resolved
Hide resolved
akosyakov
reviewed
Jun 29, 2020
|
Navigation in the commit detail view is broken. It should open a widget then split to right and don't give focus, otherwise there is no way to navigate between changes. |
|
I am not sure about removing arrows. I don't think that from user perspective these 3 views are the same, only from implementation. It seems that to simplify implementation we start removing user features. |
That's no problem. I'll just add an option to ScmTreeWidget to have or not have the arrows. I just didn't know the history of the arrows and why they were used in one place and not another. |
|
Should the arrows go in the view toolbar, next to the buttons that switch the tree/list mode, or stay where they were? |
If you leave them as before it should be fine. We have Gitpod users with muscle memory to have them there while reviewing PRs via Gitpod. |
I've now fixed this so that the appropriate editor widget location is used.
I know you suggested putting back the arrows in the same place as before, i.e. to the right of the group header. However it was becoming messy to do that. I remember the very first time I saw the Diff view and my immediate reaction at the time was that the two arrow buttons looked out of place. The more I think about it the more it seems to me much cleaner with the other toolbar buttons. They're not moved that far, not far enough for anyone to have to re-learn. So, sorry, that's where they are for now. All the other comments have also been addressed. |
akosyakov
reviewed
Jul 8, 2020
akosyakov
reviewed
Jul 8, 2020
|
How well this PR is compatible with old layout format for changed widgets? |
|
The diff widget does not work for me. I've tried to compare scm package with master in the tree mode. Navigation does not work for all files and the diff widget does not show any differences. Arrows in the toolbar looks fine to me. |
|
I've tried again. Navigation does not work sometimes, i.e. cannot move right from following positions. Backend logs are full of errors: Errors |
Yes, I saw something odd going on and I've now had a chance to investigate. The problem occurs for 'Rename' and 'Copy' changes. The commit in this PR has both so is an excellent test case for this. There are actually two parts to the problem.
|
|
@westbury It would be a regression, on master I can iterate over all files in the diff view. I've checked that it works in Gitpod Theia as well which is like one month old. I wonder why we need such changes with this PR then it worked before. Could you check please why it works on master?
That can be a good idea generally, but you are right we will need to analyse other clients. |
|
I've also noticed that |
1 task
|
@westbury Do you think you will have time this week to look into it again? If not would you mind If i rebase and try to fix memory leak and issues with navigation? |
I can't reproduce that. It cursors through added files in both directions. |
It is. There is some caching, but for other views such as FileTreeModel, although I see instances remain, the number match the number of widgets. GitDiffTreeModel instances remain without GitDiffWidget. I'll investigate further. |
Looks like it's the event firing on each 'children' change. The model builds quickly but it is slow setting the root. It's quick when restoring the tree from store, so I will need to investigate further. I should get time to investigate tomorrow. |
|
The leak of the GitDiffTreeModel objects was caused by what appears to be a bug in Inversify, or perhaps Inversify is working as designed. I have pushed a commit with changes that avoid the problem. I intended to look at the performance issue too today but unfortunately did not spend much time on this today. |
|
Removing the forceUpdate on tree change, which was not necessary, speeds up the switching from list to tree mode (as the tree change event is called for every node with children even though the new root is set as a single operation). I have been using this feature for a while to compare code and I have not seen navigation issues (other than issues that exist in master). Is there anything more that needs doing before merging this? |
Is it some special case?
Not sure what caused them.
|
The problem was caused because the diff was going to HEAD, not the working tree. This resulted in files in the change list that opened diffs that did not have any changes. The cursoring could not cope with this. The problem was introduced when the different getUriToOpen implementations were merged.
This is also caused by diffs using HEAD. Added files do not exist in HEAD, hence this error. I've pushed a commit with the fix. |
This is not new. I've been seeing these for months. Look in any Travis build and you'll see these.
I had been able to see these, but the retainers listed by Chrome were all over the place. Now I can't reproduce the problem at all. Note that one often gets a duplicate of GitDiffWidget, but then I see that duplicated in master too, and I see other widgets unexpectedly duplicated. |
|
@vince-fugnitto Would you be able to test it? |
Sure! I'll test it shortly. |
|
@vince-fugnitto I just wanted to check if you still have time to test this. |
Unfortunately I did not have the time to test the pull-request thoroughly due to internal priorities but I will try my best to review. |
There was a problem hiding this comment.
I'll update this comment with my findings as I test, the changes overall look very good 👍
Possible Future Improvements
I noticed that opening changes from the tree does not necessarily respect the functionality we have regarding opening files too large, or incorrect file types. This may be something to improve in the future as the application goes offline and is difficult to recover (unless I force quit and restart).
For newly added files (A) should their diff not be completely positive (green) when the file was first added?
Example from vscode (different extension but same general idea):
CHANGELOG.md
Outdated
| @@ -163,6 +163,7 @@ | |||
| <a name="1_4_0_deprecate_languages"></a> | |||
| - [[languages]](#1_4_0_deprecate_languages) `@theia/languages` extension is deprecated, use VS Code extensions to provide language smartness: | |||
| https://code.visualstudio.com/api/language-extensions/language-server-extension-guide [#8112](https://github.com/eclipse-theia/theia/pull/8112) | |||
| - [git] the changes in the commit details (opened from the history view) and in the diff view (opened with 'Compare With...' on a folder's context menu) are now switchable between 'list' and 'tree' modes | |||
There was a problem hiding this comment.
The changelog entry will need to be updated to the next release (not 1.4.0 as it is now).
This PR does not change that. Personally I prefer the behavior in Theia to the behavior in vscode. I get twice the width to see added and deleted files and it is easier to read the code.
I've pushed a commit which moves the entry within the changelog. |
Agreed, that's why I mentioned it under 'possible future improvements'. I think the behaviour is vscode is more consistent with other apps and tools (even github displays new files as newly added lines). If users want more space they can use
👍 It would be good for others to review as well if possible. |
Signed-off-by: Nigel Westbury <nigelipse@miegel.org>
|
@vince-fugnitto I've rebased to resolve the changelog conflicts. I think it is hard to find a third person to test this because it does not affect those who use the vs-code git builtin. If there are no issues that need to be fixed can it be approved based on testing by @vince-fugnitto and @akosyakov ? Of course I do need to get back to #7976 so these Git diffs can be included when using the vs-code builtin. |
vince-fugnitto
approved these changes
Sep 29, 2020
There was a problem hiding this comment.
@westbury I verified the changes once more and they work well for me 👍
I would have preferred if others would review as well but I believe we all migrated to using the builtin git extensions instead.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #7616
What it does
This PR updates both the following to use ScmTreeWidget.
This provides a more consistent view of resource changes, regardless of whether they are uncommitted, in a commit, or are a result of a diff between any two versions of a folder. Note that these widgets are provided by Theia's Git package, so not available if using the VS Code Git Builtin.
Some additional functionality comes along with this:
The Diff widget had two buttons, <- and ->, to step through changes within a file. Those buttons no longer exist and one now uses the cursor left and cursor right keys to step through changes. That brings it in line with the Source Control view and is also, with this PR, now the behavior in the Commit Details widget. If anyone wants to keep those buttons then that is fine (let me know) but we should probably also added them to both the Source Control and the Commit Details view.
Note that NavigableListWidget is now used only by the History widget. When the History is incorporated into the Timeline then NavigableListWidget will go away.
Note there is a known bug in this PR. If one opens a Diff on a folder, then makes a change to the files or staging that would change the Diff, the Diff widget does not immediately update. If one resizes the Diff widget then the refresh occurs. The forceUpdate is being done in ScmTreeWidget so I suspect this is some weird interaction with ReactVirtualized. I'm hoping someone will know what the cause may be, otherwise I will spend more time looking at it.
How to test
Review checklist
Reminder for reviewers