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

Fix Highlighting in SCM Tree #8929

Conversation

colin-grant-work
Copy link
Contributor

@colin-grant-work colin-grant-work commented Jan 8, 2021

What it does

This PR fixes #8581 by adjusting the ScmTreeLabelProvider class to provide labels for all of the ScmTreeWidget node types and then using the searchHighlights map to produce a highlighted caption. This introduces adjustments to the interfaces of the various ScmTreeWidget node types, but those could be made non-breaking by adding | ReactNode to the previous specifications, if that is desirable.

image

How to test

  1. start the application in a source-controlled workspace
  2. perform changes and open the scm view
  3. type to start the tree-search
  4. notice that highlights are present

Review checklist

Reminder for reviewers

@colin-grant-work colin-grant-work added scm issues related to the source control manager tree issues related to the tree (ex: tree widget) labels Jan 8, 2021
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

@colin-grant-work the changes now highlight the matches in the tree based on the search query. I'll need to confirm it still works correctly using the git built-in extensions.

I did notice the following issues that I'm not sure should be handled by the pull-request:

When no nodes are present, we still highlight the tree (which looks incorrect).

scm-changes

Changes is highlighted even though it is not part of the changeset but just a category (it is also highlighted in a different color):

scm-2

packages/scm/src/browser/scm-tree-widget.tsx Show resolved Hide resolved
packages/scm/src/browser/scm-tree-widget.tsx Show resolved Hide resolved
@colin-grant-work
Copy link
Contributor Author

@colin-grant-work the changes now highlight the matches in the tree based on the search query. I'll need to confirm it still works correctly using the git built-in extensions.

When no nodes are present, we still highlight the tree (which looks incorrect).

I'm happy to handle these in the current PR, though I'm not sure what behavior is most desirable. Arguably, if there are no changes, there's no need to render the top-level node at all (similar to how the Navigator doesn't render the top workspace node if it isn't a directory) so we could:

  1. Not render a ChangeGroupNode if it has no children (make it invisible).
  2. Not highlight the ChangeGroupNode even if it matches the search (either unconditionally or on the condition that is has no children)
  3. Just remove whatever style code is making it a different color so that the highlight at least looks like the highlighting of other nodes.

I'm inclined to opt for 1 and 3. Don't render the node if it isn't informative, and if it is rendered, highlight the same as any other node. Does that sound reasonable?

@vince-fugnitto
Copy link
Member

@colin-grant-work the changes now highlight the matches in the tree based on the search query. I'll need to confirm it still works correctly using the git built-in extensions.
When no nodes are present, we still highlight the tree (which looks incorrect).

I'm happy to handle these in the current PR, though I'm not sure what behavior is most desirable. Arguably, if there are no changes, there's no need to render the top-level node at all (similar to how the Navigator doesn't render the top workspace node if it isn't a directory) so we could:

  1. Not render a ChangeGroupNode if it has no children (make it invisible).
  2. Not highlight the ChangeGroupNode even if it matches the search (either unconditionally or on the condition that is has no children)
  3. Just remove whatever style code is making it a different color so that the highlight at least looks like the highlighting of other nodes.

I'm inclined to opt for 1 and 3. Don't render the node if it isn't informative, and if it is rendered, highlight the same as any other node. Does that sound reasonable?

I know that it is done this way since it aligns with vscode, they display the changes top-level node despite no actual children (changes):

image

I think we should be consistent with the behavior present in vscode for the moment, and fix the highlighting of this node (removing the highlighting versus fixing the color).

@colin-grant-work colin-grant-work force-pushed the bugfix/highlights-in-scm branch 2 times, most recently from c0390a5 to 488db3a Compare January 12, 2021 15:43
@colin-grant-work
Copy link
Contributor Author

@vince-fugnitto, I've pushed up a version that documents the breaking (and non-breaking) changes in the CHANGELOG.md and restricts highlights to non-group nodes.

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

The changes work well for me, I now see that highlighting works well for scm.
I also verified the changes using the git built-in extensions instead of @theia/git:

"vscode-builtin-git": "https://open-vsx.org/api/vscode/git/1.39.1/file/vscode.git-1.39.1.vsix",
"vscode-builtin-git-ui": "https://open-vsx.org/api/vscode/git-ui/1.39.1/file/vscode.git-ui-1.39.1.vsix"

@colin-grant-work colin-grant-work force-pushed the bugfix/highlights-in-scm branch from 488db3a to ddaaa69 Compare January 20, 2021 17:39
@colin-grant-work colin-grant-work merged commit 47be972 into eclipse-theia:master Jan 20, 2021
@colin-grant-work colin-grant-work deleted the bugfix/highlights-in-scm branch January 20, 2021 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scm issues related to the source control manager tree issues related to the tree (ex: tree widget)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

scm: tree-search should highlight matched nodes
2 participants