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

[RDY] Add TabNew, TabNewEntered, TabClosed events #1717

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@fmoralesc
Contributor

fmoralesc commented Dec 22, 2014

TabNew triggers when entering a new tab page, but not when entering an
already created one.

EDIT: I also sent this to vim_dev.

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Dec 22, 2014

Member

It should use similar verbiage as BufNew, if it makes sense to do so.

|BufNew|        just after creating a new buffer
...
|TabNew|        just after creating a new tab

same for the long-form description.

By the way, in general, I am in favor of going nuts with new event types in Neovim.

Member

justinmk commented Dec 22, 2014

It should use similar verbiage as BufNew, if it makes sense to do so.

|BufNew|        just after creating a new buffer
...
|TabNew|        just after creating a new tab

same for the long-form description.

By the way, in general, I am in favor of going nuts with new event types in Neovim.

@fmoralesc

This comment has been minimized.

Show comment
Hide comment
@fmoralesc

fmoralesc Dec 22, 2014

Contributor

@justinmk 👍

This particular event was inspired by a question on #vim last night, where someone wanted to launch startify whenever a new tab was created. After some hacky tries using TabEnter, I decided it would be best to implement this. Compare the function using TabEnter with the one using TabNew.

Contributor

fmoralesc commented Dec 22, 2014

@justinmk 👍

This particular event was inspired by a question on #vim last night, where someone wanted to launch startify whenever a new tab was created. After some hacky tries using TabEnter, I decided it would be best to implement this. Compare the function using TabEnter with the one using TabNew.

@fmoralesc fmoralesc changed the title from Add TabNew event to [RFC] Add TabNew event Dec 22, 2014

@marvim marvim added the RFC label Dec 22, 2014

@fmoralesc

This comment has been minimized.

Show comment
Hide comment
@fmoralesc

fmoralesc Dec 23, 2014

Contributor

BM proposed to add a WinNew event too on vim_dev, I think that is a good idea, and I'll add it to this PR too.

Contributor

fmoralesc commented Dec 23, 2014

BM proposed to add a WinNew event too on vim_dev, I think that is a good idea, and I'll add it to this PR too.

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Dec 23, 2014

Member

👍

Member

justinmk commented Dec 23, 2014

👍

@fmoralesc fmoralesc changed the title from [RFC] Add TabNew event to [WIP] Add TabNew and WinNew events Dec 23, 2014

@marvim marvim added WIP and removed RFC labels Dec 23, 2014

@fmoralesc

This comment has been minimized.

Show comment
Hide comment
@fmoralesc

fmoralesc Dec 23, 2014

Contributor

The current implementation of WinNew also triggers when a new tab page is created. I'm not sure if that behavior is correct (I tend to think it's not). Ideas?

Contributor

fmoralesc commented Dec 23, 2014

The current implementation of WinNew also triggers when a new tab page is created. I'm not sure if that behavior is correct (I tend to think it's not). Ideas?

@jamessan

This comment has been minimized.

Show comment
Hide comment
@jamessan

jamessan Dec 23, 2014

Member

I would think it is. A tab page is just a container for windows (that's why you can have multiple splits in a tab page), so a newly created tab also creates a window it can use to display a buffer (which may also be new depending on what caused the creation of the tab page).

Member

jamessan commented Dec 23, 2014

I would think it is. A tab page is just a container for windows (that's why you can have multiple splits in a tab page), so a newly created tab also creates a window it can use to display a buffer (which may also be new depending on what caused the creation of the tab page).

@fmoralesc

This comment has been minimized.

Show comment
Hide comment
@fmoralesc

fmoralesc Dec 23, 2014

Contributor

Thanks, @jamessan. I'm going back and forth about this. In that case, should it trigger just after VimEnter too?

Contributor

fmoralesc commented Dec 23, 2014

Thanks, @jamessan. I'm going back and forth about this. In that case, should it trigger just after VimEnter too?

@fmoralesc

This comment has been minimized.

Show comment
Hide comment
@fmoralesc

fmoralesc Dec 23, 2014

Contributor

The thing is, if it triggers on opening new tab pages, it becomes less flexible from an API point, because the events (WinNew and TabNew) overlap. The same goes for VimEnter.

Contributor

fmoralesc commented Dec 23, 2014

The thing is, if it triggers on opening new tab pages, it becomes less flexible from an API point, because the events (WinNew and TabNew) overlap. The same goes for VimEnter.

@jamessan

This comment has been minimized.

Show comment
Hide comment
@jamessan

jamessan Dec 23, 2014

Member

BufNew doesn't trigger when starting Vim, so I would say no.

I'm not sure how it becomes less flexible. There are tab-specific actions one may want to take and there are window-specific actions one may want to take. This provides hook points for both of them.

Member

jamessan commented Dec 23, 2014

BufNew doesn't trigger when starting Vim, so I would say no.

I'm not sure how it becomes less flexible. There are tab-specific actions one may want to take and there are window-specific actions one may want to take. This provides hook points for both of them.

@jamessan

This comment has been minimized.

Show comment
Hide comment
@jamessan

jamessan Dec 23, 2014

Member

As for your earlier gists, I think the TabEnter version can be simplified to this, which I would argue is simpler than the TabNew implementation.

Member

jamessan commented Dec 23, 2014

As for your earlier gists, I think the TabEnter version can be simplified to this, which I would argue is simpler than the TabNew implementation.

@fmoralesc

This comment has been minimized.

Show comment
Hide comment
@fmoralesc

fmoralesc Dec 23, 2014

Contributor

@jamessan I didn't take into account t: variables, you have a point there. This nullifies somewhat the need for TabNew. A similar argument can be made against WinNew.

On the previous point, I meant to say that with this set of patches one can already trigger events on tab creation, and those could be used instead of the events for window creation on that particular use-case. Of course, the semantics are different, but in that sense, it makes sense to trigger WinNew after VimEnter. because there is a window (unless one would say the the initial view is not linked to a buffer yet). The scenario is similar to that of BufNew not triggering after VimEnter.

Contributor

fmoralesc commented Dec 23, 2014

@jamessan I didn't take into account t: variables, you have a point there. This nullifies somewhat the need for TabNew. A similar argument can be made against WinNew.

On the previous point, I meant to say that with this set of patches one can already trigger events on tab creation, and those could be used instead of the events for window creation on that particular use-case. Of course, the semantics are different, but in that sense, it makes sense to trigger WinNew after VimEnter. because there is a window (unless one would say the the initial view is not linked to a buffer yet). The scenario is similar to that of BufNew not triggering after VimEnter.

@jamessan

This comment has been minimized.

Show comment
Hide comment
@jamessan

jamessan Dec 23, 2014

Member

Indeed, the use of t:/w:/b: along with the relevant Enter autocmd could cover all uses of the proposed New autocmds. It's slightly simpler to use the New autocmds, especially if you can convince Bram that they should occur at Vim start as well.

Autocmds have historically been one of Vim's more brittle pieces of functionality since various state needs to be setup properly, but since these are triggered in very limited situations that's probably not as problematic.

Member

jamessan commented Dec 23, 2014

Indeed, the use of t:/w:/b: along with the relevant Enter autocmd could cover all uses of the proposed New autocmds. It's slightly simpler to use the New autocmds, especially if you can convince Bram that they should occur at Vim start as well.

Autocmds have historically been one of Vim's more brittle pieces of functionality since various state needs to be setup properly, but since these are triggered in very limited situations that's probably not as problematic.

@fmoralesc

This comment has been minimized.

Show comment
Hide comment
@fmoralesc

fmoralesc Dec 23, 2014

Contributor

Yeah, you made me wonder about the whole thing. I guess I didn't think it through. These events could be implemented like this:

au! BufEnter * if !exists('t:new_tab') | let t:new_tab = 1 | doautocmd User TabNew | endif
au! BufEnter * if !exists('w:new_window') | let w:new_window = 1 | doautocmd User WinNew | endif

and then used like this

au! User TabNew :echom "new tab"
au! User WinNew :echom "new window"

I'm not sure what approach is more efficient, though, although I would expect the gains to be minimal either way.

:/

EDIT: Notice the semantics of these User events are different to those in this PR, because they trigger much later.

Contributor

fmoralesc commented Dec 23, 2014

Yeah, you made me wonder about the whole thing. I guess I didn't think it through. These events could be implemented like this:

au! BufEnter * if !exists('t:new_tab') | let t:new_tab = 1 | doautocmd User TabNew | endif
au! BufEnter * if !exists('w:new_window') | let w:new_window = 1 | doautocmd User WinNew | endif

and then used like this

au! User TabNew :echom "new tab"
au! User WinNew :echom "new window"

I'm not sure what approach is more efficient, though, although I would expect the gains to be minimal either way.

:/

EDIT: Notice the semantics of these User events are different to those in this PR, because they trigger much later.

@jamessan

This comment has been minimized.

Show comment
Hide comment
@jamessan

jamessan Dec 23, 2014

Member

Well, actually creating new autocmds means people don't have to implement those two doautocmd events everywhere which is useful in and of itself. I certainly wasn't trying to be dismissive of your idea, just hashing out what's already available.

I think the fact that Bram seems receptive to the idea is awesome (since I still prefer to see things, which don't leverage Neovim-specific functionality, flow from Vim to Neovim), so run with it while you've got his attention. :) I'd see if you can get an opinion on having the *New events triggered on startup, too.

Member

jamessan commented Dec 23, 2014

Well, actually creating new autocmds means people don't have to implement those two doautocmd events everywhere which is useful in and of itself. I certainly wasn't trying to be dismissive of your idea, just hashing out what's already available.

I think the fact that Bram seems receptive to the idea is awesome (since I still prefer to see things, which don't leverage Neovim-specific functionality, flow from Vim to Neovim), so run with it while you've got his attention. :) I'd see if you can get an opinion on having the *New events triggered on startup, too.

@fmoralesc

This comment has been minimized.

Show comment
Hide comment
@fmoralesc

fmoralesc Dec 23, 2014

Contributor

OK, then, I'll persist ;)

The thing is, I believe I would have to create a couple more events, TabNewEnter and WinNewEnter, with the semantics of those User events, because most users will actually want that behavior (the original TabNew gist would become au TabNewEnter * Startify). This would mean I'll have to keep some state outside the functions I'm currently plugging into, so we'll see how that goes.

Contributor

fmoralesc commented Dec 23, 2014

OK, then, I'll persist ;)

The thing is, I believe I would have to create a couple more events, TabNewEnter and WinNewEnter, with the semantics of those User events, because most users will actually want that behavior (the original TabNew gist would become au TabNewEnter * Startify). This would mean I'll have to keep some state outside the functions I'm currently plugging into, so we'll see how that goes.

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Dec 23, 2014

Member

Tests would be good too.

Member

justinmk commented Dec 23, 2014

Tests would be good too.

@fmoralesc

This comment has been minimized.

Show comment
Hide comment
@fmoralesc

fmoralesc Dec 23, 2014

Contributor

I've been working on the semantics of these events. I implemented a couple of new events, TabNewEntered and WinNewEntered, which work as the User events described above. This allows for the startify use case to become

au! TabNewEntered * Startify

The main thing is that the current buffer for those events is the one linked to the current window, so bufnr("") is more intuitive, etc.

The WinNew and TabNew events merely signal that a new window and tab page are created, and they both trigger before the buffer is entered. I think there might a case for a intermediate event between WinNew and WinNewEntered: WinNewEntered won't be triggeted on :vsplit, because the focus doesn't change (other commands might cause this too). We might want to have an event that triggers when the buffer is created regardless of it gaining focus.

Contributor

fmoralesc commented Dec 23, 2014

I've been working on the semantics of these events. I implemented a couple of new events, TabNewEntered and WinNewEntered, which work as the User events described above. This allows for the startify use case to become

au! TabNewEntered * Startify

The main thing is that the current buffer for those events is the one linked to the current window, so bufnr("") is more intuitive, etc.

The WinNew and TabNew events merely signal that a new window and tab page are created, and they both trigger before the buffer is entered. I think there might a case for a intermediate event between WinNew and WinNewEntered: WinNewEntered won't be triggeted on :vsplit, because the focus doesn't change (other commands might cause this too). We might want to have an event that triggers when the buffer is created regardless of it gaining focus.

@fmoralesc fmoralesc changed the title from [WIP] Add TabNew and WinNew events to [WIP] Add TabNew, WinNew, TabNewEntered, WinNewEntered events Dec 23, 2014

@jamessan

This comment has been minimized.

Show comment
Hide comment
@jamessan

jamessan Dec 23, 2014

Member

Why do TabNew and WinNew need the buffer? If you're trying to do something buffer-specific, then these events don't seem relevant.

What use case are you trying to solve with TabNewEntered/WinNewEntered that can't be solved with WinNew + WinEnter/TabNew + TabEnter?

WinNewEntered won't be triggered on :vsplit, because the focus doesn't change (other commands might cause this too).

This is inaccurate. In the default setup (i.e., 'nosplitbelow' and 'nosplitright' and no :botright modifier), :vsplit creates a new window and pushes the previous window to the right. The cursor is in the new window. WinEnter is triggered and so should WinNew.

Member

jamessan commented Dec 23, 2014

Why do TabNew and WinNew need the buffer? If you're trying to do something buffer-specific, then these events don't seem relevant.

What use case are you trying to solve with TabNewEntered/WinNewEntered that can't be solved with WinNew + WinEnter/TabNew + TabEnter?

WinNewEntered won't be triggered on :vsplit, because the focus doesn't change (other commands might cause this too).

This is inaccurate. In the default setup (i.e., 'nosplitbelow' and 'nosplitright' and no :botright modifier), :vsplit creates a new window and pushes the previous window to the right. The cursor is in the new window. WinEnter is triggered and so should WinNew.

@fmoralesc

This comment has been minimized.

Show comment
Hide comment
@fmoralesc

fmoralesc Dec 23, 2014

Contributor

Why do TabNew and WinNew need the buffer?

They don't. That's the reason I created the *Entered variants. See the behavior of:

au TabNew * echom "tabnew:".tabpagenr().":".bufnr('')
au TabEnter * echom "tabnew:".tabpagenr().":".bufnr('')
au TabNewEntered * echom "tabnewentered:".tabpagenr().":".bufnr('')

TabNewEntered triggers like BufEnter, but only if a new tab page is created. Likewise with WinNewEnter. TabNew+TabEnter is not equivalent.

This is inaccurate. In the default setup (i.e., 'nosplitbelow' and 'nosplitright' and no :botright modifier), :vsplit creates a new window and pushes the previous window to the right.

Sorry, I missed that, this is a bug in the implementation then.

Contributor

fmoralesc commented Dec 23, 2014

Why do TabNew and WinNew need the buffer?

They don't. That's the reason I created the *Entered variants. See the behavior of:

au TabNew * echom "tabnew:".tabpagenr().":".bufnr('')
au TabEnter * echom "tabnew:".tabpagenr().":".bufnr('')
au TabNewEntered * echom "tabnewentered:".tabpagenr().":".bufnr('')

TabNewEntered triggers like BufEnter, but only if a new tab page is created. Likewise with WinNewEnter. TabNew+TabEnter is not equivalent.

This is inaccurate. In the default setup (i.e., 'nosplitbelow' and 'nosplitright' and no :botright modifier), :vsplit creates a new window and pushes the previous window to the right.

Sorry, I missed that, this is a bug in the implementation then.

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Dec 23, 2014

Member

au TabNewEntered *

What is <afile> for each event type? Or what does the * match against?

Member

justinmk commented Dec 23, 2014

au TabNewEntered *

What is <afile> for each event type? Or what does the * match against?

@fmoralesc

This comment has been minimized.

Show comment
Hide comment
@fmoralesc

fmoralesc Dec 23, 2014

Contributor

@justinmk Same as TabEnter and WinEnter. * can be used to match against files to edit.

Contributor

fmoralesc commented Dec 23, 2014

@justinmk Same as TabEnter and WinEnter. * can be used to match against files to edit.

@fmoralesc

This comment has been minimized.

Show comment
Hide comment
@fmoralesc

fmoralesc Jan 30, 2015

Contributor

I decided to reduce the scope of this to the Tab* events, I don't think I'll be able to advance much with the Win* events because of lack of time (also, the other PRs, like #1793 and #1667). I'm marking as RFC too because I think this set of events is behaving well.

Contributor

fmoralesc commented Jan 30, 2015

I decided to reduce the scope of this to the Tab* events, I don't think I'll be able to advance much with the Win* events because of lack of time (also, the other PRs, like #1793 and #1667). I'm marking as RFC too because I think this set of events is behaving well.

@fmoralesc fmoralesc changed the title from [WIP] Add TabNew, TabNewEntered, TabClose events to [RFC] Add TabNew, TabNewEntered, TabClose events Jan 30, 2015

@marvim marvim added RFC and removed WIP labels Jan 30, 2015

Show outdated Hide outdated runtime/doc/autocmd.txt Outdated
Show outdated Hide outdated src/nvim/fileio.c Outdated
@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Jan 30, 2015

Member

Also, the test for TabClose is currently incomplete, because without patch 530 I can't do 2tabonly (foe example), which I would want to check that the tab numbers are updated correctly. I might make a separate PR for it.

Will wait for 530 to get merged.

Member

justinmk commented Jan 30, 2015

Also, the test for TabClose is currently incomplete, because without patch 530 I can't do 2tabonly (foe example), which I would want to check that the tab numbers are updated correctly. I might make a separate PR for it.

Will wait for 530 to get merged.

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Jan 31, 2015

Member

One last nitpick: I see you named TabNewEntered with past tense, I think that's a good convention (and use present participle for "before", i.e. TabNewEntering). But then TabClose uses imperative voice.

I think either TabClose should be renamed to TabClosed, or TabNewEntered should be renamed to TabNewEnter. We can choose either approach since vim isn't consistent about any of it :/

Member

justinmk commented Jan 31, 2015

One last nitpick: I see you named TabNewEntered with past tense, I think that's a good convention (and use present participle for "before", i.e. TabNewEntering). But then TabClose uses imperative voice.

I think either TabClose should be renamed to TabClosed, or TabNewEntered should be renamed to TabNewEnter. We can choose either approach since vim isn't consistent about any of it :/

@fmoralesc

This comment has been minimized.

Show comment
Hide comment
@fmoralesc

fmoralesc Jan 31, 2015

Contributor

I like TabClosed better, I think I used TabClose because of the original idea proposal by @traycerb.

Contributor

fmoralesc commented Jan 31, 2015

I like TabClosed better, I think I used TabClose because of the original idea proposal by @traycerb.

@fmoralesc

This comment has been minimized.

Show comment
Hide comment
@fmoralesc

fmoralesc Feb 1, 2015

Contributor

OK. I updated the TabClosed tests to use the command ranges syntax from #1793, so it should fail until that is merged. In my local tests (rebasing over that set of patches) it behaves as expected.

Contributor

fmoralesc commented Feb 1, 2015

OK. I updated the TabClosed tests to use the command ranges syntax from #1793, so it should fail until that is merged. In my local tests (rebasing over that set of patches) it behaves as expected.

@fmoralesc

This comment has been minimized.

Show comment
Hide comment
@fmoralesc

fmoralesc Feb 1, 2015

Contributor

Ah! It seems like I didn't need the full command ranges syntax, so this should be good to go as-is.

Contributor

fmoralesc commented Feb 1, 2015

Ah! It seems like I didn't need the full command ranges syntax, so this should be good to go as-is.

@fmoralesc

This comment has been minimized.

Show comment
Hide comment
@fmoralesc

fmoralesc Feb 1, 2015

Contributor

I sent the updated patches to vim_dev at https://groups.google.com/forum/#!topic/vim_dev/bduueOtT-4c

Contributor

fmoralesc commented Feb 1, 2015

I sent the updated patches to vim_dev at https://groups.google.com/forum/#!topic/vim_dev/bduueOtT-4c

fmoralesc added some commits Dec 22, 2014

Add TabNew event
TabNew triggers when entering a new tab page, but not when entering an
already created one.
Add TabNewEntered
TabNewEntered is triggered after vim has entered a buffer in new tab.
Add TabClosed event
TabClosed is triggered when a tab page closes.
@fmoralesc

This comment has been minimized.

Show comment
Hide comment
@fmoralesc

fmoralesc Feb 17, 2015

Contributor

@justinmk Rebased, as requested in #1172

Contributor

fmoralesc commented Feb 17, 2015

@justinmk Rebased, as requested in #1172

@fmoralesc fmoralesc changed the title from [RFC] Add TabNew, TabNewEntered, TabClose events to [RFC] Add TabNew, TabNewEntered, TabClosed events Feb 17, 2015

@fmoralesc fmoralesc changed the title from [RFC] Add TabNew, TabNewEntered, TabClosed events to [RDY] Add TabNew, TabNewEntered, TabClosed events Feb 17, 2015

@marvim marvim added RDY and removed RFC labels Feb 17, 2015

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Feb 17, 2015

Member

After reviewing :help autocmd-events, and after comparing all the different names, here are some thoughts.

Only these events use the past tense:

EncodingChanged
TermChanged
GUIFailed
FileChangedShell
FileChangedShellPost
FileChangedRO
CmdUndefined
FuncUndefined
VimResized
FocusGained
CursorMoved
CursorMovedI
TextChanged
TextChangedI

TextChanged is fairly new, so it can't be said that the events are all legacy names. Also, use of past tense doesn't necessary imply chronology because FileChangedShell and FileChangedShellPost both exist.

Only nitpick that can be made about this PR, I think, is that TabNewEntered is inconsistent with the other FooEnter events. If that bothers anyone, I wouldn't be opposed to changing it to TabNewEnter. But otherwise I think we have done a reasonable job here.

Member

justinmk commented Feb 17, 2015

After reviewing :help autocmd-events, and after comparing all the different names, here are some thoughts.

Only these events use the past tense:

EncodingChanged
TermChanged
GUIFailed
FileChangedShell
FileChangedShellPost
FileChangedRO
CmdUndefined
FuncUndefined
VimResized
FocusGained
CursorMoved
CursorMovedI
TextChanged
TextChangedI

TextChanged is fairly new, so it can't be said that the events are all legacy names. Also, use of past tense doesn't necessary imply chronology because FileChangedShell and FileChangedShellPost both exist.

Only nitpick that can be made about this PR, I think, is that TabNewEntered is inconsistent with the other FooEnter events. If that bothers anyone, I wouldn't be opposed to changing it to TabNewEnter. But otherwise I think we have done a reasonable job here.

@fmoralesc

This comment has been minimized.

Show comment
Hide comment
@fmoralesc

fmoralesc Feb 17, 2015

Contributor

Sure. TabNewEntered's name was bound to be a point of contention, since it works like a buffer event, but is more meaningful as a tab page event. BufEnterOnNewTab would be the more descriptive option (if only BufEnter wasn't triggered also), but it is a mouthful.

Contributor

fmoralesc commented Feb 17, 2015

Sure. TabNewEntered's name was bound to be a point of contention, since it works like a buffer event, but is more meaningful as a tab page event. BufEnterOnNewTab would be the more descriptive option (if only BufEnter wasn't triggered also), but it is a mouthful.

justinmk added a commit that referenced this pull request Feb 17, 2015

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Feb 17, 2015

Member

merged

Member

justinmk commented Feb 17, 2015

merged

@justinmk justinmk closed this Feb 17, 2015

@justinmk justinmk removed the RDY label Feb 17, 2015

@ghost ghost referenced this pull request Feb 21, 2015

Merged

Upcoming Newsletter #82

22 of 22 tasks complete

@fmoralesc fmoralesc deleted the fmoralesc:tabnew branch Jul 23, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment