Skip to content
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

Provides requires #2782

Merged
merged 18 commits into from Apr 25, 2019

Conversation

Projects
None yet
5 participants
@laelath
Copy link
Contributor

commented Mar 13, 2019

Implemented provide-module and require-module commands from #2772.
Also added a ModuleLoad hook which is run when a module is required for the first time. This is needed for setting options that are no longer immediately declared, such as termcmd.

I'm seeing an ~2x speed up in load time on my laptop and ~3x speedup on a raspberry pi.

As this required editing a lot of files, there's a high likelihood that I've broken some functionality, but everything I've tested except for markdown/restructured text works, which don't highlight code sections unless you manually load the relevant module.

@Screwtapello

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2019

Should these new commands be added to doc/pages/commands.asciidoc, and the new hook to hooks.asciidoc? Probably also something in changelog.asciidoc.

@mawww

This comment has been minimized.

Copy link
Owner

commented Mar 16, 2019

Thanks for working on this,

I was initially suprised by the addional hook on BufSetOption, but I understand this makes the module work for files where we manually set the filetype.

I was trying to understand how this managed to work, as I expected the following problem:

  1. User/hook sets filetype to cpp
  2. The added hooks requires-module cpp
  3. Somehow magically, hooks added by the cpp module are run as well for this filetype change.

This 3. was surprising to me, because Kakoune is supposed not to execute those newly added hooks until the hook itself gets triggered again. And yet, it does seems to work...

After a bit of debugging, I understood what was going on: The require-module command is triggered by BufSetOption while most filetype support hooks added by that module are triggered by WinSetOption which happens to run afterwards, making all that work.

I dont think this is good enough, it works when we load based on filetype as long as modules do add their hooks at window scope, but if we had a module adding BufSetOption hooks, I think it would be reasonable to expect that to work.

So we either need a way to re-trigger the BufSetOption hook after running require-module, or modify the design so that it works by construction. One solution could be to change how Kakoune handle hooks to guarantee that if we add new hook commands for the currently executing hook, they will be executed.

@laelath

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2019

Yeah, I originally didn't plan to create the extra BufSetOption hook, but when implementing it I thought that we would want to maintain the ability to manually set the filetype without also requiring a manual require-module.

As for the hook situation, I agree that the way I implemented it is not ideal. I'm not sure about guaranteeing that new hooks are run for the currently executing hooks. It seems like it could be really powerful, but also easily infinitely loop (though maybe not really in practice?). The best way to change the design that I can think of would be to run unset-option buffer filetype, set-option buffer filetype <name> after the requires-module <name.

Show resolved Hide resolved doc/pages/hooks.asciidoc Outdated
@occivink

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2019

One solution could be to change how Kakoune handle hooks to guarantee that if we add new hook commands for the currently executing hook, they will be executed.

This was changed recently, I don't think we absolutely need to keep the current behavior but we have this usecase to think about as well.

@laelath laelath force-pushed the laelath:provides-requires branch 2 times, most recently from ea44703 to 317c80e Mar 21, 2019

@laelath laelath force-pushed the laelath:provides-requires branch from 317c80e to 1484a8c Mar 25, 2019

@laelath

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

After thinking about it, I'm not too sure the WinSetOption/BufSetOption is that big of an issue. Filetype modules are responsible for defining how they're loaded, so if a module used BufSetOption hooks to setup, that module would be responsible for making sure those hooks are run after the first load.

That being said, this solution is still kinda messy, and I would be happy to implement a more robust solution.

@mawww

This comment has been minimized.

Copy link
Owner

commented Mar 26, 2019

In my view it is problematic because module kakrc will require module sh, so sh cannot assume it is being loaded by a BufSetOption filetype=sh hook and hence would be wrong if it started to enable sh highlighting and indentation on that buffer.

The current implementation works by chance, because most filetype support hook on WinSetOption which gets triggered after BufSetOption. If a filetype was, as it was suggested a few times, to be added at buffer scope (so using BufSetOption), the module pattern that works in all other filetypes would break on this filetype, for reasons that are far from obvious...

One solution would be to change the behaviour of the hook command itself to eagerly execute the hooks commands if we are already running that hook. Might be surprising though as it means sometimes hook ... would work as evaluate-commands.

@laelath

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

A lot of hooks use the pattern:

hook -group kak-highlight global WinSetOption filetype=kak %{
    add-highlighter window/kakrc ref kakrc
    hook -once -always window WinSetOption filetype=.* %{ remove-highlighter window/kakrc }
}

Which would be broken by always eagerly executing the hooks.

Maybe it could be a switch in the hook command? Something like hook -eager ... it would reduce the amount of unexpected behavior at the cost of an esoteric switch. But this would mean the modules would have to anticipate that they could be loaded from those hooks.

Another option would be to require loading the module before setting filetype, since the module would be loaded before the execution of any of these hooks, like how I wrote it in the proposal. However this could be confusing to people who aren't aware of modules, which ideally would be invisible to users.

