onivim / oni

Oni: Modern Modal Editing - powered by Neovim
https://www.onivim.io
MIT License
11.34k stars 298 forks source link

Status Bar: Add git branch status #486

Closed bryphe closed 6 years ago

bryphe commented 7 years ago

Splitting out from #201 - it'd be great to see the current git branch as an optional setting in the status bar. This is one item I miss with the new status bar from #201.

Bretley commented 7 years ago

I've got some code for this, I made a component and everything, I'm just not sure where it should go. I added a method to the Git.ts that this needs to run, so it seems like oni-core-statusbar/index.js is not the right place. Any thoughts?

bryphe commented 7 years ago

Awesome! That's really great. I've been missing have my git branch in the status bar, I'd be stoked to have this back.

Just to give you an idea of my long-term vision - I'd like to split up pieces of Oni into subcomponents (kind of like how Atom does it). For example, move oni-core-statusbar to an npm package, and same with other plugins. And move some existing functionality into plugins / packages (like quickopen / fuzzy finder).

For git integration in general, I was thinking of creating an oni-core-git plugin under vim/core. To start with, this would be focused mainly on the statusbar. But then later I was planning on extending it a bit, especially to bring in things like diffing and git blame.

So if you're up for that - adding a new plugin (which you can start by copying oni-core-statusbar to a new folder oni-core-git) - that would be awesome. Otherwise, I think oni-core-statusbar is reasonable, because we can always split that out later.

Thanks for looking at it, excited to check it out. Keep me posted if you need any help.

Bretley commented 7 years ago

I meant more along the lines that I didn't know if I could put it in it's own file in there or not. I'm not sure about importing Git into index.js, doesn't seem very "decoupled" and also I don't know if javascript is just going to let me import a Typescript file...

Sorry these are just basic questions haha. I like code, not architecture

I wouldn't know how to take what I had in the index.js and port it to its own file

bryphe commented 7 years ago

Sorry these are just basic questions haha. I like code, not architecture

Haha not at all, the module / import system is pretty confusing in JavaScript as it is. Combine that with different strategies we have in TypeScript+Webpack vs JavaScript, and it is certainly a recipe for confusion.

It sounds like you're adding the functionality in Git.ts and then using it in the statusbar. You're right, you can't import that, but we can give access to it in other ways. In that case, we might want to extend the Oni object we expose to plugins to have those methods available.

Having the API available through Oni.git:

would be pretty cool / useful. We could add it on the Oni object in browser\src\Plugins\Api\Oni.ts. Let me know if you want help there.

Bretley commented 7 years ago

So I'm reading through API/Oni.ts and I don't quite understand it >_<

Am I just supposed to have Git.ts Imported in Oni.ts? I see lots of notifications getting pushed through plugin channels but that's it.

bryphe commented 7 years ago

Am I just supposed to have Git.ts Imported in Oni.ts? I see lots of notifications getting pushed through plugin channels but that's it.

Ya, it used to be that the plugins ran in a separate process - and the plugin channels were an abstraction to communicate between the processes. There are pros and cons to each approach, but we moved to an in-process model. The plugin channel is still used, but you don't really need it - it's an abstraction that's necessary for the cross-process communication, but now that all the plugins run in the same process as Oni, it's just an extra layer.

Some of the newer additions to the API don't use it - an example of a place where it isn't used is the configuration - so that might be a good model to follow for implementing Git on the Oni object.

I'd recommend doing the following, and using the Configuration piece of the API to build this out, because that's pretty similar:

Hope that helps. It's really just using the Plugins\Api\Configuration.ts as a base, which is how the configuration API is defined for plugins. It's pretty similiar because that imports Config.ts (which is the core configuration logic) - we'd just be importing Git.ts.

Once these steps are complete, you should now have Oni.git available in the plugins to call.

Let me know if you need more details on any of those steps in particular!

Bretley commented 7 years ago

I have the component made and showing, but I'm not exactly sure why it isn't working. It works if I put a default value in for the branch name, but the call to getBranch() is failing and blocking the Statusbar.

I'm going to submit a PR so you can see the code

Galbar commented 7 years ago

Adding feedback here to for visibility: https://github.com/extr0py/oni/pull/540#issuecomment-315550296

@extr0py:

  • @bert88sta 's feedback regarding additional status info (ie, files modified/uncomitted/etc) . We might be able to get some of this info from the output of the git
  • Detecting when the branch has changed, since currently we only update on entering a buffer (for example, a user changed branch in the terminal). We could potentially add an event on the Git shared object like branch-changed that the plugin could subscribe to.
  • A configuration option to enable / disable the statusbar (ie, git.statusBar.enabled)

Regarding the aditional status. I think it is a good idea but I would make it optional with a config (I don't really need that information there and it would take space in the status bar) and/or maybe make it a hover popup thing on the status bar item.

bryphe commented 7 years ago

Cheers, thanks for tracking it, @Galbar!

Regarding the aditional status. I think it is a good idea but I would make it optional with a config (I don't really need that information there and it would take space in the status bar) and/or maybe make it a hover popup thing on the status bar item.

Agree 100% - some users will prefer the simpler / less busy experience, so making the additional status optional would be ideal.

badosu commented 7 years ago

@bryphe One idea would be to split this into a Git plugin since there are many different CVSs to choose from. The Git plugin would be responsible to expose this configuration.

bryphe commented 6 years ago

👍 , yes that's a great suggestion @badosu . It'd be helpful to generalize a 'version-control-system' interface / API, and then have the git integration be a plugin that implements that (as a proof-of-concept for other systems, like subversion, etc)

bryphe commented 6 years ago

I think this is tracked by #1310 and #451, so closing this out to be tracked by those items.