Feature/buffers menu #1334

akinsho · 2018-01-21T23:52:02Z

This PR adds a open buffers menu, which fixes #1173,

Changes:

Added a oni-plugin-buffers plugin and registered associated commands
Created an inactiveBuffer class to ensure active and inactive buffers are passed in (bufferManager only had access to the visited buffers)
Changed menu styling to prevent text wrapping if long item name

Todo

Fix openFile typings (oni-api)
Add commands to open splits, tabs and delete buffers
Add InactiveBuffer to Oni api and reference Oni.InactiveBuffer
Add metadata field to menuItem to store original value of absolute file path
Fix buffer population - buffersById in bufferManager does not reflect buffers in vim.
Truncate filenames

add inactive buffers to buffer manager add inactive buffer class which is distinct from buffer class add icon class for file fn for menu icons

experiment with command to delete buffer

use this as buffer source as bufbyId is inaccurate

bryphe · 2018-01-22T17:21:45Z

vim/core/oni-plugin-buffers/index.js

+    const bufferMenuItems = buffers.map(b => ({
+      label: `${active === b.filePath ? b.id + ' %': b.id}`,
+      detail: truncateFilePath(b.filePath),
+      icon: Oni.ui.getIconClassForFile(b.filePath),


Thanks for hooking up the file icons here!

bryphe · 2018-01-22T17:22:24Z

vim/core/oni-plugin-buffers/index.js

+
+  const deleteBuffer = menu => {
+    if (menu.selectedItem) {
+      //TODO: Command to execute buffer delete by Neovim


We could add a buffer.delete command to NeovimEditorCommands, which has access to the neovimInstance to execute the command directly.

bryphe · 2018-01-22T17:24:05Z

vim/core/oni-plugin-buffers/index.js

@@ -0,0 +1,80 @@
+// @ts-check
+const path = require('path');


Thanks for creating this as a plugin! Great exercise for our plugin API.

bryphe · 2018-01-22T17:28:09Z

Change is looking great to me. Stoked to have this in - lots of time where I'd find this functionality useful.

Could we add a default binding for this as <Control+Tab> in our default key bindings? I think that's similiar to what Atom/VSCode have for similiar features.

Regarding this todo:

Add commands to open splits, tabs and delete buffers

One plan I had for that was to add an optional argument to the openFile API, which would be a split option:

'horizontal'
'vertical'
'newtab'
Something like that, at least - but I think that could help here, because we could hook up commands to open in splits/tabs, and then just use different arguments for editor.openFile(file, splitOption).

akinsho · 2018-01-22T17:40:55Z

@bryphe thanks 😄 .
I can add a default binding and I like the optional arg for openFile although tbh I can't actually find how that command works as on the activeEditor the method is as follows:

public openFile(file: string): Promise<Oni.Buffer> {
        Log.warn("Not implemented")
        return Promise.resolve(null)
 }

So not sure how this is finally resolved to open the buffer.

EDIT: Found it wasn't sure how it was hooking up to the active editor but found the function definition.

Also re. where to add the buffer delete would this be to the activeEditor like or to the NeovimEditorCommands as that file seems to relate to the autocompletion menu?

bryphe · 2018-01-22T18:00:28Z

Ah ya, sorry, I guess we'll have to implement this. I was thinking something like:

public openFile(file: string): Promise<Oni.Buffer> {
        await this._neovimInstance.command(":e " + file)
        return this.activeBuffer
 }

And if we had a second argument like openDirection: string, we could do other stuff instead of :e - like :vsp (vertical split), :sp (horizontal split), :tabball (new/existing tab).

Also re. where to add the buffer delete would this be to the activeEditor like or to the NeovimEditorCommands as that file seems to relate to the autocompletion menu?

I'm starting to move some editor-specific commands from the global commands to the editor instance, to give us a better separation (and, in preparation of hosting multiple editor instances). It's in #1316 , so you can at least see how I'm starting that.

If it's easier to make a global command for this PR, that's fine too.

akinsho · 2018-01-22T18:51:16Z

@bryphe Ideally I'd prefer to add the change were it'd be most useful long term although an issue I faced having just tried this out is that it doesn't actually read the non-implemented function understandably, can you point me towards how to get it to look at the correct function? I'm not clear on how it calls the function in neovim,

From #1316 seems there are a few changes that will be happening do I need some of the larger changes to be incorporated for this to work or is there a smaller change I can make to get this working. its all in all a small change that can easily be copy/pasted over later either way

bryphe · 2018-01-22T19:12:14Z

Ideally I'd prefer to add the change were it'd be most useful long term although an issue I faced having just tried this out is that it doesn't actually read the non-implemented function understandably, can you point me towards how to get it to look at the correct function? I'm not clear on how it calls the function in neovim,

Hmm, is this regarding openFile?

From #1316 seems there are a few changes that will be happening do I need some of the larger changes to be incorporated for this to work or is there a smaller change I can make to get this working. its all in all a small change that can easily be copy/pasted over later either way

Ah I was just trying to give you some visibility in terms of how I pictured things changing (since right now, most commands are 'global'). But for this, feel free to create a global command or whatever is easier. As you mentioned, it's a pretty small tweak to move it around.

add buffer delete command

akinsho · 2018-01-22T20:04:53Z

@bryphe is it possible currently in Oni to create a binding that only applies when a menu is open?. I realise that I've now added several commands all of which can eat up bindings and are only valid when the menu is open

bryphe · 2018-01-24T01:38:11Z

@bryphe is it possible currently in Oni to create a binding that only applies when a menu is open?

Yep! That's the third argument to bind:

        input.bind("<m-t>", "language.symbols.workspace", () => !menu.isMenuOpen())

The last argument is a filter - so in this case, the binding is only active when a menu is not open.

We have other ones, like to only make the binding available for normal mode:

           input.bind("<S-C-P>", "commands.show", isNormalMode)

Hope that helps!

And I saw you were waiting on the change for oni-api, I just brought that in/bumped the version/will release. Can't wait for this feature! 👏

akinsho · 2018-01-25T20:51:35Z

@bryphe thanks for explaining this PR I think is almost done although I'd like to hide the implementation details about passing in a third argument for the menu bindings from the user is it possible to create a command that has already taken care of these details for the user from the plugin makers perspective

I know that there is commands file which has done that for certain command so would I need to add a command there or is there a way using the plugin API to accomplish this

bryphe · 2018-01-26T02:07:13Z

@bryphe thanks for explaining this PR I think is almost done although I'd like to hide the implementation details about passing in a third argument for the menu bindings from the user is it possible to create a command that has already taken care of these details for the user from the plugin makers perspective

Awesome! Yes, I agree, it's simpler to just have the binding be enabled when it makes sense, and not other times.

There's a couple different ways to accomplish this:

When you register your command, you can pass in an enabled callback - that makes the command only enabled in specific cases. Even if the user has bound to it, it will only get executed if it's actually enabled. That's how the context menu bindings work, for example.
You can also optionally return a result from the command itself - if you return false that means it was unhandled, and it should let the input manager try other bindings.

I like the explicit enabled callback for the command.

bryphe · 2018-01-26T02:09:22Z

vim/core/oni-plugin-buffers/index.js

+  Oni.commands.registerCommand({
+    command: 'bufferlist.vsplit',
+    name: 'Vertical Split Selected Buffer',
+    execute: () => openBuffer(menu, 'vertical'),


This is where I was mentioning you can add an enabled filter - like - so you could bake in the filter to the command, instead of having the user needing to know to apply the filter. It definitely is more convenient to do it here, so that we always do the 'right' thing - and the user doesn't need to think hard about it.

bryphe · 2018-01-26T02:23:02Z

Just tried this out, looks awesome! 💯 I'll definitely be using this.

Great work, @Akin909 !

akinsho · 2018-01-26T14:00:24Z

Thanks @bryphe it's almost there just tried the filter method you recognised but ran into a type error with openFile so I've made a PR to the oni-api

bryphe · 2018-01-26T15:17:10Z

Just pulled in the new oni-api typings - thanks for fixing those!

akinsho · 2018-01-26T21:35:30Z

@bryphe all done with this now thanks for merging the oni-api PR, I just wanted to draw your attention to 2 changes I made (aka to oni autocommand hooks).

Whilst trying to hook up the buffer delete command I realised that currently in order to properly respond to a buffer being deleted oni is entirely dependent on a subsequent bufEnter action. I tried to fix this by adding 2 events to the neovimEditor - BufDelete which we had previously but I removed at some juncture as it actually occurs before the buffer is deleted and BufUnload. I realised that BufWipeout (which is being used currently) and BufUnload are called not entirely consistently or rather seemingly irregularly based on some vim-logic im unaware of aka sometimes BufWipeout which should be called after BufDelete is occasionally not called, and BufUnload I think is more consistently called, though None of these commands are actually an accurate hook for detecting After a buffer has been deleted.

I currently have no idea how to do this without forcing a bufEnter event which is what I do to ensure the mapping works, (or manually filtering out the deleted buffer which we discussed previously). I've left them in as I believe they can be useful hooks to have for some other Oni purpose rather than deleting them just to have to write them in again at some future date.

EDIT: My current solution has the undesirable side-effect of if a user closes a buffer which isn't the current buffer as I force a bnext command they are navigated away from their current buffer ☹️ not sure of a way around it and personally for the ability to remote close a buffer its a price I'd personally pay in the interim

bryphe · 2018-01-26T22:01:07Z

browser/src/Services/EditorManager.ts

-        Log.warn("Not implemented")
-        return Promise.resolve(null)
+    public async openFile(file: string, method = "edit"): Promise<Oni.Buffer> {
+        const cmd = new Proxy(


Could we have this call into the activeEditor's openFile method, as opposed to duplicating the logic in NeovimEditor's openFile method? I'd like for the EditorManager to be agnostic to Neovim, if possible - ideally, the EditorManager could support all sorts of IEditor implementations - even ones that didn't depend on Neovim.

bryphe · 2018-01-26T22:02:03Z

vim/core/oni-plugin-buffers/index.js

+            // This line forces vim to navigate to the next buffer after deleting
+            // NOTE: This is essential as otherwise Oni does not properly register the buffer
+            // delete event
+            await Oni.editors.activeEditor.neovim.command("bn")


If this will always need to be done along with the bufferDelete, perhaps we should just push this inside the bufferDelete method?

In addition - can we check if the current buffer's id (activeEditor.activeBuffer.id) is equal to the one we're deleting? If I understand correctly, we only need to do the bn if the buffer is the same, so if activeEditor.activeBuffer.id === menu.selectedItem.metadata.id.

akinsho · 2018-01-26T22:03:00Z

Actually found an unusual bug, it seems that using the filterFunction messes with the meta data fields I've added to the menu Items which the plugin depends on for its functionality 🙁 , I'll look into that report back once it's sorted out

bryphe · 2018-01-26T22:04:22Z

vim/core/oni-plugin-buffers/index.js

+
+    Oni.commands.registerCommand({
+        command: "bufferlist.delete",
+        name: "Delete Selected Buffer",


Could we add a common 'prefix' for these, like:

name: "Buffer: Delete", detail: "Delete selected buffer", ``` Trying to organize the commands a bit 😄

@bryphe did you intend for the commands to be in caps with the space there, I've changed the command to be buffer:action as from looking at the commands none start with caps and none have spaces in them ❓

Yeah, no set standard yet, but here's what I'm thinking:

command: camel case / lowercase - ie, workspace.openFolder

name: noun + verb, capitalized - Workspace: Open Folder

detail: sentence case - Set a folder as the current workspace
(not a great example, but hopefully that helps).

I think the commands you have look good - I was thinking for the name/detail fields.

Like looking at the command palette we have today:

The 'Workspace' ones look good to me, but the 'Show Errors' looks like it's just kind of floating. It'd be nice to group it with some other error/diagnostic related commands (maybe like Errors: Show, Errors: Go to next, Errors: Go to previous, etc..)

bryphe · 2018-01-26T22:06:20Z

Thanks for the details, @Akin909 ! I left a few comments on the PR. I'm not exactly sure how the buffer lifetime works, or why'd we see inconsistent results.

The buffer navigation seems like a reasonable workaround, but I'm wondering - can we check the case where the deleted buffer is equal to the current buffer, and conditionally execute bn? Or do we always need to execute bn? If it does always need to be executed to get the latest list - could we do bn/bp/ in the case where we're not on the buffer we're deleting?

akinsho · 2018-01-26T22:18:45Z

Tbh I'm sure there's some reasoning there somewhere or a way around this issue but from reading the vim docs and autocommand hooks I'm not clear on where is the earliest correct place to hook into following a buffer being delete aka what action is unconditionally always run following a BufDelete when the buffer has definitely already been unloaded, but currently given up (at least for this PR)

As for the workaround I could pass in % as the buffer to be deleted and if it isn't the current buffer execute a bn/bp otherwise just a bn, although as what I hope is a temporary solution.

Also looking at the comments and will make those changes

add buffer navigation following deletion rename commands

fix comment typo

akinsho · 2018-01-26T23:27:44Z

All done 👍 I've changed the command names, moved the post deletion navigation to the command itself, and removed the openFile implementation to the neovimEditor, as well as fixed the metadata bug

bryphe · 2018-01-27T03:47:47Z

Looks great, @Akin909 ! Bringing this in now. Really excited to start using this and have this part of the next release. Thanks for the contribution!

akinsho added 7 commits January 21, 2018 22:40

add buffer menu plugin

13d2feb

add inactive buffers to buffer manager add inactive buffer class which is distinct from buffer class add icon class for file fn for menu icons

prettify buffers-menu index

67ed985

experiment with command to delete buffer

add indicator for current and tweak styles to menu nowrap

327ee3b

recreate inactive buffers on enter and remove max-width

15aefa3

rename addInactiveBuffers to populateBuffers

d610376

use this as buffer source as bufbyId is inaccurate

truncate filenames

817abb9

remove unnecessary loading false call

e86e4ef

bryphe reviewed Jan 22, 2018

View reviewed changes

akinsho added 2 commits January 22, 2018 19:14

register different buffer open commands

739cc45

add proxies to handle incorrect input for file open

7b993cf

add buffer delete command

add extra check in item selected to ensure menu item

9926759

akinsho added 3 commits January 25, 2018 19:53

use inactive buffer from oni type

6252a36

merge upstream fix conflicts

2515171

fix openBuffers to work with OniEditor

64168cc

akinsho added 4 commits January 25, 2018 20:54

use already defined buffer delete command in oniEditor

51d8b21

merge upstream fix conflicts

4d57365

re-add new oni api

3d65b5f

fix keybindings typo

15d3819

bryphe reviewed Jan 26, 2018

View reviewed changes

akinsho added 2 commits January 26, 2018 08:17

add filter to bufferlist commands

32d96dc

[WIP] add filter for menu commands - need to update oni-api

e7fd336

akinsho added 2 commits January 26, 2018 14:31

update oni-api version

9a35fc0

add fixme comment to match filepath accurately

474a0d1

akinsho added 2 commits January 26, 2018 18:41

Remove bufDelete function for now as event is noop

66586dd

fix buffer delete command add new event hooks

3162586

bryphe reviewed Jan 26, 2018

View reviewed changes

akinsho added 2 commits January 26, 2018 22:50

fix metadata removal

31b25cc

add buffer navigation following deletion rename commands

fix default binding to use new command name

e07f1dd

fix comment typo

akinsho changed the title ~~[WIP] Feature/buffers menu~~ Feature/buffers menu Jan 26, 2018

bryphe merged commit 0fc0f43 into onivim:master Jan 27, 2018

akinsho deleted the feature/buffers-menu branch January 27, 2018 11:27

akinsho mentioned this pull request Feb 9, 2018

Closing tabs from command-line (ex) mode #602

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/buffers menu #1334

Feature/buffers menu #1334

akinsho commented Jan 21, 2018 •

edited

Loading

bryphe Jan 22, 2018

bryphe Jan 22, 2018

bryphe Jan 22, 2018

bryphe commented Jan 22, 2018

akinsho commented Jan 22, 2018 •

edited

Loading

bryphe commented Jan 22, 2018

akinsho commented Jan 22, 2018

bryphe commented Jan 22, 2018

akinsho commented Jan 22, 2018

bryphe commented Jan 24, 2018 •

edited

Loading

akinsho commented Jan 25, 2018 •

edited

Loading

bryphe commented Jan 26, 2018

bryphe Jan 26, 2018

bryphe commented Jan 26, 2018

akinsho commented Jan 26, 2018

bryphe commented Jan 26, 2018

akinsho commented Jan 26, 2018 •

edited

Loading

bryphe Jan 26, 2018 •

edited

Loading

bryphe Jan 26, 2018

bryphe Jan 26, 2018

akinsho Jan 26, 2018

akinsho commented Jan 26, 2018 •

edited

Loading

bryphe Jan 26, 2018

akinsho Jan 26, 2018

bryphe Jan 26, 2018

bryphe commented Jan 26, 2018 •

edited

Loading

akinsho commented Jan 26, 2018

akinsho commented Jan 26, 2018

bryphe commented Jan 27, 2018

Feature/buffers menu #1334

Feature/buffers menu #1334

Conversation

akinsho commented Jan 21, 2018 • edited Loading

Changes:

Todo

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryphe commented Jan 22, 2018

akinsho commented Jan 22, 2018 • edited Loading

bryphe commented Jan 22, 2018

akinsho commented Jan 22, 2018

bryphe commented Jan 22, 2018

akinsho commented Jan 22, 2018

bryphe commented Jan 24, 2018 • edited Loading

akinsho commented Jan 25, 2018 • edited Loading

bryphe commented Jan 26, 2018

Choose a reason for hiding this comment

bryphe commented Jan 26, 2018

akinsho commented Jan 26, 2018

bryphe commented Jan 26, 2018

akinsho commented Jan 26, 2018 • edited Loading

bryphe Jan 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akinsho commented Jan 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryphe commented Jan 26, 2018 • edited Loading

akinsho commented Jan 26, 2018

akinsho commented Jan 26, 2018

bryphe commented Jan 27, 2018

akinsho commented Jan 21, 2018 •

edited

Loading

akinsho commented Jan 22, 2018 •

edited

Loading

bryphe commented Jan 24, 2018 •

edited

Loading

akinsho commented Jan 25, 2018 •

edited

Loading

akinsho commented Jan 26, 2018 •

edited

Loading

bryphe Jan 26, 2018 •

edited

Loading

akinsho commented Jan 26, 2018 •

edited

Loading

bryphe commented Jan 26, 2018 •

edited

Loading