daa84 / neovim-gtk

gtk ui for neovim
GNU General Public License v3.0
716 stars 57 forks source link

Add patterns to event subscriptions and test with settable title prefix #135

Closed LouisJackman closed 5 years ago

LouisJackman commented 5 years ago

Events were subscribed and keyed with their event name; now the combination of the event name and a matching pattern is used instead. This allows Subscriptions#subscribe to be used with patterns too.

A real world use case for this is addressing issue #39, which this PR does as a test case.

daa84 commented 5 years ago

Instead of add new event subscription maybe better to use global events that already come from nvim? https://neovim.io/doc/user/ui.html#ui-global I mean set_title event. And add case here: https://github.com/daa84/neovim-gtk/blob/07284db822e5fc6a7cadc3c1ae0fe74edc290166/src/nvim/redraw_handler.rs#L209

Does this solution have some drawbacks?

LouisJackman commented 5 years ago

I did it that way to ensure it played well with the title changing in the existing BufEnter and DirChanged subscriptions. Your suggestion about using a global UI event makes sense though, and surely there's a way to get it to play well with the existing event subscriptions. I'll take a look.

The title changing was mostly for testing the event pattern support added to the subscription system. Even without title changing using it, it could be useful for future additions.

LouisJackman commented 5 years ago

I experimented with a few alternatives using the global UI set_title event. They all seemed more complicated than the current code in the PR, so I'd advocate for the PR's current approach of set_option subscriptions.

New title changes must go through the subscription system to remain synchronised with the existing dynamic filename change in the title, which as you know are obtained via args evaluated from the subscription: expand('%:p) and getcwd(). It's possible to get these values without subscription args of course, but this mechanism is already established.

Furthermore, the global UI events trigger methods on the shell state, yet the set_title feature relies on access to the application window. This relies on passing something up to the shell state from the main Ui construction to allow triggering the setting of the window title. This is complicated further by the shell state being setup in Ui::new and the window being created afterwards in Ui::init, meaning the connection must be made after shell state construction.

If you want to go with this approach, I can update the PR. Honestly, it seems more complicated that just adding another subscription though. Of course, there may be a better solution I haven't thought of given that I'm new to this codebase, so suggestions are welcome.

daa84 commented 5 years ago

Ok, then i'll check how it works and merge

daa84 commented 5 years ago

I do some tests - titlestring in vim can contains not only string but also some patterns. :h titlestring, this works strange if user already have configured settings. something like expand("titlestring_here") - maybe fix this problem? Also think in case titlestring exists - it is better to replace all title and not add prefix to it. So pattern will be something like neovim - %:t?

LouisJackman commented 5 years ago

Good catch. It now replaces rather than prefixes, and expands patterns, but it appears that the standard expand function doesn't expand in quite the same way as setting titlestring or statusline, so I'll need to think of another solution for that. I should have read :h titlestring better.

Assuming the global set_title event passes in patterns already processed, that'd be the way to do it here, as you originally suggested in your first comment. I'll put something together for that original plan.

daa84 commented 5 years ago

@LouisJackman i think to use this pr not for titlestring but for completeopt

LouisJackman commented 5 years ago

Allowing customisable matching patterns for subscriptions should be generally useful even though my code modifying set titlestring was admittedly a bad example.

Apologies for not getting around to that PoC for titlestring, however I saw other NeoVim GUIs often changing the matching pattern when setting up subscriptions so I assumed neovim-gtk would eventually encounter a similar case.

daa84 commented 5 years ago

Merge subscription changes but replace title to completeopt handle.

Thanks :smile: