Thanks. Did you notice any performance issues?

Thanks. Did you notice any performance issues?

I haven't actually tried it myself ;) A reasonable test might be to run a :terminal, create lots of prompt and output lines, set the variable, and run the plugin to see what happens. I just haven't done it yet.

I think you would need to create a lot of lines for performance to be an issue. This SHELL_PROMPT callback is called once per line to retrieve the property, which I would imagine should be fast. The plugin is doing a regex comparison per line anyway so you would have a linear scaling no matter what and the regex comparison is probably the limiting factor.

But we could potentially create a function, e.g. named UpdateUserSettings() that's called at the beginning of SetToc() and that could update a static SHELL_PROMPT. This would mean it's only called once instead of per line. I'm not sure if it's necessary though, but it would be a better way to implement it IMO (would also be the time to see if a buffer-local variable exist instead of a global one so allow users to have multiple shell types).

But we could potentially create a function, e.g. named UpdateUserSettings() that's called at the beginning of SetToc() and that could update a static SHELL_PROMPT. This would mean it's only called once instead of per line. I'm not sure if it's necessary though, but it would be a better way to implement it IMO (would also be the time to see if a buffer-local variable exist instead of a global one so allow users to have multiple shell types).

[Assuming I understood what you meant while I'm away from the code] I like this idea of fetching possible changes on demand/as needed while still keeping the cost of the test low (a variable lookup rather than a function call). Doubly so if it provides a good place to hook in the other follow-up.

Let me prepare a v2 of this patch.

I've pushed a v2:

1:  52417744e < -:  --------- runtime/helptoc: uncache g:helptoc.shell_prompt
-:  --------- > 1:  a2a4c86dd runtime/helptoc: reload cached g:helptoc.shell_prompt when starting toc
-:  --------- > 2:  bad2ea947 make changes to user settings invalidate the cache

I noticed a bit of odd behavior when testing, so I added the tip bad2ea9 (make changes to user settings invalidate the cache, 2024-11-30). It invalidates a little too eagerly at the moment (for example, changing g:helptoc.shell_prompt probably should only invalidate terminal buffer caches), but also doesn't invalidate caches in other buffers (e.g., go through the steps in the commit message but with two terminal buffers: blanks in both, fix config, try again in both, and notice that only one is "fixed"), so the whole thing can be a bit confusing.

Comments welcome: in particular, I'd like to consider "changing SHELL_PROMPT" to invalidate all :terminal caches, but that might not be the right move if some terminals use different prompts (ugh). Of course, that could be better solved with a b: variable, which I'd like to tackle (or for someone else to tackle) in a separate series, probably.

Hmm my comments on the file didn't seem to show up in the GitHub PR, maybe because it's an individual commit, just pasting below.

Is there a reason we need this or any invalidation management code at all? Can't we just have the original code and update SHELL_PROMPT all the time in UpdateUserSettings()? This is just reading a variable from memory and shouldn't really need optimizations around it. (Maybe that's why I think it would actually have been ok if we just call SHELL_PROMPT as a function per line, but either way works). Either way for validation to work you have to query the variable to begin with so you don't save anything.

I think to get buffer local we just need something like this:

def UpdateUserSettings() #{{{2
    SHELL_PROMPT = b:
        ->get('helptoc', {})
        ->get('shell_prompt', g:
            ->get('helptoc', {})
            ->get('shell_prompt', '^\w\+@\w\+:\f\+\$\s'))
enddef

Hmm my comments on the file didn't seem to show up in the GitHub PR, maybe because it's an individual commit, just pasting below.

I did get a notification about the comments, but if you don't leave them through the commits tab or files changed tab and press "Submit Review" (I think) they won't show up as a "Review."

Is there a reason we need this or any invalidation management code at all? Can't we just have the original code and update SHELL_PROMPT all the time in UpdateUserSettings()? This is just reading a variable from memory and shouldn't really need optimizations around it. (Maybe that's why I think it would actually have been ok if we just call SHELL_PROMPT as a function per line, but either way works). Either way for validation to work you have to query the variable to begin with so you don't save anything.

See above (some formatting lost):

I wrote:

I noticed a bit of odd behavior when testing, so I added the tip bad2ea9 (make changes to user settings invalidate the cache, 2024-11-30). It invalidates a little too eagerly at the moment (for example, changing g:helptoc.shell_prompt probably should only invalidate terminal buffer caches), but also doesn't invalidate caches in other buffers (e.g., go through the steps in the commit message but with two terminal buffers: blanks in both, fix config, try again in both, and notice that only one is "fixed"), so the whole thing can be a bit confusing.

Comments welcome: in particular, I'd like to consider "changing SHELL_PROMPT" to invalidate all :terminal caches, but that might not be the right move if some terminals use different prompts (ugh). Of course, that could be better solved with a b: variable, which I'd like to tackle (or for someone else to tackle) in a separate series, probably.

and the commit message:

Let's say in a terminal buffer we run :HelpToc but it creates a blank
popup because none of our lines match SHELL_PROMPT. We fix that with

:let g:helptoc = #{shell_prompt '…'}

and rerun :HelpToc. The blank popup appears again! If we run a new
command or modify the buffer some other way, :HelpToc starts working.

This is because of the cached popup buffer: it wasn't invalidated by the
change to g:helptoc.

Try it out. For example, my PS1 usually contains a lambda character followed by a space; when I do :terminal, a few random things, then <C-w><C-n>:HelpToc<enter> the popup is blank. Running

:let g:helptoc = #{shell_prompt: 'λ '}
:HelpToc

doesn't fix it without this commit, :unlet b:toc, or A<another command> (to modify the terminal).

ychin wrote:

I think to get buffer local we just need something like this:

def UpdateUserSettings() #{{{2
    SHELL_PROMPT = b:
        ->get('helptoc', {})
        ->get('shell_prompt', g:
            ->get('helptoc', {})
            ->get('shell_prompt', '^\w\+@\w\+:\f\+\$\s'))
enddef

This looks right to me, but again I'd rather address that separately (esp. given the cache invalidation bits from refreshing config above).

To be a little more explicit: if you checkout a2a4c86 and do the example case I mentioned, you might be surprised by the results: the initial placement of UpdateUserSettings() is too late to intercept the fact that the cached toc buffer has been used despite it being based on old configuration.

Can't we just have the original code and update SHELL_PROMPT all the time in UpdateUserSettings()?

Yes, if we don't have a cached toc buffer ;)

There might be a way to do things without the check in UpdateUserSettings, but somewhere before the b:toc is used and after g:helptoc has changed we need to know to skip using b:toc.

Ah you are right. For some reasons I read what you wrote but didn't register. Looks good to me then.

thanks!