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

Updated instance of a-sync call for tagbar originally implemented by @pangchol #663

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

raven42
Copy link
Collaborator

@raven42 raven42 commented Sep 25, 2020

Closes #532

Update of PR #561 originally implemented by @pangchol

This uses a lambda callback to pass the parameters into the callback routine.

note: I kept the has('win32') bypass logic in there as per the comment left by @pangchol. I don't have a win32 instance to attempt this on, so I can't speak to the comment about timer_start() not working correctly on windows.

Currently this is a partial implementation. It has not been heavily tested.

@raven42 raven42 requested review from alerque and a team September 25, 2020 20:51
@alerque
Copy link
Member

alerque commented Sep 25, 2020

I won't be able to give this a real shakedown for a week or so, I'm over my eyeballs. There are a couple other people that have access and might be able to offer a review and any member review approval will work so you can merge this. It looks sane to me I just can't test it right now.

By the way I'd suggest squashing the last 4 commits here into your main one since they are all fixups of that. This should be just the original contribution, a merge commit, and your fixups.

If I forget about this in 1.5 weeks feel free to ping me! Great work recently by the way.

@raven42
Copy link
Collaborator Author

raven42 commented Sep 28, 2020

So did a little testing with this... as per @noscript's comment on NERDTree#1170 where it was stated that the timer starts in the same thread, I added a delay in the timer, then some additional debug logs, then opened a large file to see the behavior. This is indeed correct that the delay will always be present, the timer_start() will only delay the operation. So in my testing, when delaying for 5 seconds before starting the AutoUpdate_CB() routine, I did see the file load right away and I was able to move the cursor around, however after the 5 second timer, vim did lock up for a bit while the ctags were being processed. So this isn't really a solution for an async call, but just delays the call that is made. A different solution will have to be found.

@alerque alerque marked this pull request as draft September 28, 2020 13:52
Copy link
Member

@alerque alerque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As found by @raven42, this doesn't really solve the problem, it only delays it. Marking as Draft so it's obvious we're not ready to merge this until it actually solves the problem.

@raven42
Copy link
Collaborator Author

raven42 commented Oct 6, 2020

We may want to look at integration with the async-run plugin by @skywind3000 to do this. It looks like that may be the best way to do a background process within a single threaded vim instance. It looks like that opens a new buffer in a temp file and all output gets written to that buffer. I'm not sure how the buffer management works in tagbar though, so not sure if this would be feasible.

@noscript
Copy link

noscript commented Oct 6, 2020

Sorry to invade the thread, but you cannot run vim functions in the background and async-run plugin is not helping there. However what is possible in theory is fire up another vim instance and run some heavy vimL code there and return the results via channel.

@raven42
Copy link
Collaborator Author

raven42 commented Oct 6, 2020

@noscript Ya we understand that the vim functions themselves will not be able to run in the background. We are looking at the external ctags call to run in the background. I am not sure of the performance impact of the processing of the ctags output though, so doing the actual processing in another vim instance and returning data via the channel interface as you mentioned might be a good idea for that though. I am not very familiar with a lot of that though, so I'm not sure I would be the best person to implement it. Mainly just documenting the possible approaches here incase there is somebody that can take this up, or if I am able to learn more about it and able to work on it myself.

@noscript
Copy link

noscript commented Oct 6, 2020

Using job_start() is the best solution in this case but s:run_system() will have to be split into two calls.

Before:

        silent let ctags_output = s:run_system(a:ctags_cmd, py_version)

After:

        call s:run_system(a:ctags_cmd, function("s:foo"))

        function! s:foo(msg)
            let ctags_output = a:msg
            " do something with ctags_output
        endfunction

" ...

function! s:run_system(cmd, callback) abort
    if exists('g:tagbar_job') && job_status(g:tagbar_job) == "run"
        return
    endif
    let g:tagbar_job = job_start(a:cmd, #{ out_cb: function("s:run_system_post", [a:callback]) })
endfunction

function! s:run_system_post(callback, channel, msg) abort
    let Func = function(a:callback)
    call Func(a:msg)
endfunction

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

suggest let s:AutoUpdate function run async
3 participants