Finally, we could pull the initialization hooks out of the module, and have them require their own module, i.e.:

hook -group kak-highlight global WinSetOption filetype=kak %{
    require-module kak
    ...
}

hook global WinSetOption filetype=kak %~
    require-module kak
    ...
~

Thoughts?

@Screwtapello

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

I like the idiom of having all the hooks outside the module. As I understand it, hook statements generally aren't what slows down Kakoune startup, and I think it's easier to understand and reason about a system where all the hooks are set up at the start, instead of some of them appearing part-way through.

@laelath

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2019

Turns out many highlighters use a shell script to create a hook that adds the language's keywords to the static_words list, so moving the hooks outside the modules won't work. Having shell scripts outside of the modules would defeat one of the main goals of a module system.

@laelath

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

I've come up with this potential pattern. After experimenting I've found that changing options inside of *SetOption hooks results in unexpected behavior.

hook -once global BufSetOption filetype=(c|cpp|objc) %{
    require-module c-family
    hook -once buffer NormalIdle '' %sh{
        printf "
            unset-option buffer filetype
            set-option buffer filetype %s
        " $kak_hook_param_capture_1
    }
}
@mawww

This comment has been minimized.

Copy link
Owner

commented Mar 28, 2019

@laelath The static_words problem can be fixed another way, through I feature I have wanted to tackle for while: user scopes.

The idea is to introduce a way to create named scopes (say a cpp scope), add all the aliases, hooks, option values, highlighters that are specific to those directly inside that scope, and provide a way to add those scopes to the built-in ones:

enable-scope window cpp would add the cpp scope as a "delegate scope" of window, and would act as an additional "parent" of the window scope (so that say options in the window scope do override the value they might have in the cpp scope).

With that, the cpp module would mostly consist of setting up the cpp scope, and there would likely only be one hook that on filetype set to cpp would enable the cpp scope in the current window. The static_words would hence only be set once, eagerly during the cpp module execution, and there should be no need for hooks inside modules.

Of course that means we still have to implement all that... And it also means we are introducing two brand new concepts into Kakoune, which adds a bit of complexity.

As an aside, I have been wondering if we can get the module system implemented directly as a pattern using hooks (potentially adding resonable features to the hook system) as they are pretty close to each other, but did not find any satisfying design so far.

@laelath

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

Something similar to the pattern I was doing here would be to (ab)use the command overriding to make a command that only meaningfully runs once.

define-command load-c-family %{
    define-command -override load-c-family ""

    # c-family commands
}

Though this is basically just a replacement for provides/requires, and misses some nice things like erroring if you try and override a module that's already been loaded.

Named scopes sound really interesting! It sounds like a lot of the use cases of modules would be very related to named scopes. Maybe something like

# detection hooks

hook -once global ScopeEnable cpp %{
    # indention hooks, aliases, keywords
    require-module c-family
}

provide-module c-family %{
    # highlighters, commands
}
@mawww

This comment has been minimized.

Copy link
Owner

commented Apr 7, 2019

As a temporary stop-gap for static_words, we could make support script introduce their own cpp_static_words that gets filled at require time with the correct list of words, then have the hook just
do set buffer static_words %opt{cpp_static_words} after require.

This way we should have a robust solution to all the problems we identified so far, and we could go ahead with provide/requires support.

@laelath laelath force-pushed the laelath:provides-requires branch from 55ebb13 to 80ac46e Apr 10, 2019

@laelath

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

@mawww I've finished updating all the files. If it looks good I can create a squash commit for merging.

@mawww
Copy link
Owner

left a comment

While cute, I am not a big fan of using 🦀 for string delimiters as it will fail to display correctly on non-emoji supporting terminals. If we could use something from ascii it would be pretty nice. I am thinking it might make sense to introduce another string syntax (yet another...) such as:

%`word`
...
`word`

with word some arbitrary text.

@laelath

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2019

We're out of strictly ascii characters with some of the larger language support files, but we could use some characters from the Latin-1 block, like §, or ¶. We do kind of assume support for these, since we use » in the changelog.

@mawww mawww merged commit f49644e into mawww:master Apr 25, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@maximbaz

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

Nice job everyone 🎉

I have one question on the usage, previously I had this line in kakrc:

set-option global kitty_window_type 'os'

In the current master it fails with 'set-option' option not found: 'kitty_window_type'. Use declare-option first.

Adding require-module kitty before that line helps, but is that the intended usage pattern to require some stuff in kakrc, or there's another approach I'm missing?

@laelath

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

I put the windowing support into modules, so those options aren't defined until the module is first loaded, which is usually after the user's kakrc is loaded. The intended approach would be

hook ModuleLoad kitty %{
    set-option global kitty_window_type 'os'
}

The ModuleLoad hook is run when the module is first sourced by a require-module. Ideally, require-module should only be used when implementing support files and invisible to users. (except that hook)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.