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

Enhance branch display #260

Closed
wants to merge 8 commits into from
Closed

Conversation

Yukaii
Copy link
Contributor

@Yukaii Yukaii commented Jan 26, 2018

This implements #258

Add gitlens.gitExplorer.branches.layout config with three available option: list, tree, and mix-tree

list is the current behavior, would display all branches with full branch name:

screen shot 2018-01-26 at 11 01 05 am

tree organize branch name with slashes as folder:

screen shot 2018-01-29 at 9 13 26 pm

mix-tree sort all branches alphabetically:

screen shot 2018-01-26 at 11 14 08 am

TODOs

  • Unexpected ordering with tree layout
  • Current branch within branch folder should be at first in tree layout
    • and should be expanded by default
  • Detach state branch name shouldn't be parse as tree

@Yukaii Yukaii force-pushed the improve-branch-folder branch from 5a600ce to 6e5403a Compare January 26, 2018 08:31
@Yukaii Yukaii changed the title WIP: Enhance branch display Enhance branch display Jan 29, 2018
@Yukaii Yukaii force-pushed the improve-branch-folder branch from c9fe1fb to 360bf2e Compare January 30, 2018 01:20
- Add current getter method to BranchNode
- Implment a recursive findCurrent method on BranchFolderNode
@Yukaii Yukaii force-pushed the improve-branch-folder branch from c5ace7d to 34172e4 Compare January 30, 2018 03:16
@Yukaii
Copy link
Contributor Author

Yukaii commented Jan 30, 2018

@eamodio I think it's ready for review 😄 Ping me if any change is needed.

@eamodio eamodio self-requested a review February 2, 2018 19:37
@eamodio eamodio added this to the Soon™ milestone Feb 2, 2018
@eamodio
Copy link
Member

eamodio commented Feb 2, 2018

@Yukaii Awesome -- I'm starting to look at this. But first can you adapt this to target the develop branch? As that branch is what will become GitLens 8 -- which will be the next release. Thanks!

@eamodio eamodio changed the base branch from master to develop February 2, 2018 21:34
@Yukaii
Copy link
Contributor Author

Yukaii commented Feb 3, 2018

@eamodio I've merged the changes from develop branch 😸

"tree",
"mix-tree"
],
"description": "Specifies how the `Branches` view will display branches\n `list` - display all branches \n`tree` - organize branch as folder if branch name contains slashes \"\/\"\n `mix-tree` - display branch folders along with normal branch alphabetically",
Copy link
Member

Choose a reason for hiding this comment

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

Any objection to getting rid of mix-tree and just having tree behave this way (i.e. sorted naturally)?

Copy link
Member

@eamodio eamodio Feb 5, 2018

Choose a reason for hiding this comment

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

Or if you'd like to keep the feature, can we change it to a different setting?

Like gitlens.gitExplorer.branches.sort = 'alphabetic' | 'foldersFirst' | 'foldersLast'

or

gitlens.gitExplorer.branches.folderGrouping = 'none' | 'start' | 'end'?

Thoughts? (I think I'm leaning towards the latter -- as it is more explicit than just sort)

Copy link
Contributor Author

@Yukaii Yukaii Feb 5, 2018

Choose a reason for hiding this comment

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

I think having only two options (list and tree) is fine and simpler for user too, actually current tree sort implementation (not the mix-tree one) calculates more. It runs a recursive function, marking the parent folder as current (BranchFolderNode#findCurrent)

If you think it's okay, I can drop those commits 😄

Copy link
Member

Choose a reason for hiding this comment

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

Ok let's drop it and just have the 2 options. Thanks!

if (this.explorer.config.branches.layout === ExplorerBranchesLayout.List) {
return branchName;
} else {
return !!branchName.match(/\s/) ? branchName : this.branch.getBasename();
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand the logic here. Why are you checking if the name contains any whitespace?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In some case like detached git state, the folder name will not be parsed correctly. So here I see any white space as invalid folder branch name, and choose not to handle them. Same logic appears in branchesNode.ts:38 and remoteNode.ts:39.

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok, but why is it needed? A "branch" with spaces could be handled just the same as any other without '/' characters right? Was there an explicit case you were protecting against?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I encounter this issue with the branch name like " ✔️ (HEAD detached at xxxxxx/xxxxxx)", current implementation will split the name to

  • (HEAD detached at xxxxxx (branch folder)
    • xxxxxx) (sub-branch)

Copy link
Member

Choose a reason for hiding this comment

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

Ah -- ok, thanks

@eamodio
Copy link
Member

eamodio commented Feb 6, 2018

@Yukaii I'm trying to finalize GitLens 8 for release and as I'd like to get this in, I'm planning on just getting this in and then remove the mix-tree option in a follow-on commit -- sound ok?

@Yukaii
Copy link
Contributor Author

Yukaii commented Feb 6, 2018

@eamodio Sure 👍

@eamodio
Copy link
Member

eamodio commented Feb 6, 2018

This has been merged into develop and as long as GitLens 8 release candidate looks good this will ship in GitLens 8.

FYI, I made a bunch of changes to the behavior here: 112879d

  • I removed the mix-tree option and changed tree sorting to be similar to what mix-tree was. Although I pulled the current branch out of any nesting. Since I think it is important to keep the current branch at the top and stable.

  • I removed the folder icons in favor of using a new feature in vscode 1.20 (using the File Icon Theme). Unfortunately for vscode 1.19 there will be no icons at all, but since 1.20 is supposed to ship this week, I don't think it matters much.

  • I also added the new layout option to the new GitLens Settings page.

Thanks again for your awesome work! This is a great feature!

@eamodio eamodio closed this Feb 6, 2018
@eamodio eamodio removed this from the Soon™ milestone Jul 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants