Timoses / vim-venu

Simple menu for vim
20 stars 2 forks source link

Add a user event just before VenuPrint #9

Open ozars opened 5 years ago

ozars commented 5 years ago

In the plugin that I'm using with Venu, vim native menus are modified upon entering and leaving a buffer (on BufEnter and BufLeave). Currently, vim doesn't provide any proper ways to detect changes on menu. Moreover, when I use vemu#native#import in my .vimrc, printed menu does not include these entries since BufEnter is not called yet. I source my .vimrc again after I go to that buffer to include those menu entries, but that's certainly not a good workaround.

Can we add a custom event e.g. User VenuPrint whose attached handlers called just before :VenuPrint command? In this way I can attach a handle to this event to empty all Venu entries and reimport them just before :VenuPrint so that it will print up-to-date menu.

On a side note, this also requires something like venu#unregisterAll() or venu#clear(). #4 has a todo item for unregistering a menu, but it would really make things much simpler if there would be 'remove all menus' function. I believe it is also much easier to implement compared to unregistering a certain menu.

Thanks!

Timoses commented 5 years ago

4 was merged and contains a venu#unregisterAll().

Regarding the event, I'm thinking of something like:

venu#create(<name>, <eventHandle>)

where <eventHandle> is called:

Then this eventHandle function receives a menu-handle which it can add items to (or remove existing ones if needed).

That way when creating a menu it can be updated dynamically whenever necessary.

The only part I haven't figured out is how it would interact with merging menus which have the same name. E.g. when there is a static menu registered with the name MyMenu and also a menu which is added to with an event handle with the same name MyMenu. I suppose after the the user's <eventHandle> was called, venu would have to redo some merging attempts... This sounds a little complex still. Some more thought has to go into that : D.

What do you think?

ozars commented 5 years ago

4 was merged and contains a venu#unregisterAll().

Awesome, thanks!

Regarding the event, I'm thinking of something like:

venu#create(<name>, <eventHandle>)

This might be good idea, but it involves many implementation details since traversing menu entries and moving them around requires some work. I found unregistering all menus and repopulating them just before VenuPrint every time is much easier to implement and manage. Performance difference should be insignificant as users are not supposed to have many millions of menu entries (I guess? :-) )

The only part I haven't figured out is how it would interact with merging menus which have the same name. E.g. when there is a static menu registered with the name MyMenu and also a menu which is added to with an event handle with the same name MyMenu. I suppose after the the user's <eventHandle> was called, venu would have to redo some merging attempts... This sounds a little complex still. Some more thought has to go into that : D.

I'm not sure if there is a simpler solution. I couldn't come up with a good solution on top of my head as well other than straight-forward refill method I mentioned above. I actually implemented above idea as a temporary solution and it works well so far: https://github.com/ozars/vim-venu/commit/3030902f7c61770c3d2693fdd3c40c5d63d24e60 It's not necessarily better, but it is definitely much simpler to implement and maintain.

This is how vimrc may look like with the above solution: https://github.com/ozars/dotfiles/blob/8e52f1e354fe2bd3f1b3f5464e2fa55e438db897/vimrc#L619-L632

So, I passed the responsibility of refilling menus to the user as well. I don't know if I am asking too much from users. :-)

Timoses commented 5 years ago

I'm trying to keep flexibility in mind. E.g. a user might want to import only a specific :amenu section into a specific menu. So if the user could do

function! updateMyMenu(menu)
    venu#clear(a:menu)
    " ... steps to repopulate the menu
endfunction

venu#create("Mymenu", function("updateMyMenu"))

A simple :VenuUpdate would then update the menu.

I'd favor this approach over autocommands. What do you think?

This might be good idea, but it involves many implementation details since traversing menu entries and moving them around requires some work.

Would above solve this?

I think it could make sense to initially disallow merging for menus that have an update handle.

ozars commented 5 years ago

I'm trying to keep flexibility in mind. E.g. a user might want to import only a specific :amenu section into a specific menu.

I'd prefer using filter callback while I reimport all menus before printing. That said, that I prefer something else doesn't make it unnecessary. Providing another option to users would be nice.

I'd favor this approach over autocommands. What do you think?

Is it possible having it both ways? I'd prefer doing it with autocmd because I don't make direct calls to venu#create, instead it's handled through venu#native#import call in my conf.

ozars commented 5 years ago

Implementing StartVenuPrintCallback would be preferable over custom event. Please also see L198-203 in the commit https://github.com/ozars/vim-venu/commit/afba5d0a5e0d8280b49fe59807ca593fd665fd51 and #13 about a related discussion.

Timoses commented 5 years ago

In hindsight, I believe the autocmd approach is solid.

What is the StartVenuCallback for? It looks like letting the user plug into venu that early pretty much removes the necessity to even use venu (as the user would have to either implement all himself or manually call s:startVenu). What is the use case you're trying to map? Do you have examples of how you use this?

ozars commented 5 years ago

What is the StartVenuCallback for?

It is the function called by venu#print calls after setting up callbacks to either user-provided options or to library defaults. In the way I used it, I clear all venus and reimport from native menu: https://github.com/ozars/dotfiles/blob/98af59b513fdbf6ac525922bcc9cb6a9d059393f/vimrc#L621-L636

It looks like letting the user plug into venu that early pretty much removes the necessity to even use venu (as the user would have to either implement all himself or manually call s:startVenu).

It gives a way to inject user callback before (and after) menus are printed. You are correct in your observation though, that's why this callback needs to call venu#defaultStartVenuCallback(). So, this gives user to do stuff before/after printing menu. This is like having BeforePrint and AfterPrint events.