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

Add a version-control statusline element #5682

Merged
merged 10 commits into from
Mar 10, 2023

Conversation

dgyurov
Copy link
Contributor

@dgyurov dgyurov commented Jan 25, 2023

What is this Pull Request about?

This pull request is introducing a new statusline element capable of displaying the version control HEAD of the current workspace.

How can I try it?

The proposed statusline element is opt-in only and will require the user to add "version-control" element in their [editor.statusline] section of the user configuration file.

How does it work?

  1. If the current HEAD is a reference, it's shortened name will be displayed in the statusline.
  2. In the case of a detached HEAD, the first eight characters of the commit hash will be displayed instead.
  3. In the case of opening helix with a root folder without a git-repository, nothing will be displayed.

Visual preview

preview

UX Considerations

The current version control HEAD is displayed in the statusline for each opened document. That means that if the user opens files from different repositories in the same Helix instance, this implementation will be able to tackle that. Updates to the data feeding the statusline are only done when opening a file, or when explicitly reloading a file. The current implementation of the version control element also takes into account that in the future we will have a file watching capability in Helix, which will allow us to reactively update the data feeding the version control element.

@the-mikedavis the-mikedavis added C-enhancement Category: Improvements A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer. A-vcs Area: Version control system interaction labels Jan 25, 2023
@pascalkuthe
Copy link
Member

Thank you for working on this. I definitely want to see this feature in the future and the implementation seems decent.

Sadly this feature is still missing some prerequisites (and hence I didn't implement it myself).

Without repo caching this is sadly too slow. The status line rendering is called VERY often so not having the repo cached here causes a LOT of IO on every redraw. The problem with caching this is that a cached repo doesn't reload HEAD automatically so we need file watching too. I want to implement both of these things eventually but that will take some time. We could accept a version without file watching (but then the utility is a bit limited) but I think repo caching is a hard requirement. Performing large amounts of IO on every redraw is not something we want to ever do

@dgyurov
Copy link
Contributor Author

dgyurov commented Jan 31, 2023

@pascalkuthe This is a fantastic feedback! Thanks a lot! I thought a lot about this PR in the last couple of days and I must agree that the best way to go forward will be to have a watcher in place and reactivity update the version control element.

I had a look at how a simillar feature was implemented by the lualine.nvim project and they are following very similar approach to the one suggested by you. I do feel however that having a file watching functionality should be centralized in the codebase (simillar to Neovim), as there might be more use-cases in the future that can benefit from it.

I implemented an intermediate approach of having a version-control statusline element, which "piggy-backs" on the current git diff implementation. That is probably a reasonable MVP, as you mentioned, that will not have extra performance implications. Do you think that this will be an acceptable solution to the problem? The UX implications of it are that the statusline element will only be updated when a document is opened or when an explicit reload of the buffer is requested. Not ideal, but it might be enough and acceptable for most of the users. Personally, as a user, I would prefer to have a version control statusline element, even with the above mentioned imperfection.

Perhaps, it will be reasonable to complete this PR as a MVP first, and explore a centralized file watching solution separately? Will such iterative improvement be acceptable? Let me know what your thoughts on this are.

Thank you so much for the time again and have an awesome day! :)

@pascalkuthe
Copy link
Member

Good to see you are making progress. I didn't actually mean that you need to implement file watching. I am working on that locally and you are right a centralized solution is required. I think a MVP which doesn't change git branch automatically is required.

That being said I am not quite happy with the implementation you choosen here. I don't want to couple the git diff gutter to the statuline element at all.
I actually liked that your previous implementation had a seperate function in the DiffProvider.
That functional should simply return an Arc<ArcSwap<Arc<str>>> which will be updated by the file-watcher automatically in the future. The returned value should be stored on the document just as DiffHandle as different files can belong to different git repositories.

@pascalkuthe pascalkuthe added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Feb 1, 2023
pascalkuthe
pascalkuthe previously approved these changes Feb 5, 2023
Copy link
Member

@pascalkuthe pascalkuthe left a comment

Choose a reason for hiding this comment

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

After applying the review comments this look much better. The amaount of changes is actually quite small now, nice 👍 Just like the diff gutters there is no more IO in the rendering loop and the implementation is also future proofed so that it's easy for a background thread to update the statusline through the ArcSwap 👍.

As there is no file watching implemented the status line does not update automatically. However, I think that is actually a good think because then it is also more intuitive that the diff gutter does not update automatically either. So this status line indicator can make it more apparent what the diff is being computed against.

I think it makes sense to merge this as is and file watching in a future (separate) PR. Getting file watching right is fairly challenging, and as diff gutter where merged without that I think it's also fine to similarly accept this feature without filewatching.

@pascalkuthe pascalkuthe added S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 5, 2023
@dgyurov
Copy link
Contributor Author

dgyurov commented Feb 5, 2023

Thank you so much for the feedback! I really liked the proposed solution and iterated over my initial commit.

Now each document is caching a head name internally and there is much stricter control for opening a corresponding repository. The cache is only invalidated and updated after an explicit reload by the end user. In the future when we have a centralized implementation of a file watcher, we can watch .git/HEAD for changes and reactively update the cache.

Assigning a head to each document was a fantastic idea! It allows us to open files from different repositories in the same session and the statusline element will still work as expected. Definitely an user experience improvement. Thanks again! 🚀

@pascalkuthe pascalkuthe added the E-easy Call for participation: Experience needed to fix: Easy / not much label Feb 6, 2023
helix-view/src/document.rs Outdated Show resolved Hide resolved
helix-vcs/src/lib.rs Outdated Show resolved Hide resolved
@pascalkuthe pascalkuthe added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Feb 8, 2023
@dgyurov dgyurov requested review from archseer and pascalkuthe and removed request for archseer February 8, 2023 19:57
@pascalkuthe pascalkuthe removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 8, 2023
@pascalkuthe pascalkuthe added the S-waiting-on-review Status: Awaiting review from a maintainer. label Feb 8, 2023
helix-view/src/document.rs Outdated Show resolved Hide resolved
@dgyurov dgyurov requested a review from archseer February 9, 2023 21:20
archseer
archseer previously approved these changes Mar 8, 2023
Copy link
Member

@archseer archseer left a comment

Choose a reason for hiding this comment

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

Looks good, just needs a conflict resolution

@pascalkuthe pascalkuthe added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Mar 8, 2023
@dgyurov
Copy link
Contributor Author

dgyurov commented Mar 8, 2023

Thanks a lot! I just resolved the conflicts with the latest commit. :)

@pascalkuthe pascalkuthe added S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 9, 2023
@pascalkuthe pascalkuthe added this to the next milestone Mar 9, 2023
@the-mikedavis the-mikedavis changed the title Implements a version control element for the statusline Add a version-control statusline element Mar 10, 2023
@the-mikedavis the-mikedavis merged commit 1661e4b into helix-editor:master Mar 10, 2023
sagnibak pushed a commit to sagnibak/helix that referenced this pull request Mar 21, 2023
wes-adams pushed a commit to wes-adams/helix that referenced this pull request Jul 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements A-vcs Area: Version control system interaction C-enhancement Category: Improvements E-easy Call for participation: Experience needed to fix: Easy / not much S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants